Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Only focus/select when current state is focus/select
Browse files Browse the repository at this point in the history
Follow up tweak for #5389

The fix for the JS error in 83eccb5 actually introduced some test failures because that JS error was saving this code from getting hit.  This makes the tests pass and is more logical, if the prevState is selected and the current state it not selected, we don't want to select

Auditors: @diracdeltas
  • Loading branch information
bbondy committed Nov 12, 2016
1 parent 08bfc40 commit fcde124
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions js/components/urlBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -371,10 +371,10 @@ class UrlBar extends ImmutableComponent {
}
}
}
if (this.isFocused() !== prevProps.urlbar.get('focused')) {
if (this.isFocused() && !prevProps.urlbar.get('focused')) {
this.updateDOMInputFocus()

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Nov 13, 2016

Member

updateDOMInputFocus takes a focused argument, which is missing here, so i think this does nothing. my other PR addressed that by getting rid of the arg

This comment has been minimized.

Copy link
@bbondy

bbondy Nov 14, 2016

Author Member

do you know of any problem it causes? Maybe we should just remove the call here?

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Nov 14, 2016

Member

i don't know offhand why this call is here

This comment has been minimized.

Copy link
@bbondy

bbondy Nov 14, 2016

Author Member

I think I have to do the same for focus as I did for selection, I have a task to fix selection not remembering when you change tabs to work on still so I'll fix it as part of that.

}
if (this.isSelected() !== prevProps.urlbar.get('selected')) {
if (this.isSelected() && !prevProps.urlbar.get('selected')) {
this.select()
windowActions.setUrlBarSelected(false)

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Nov 13, 2016

Member

shouldn't this be true?

This comment has been minimized.

Copy link
@bbondy

bbondy Nov 14, 2016

Author Member

I don't think there's a way to properly track if it is selected, so it more means wantSelected, maybe we should rename to that. But that was intentional.

}
Expand Down

0 comments on commit fcde124

Please sign in to comment.