STR: (Win7_64, Nightly 45, 32bit, ID 20151108030417, new profile) 1. Click mouse on the empty searchbar 2. Hover mouse over location bar 3. Type "zxcv" in focused search bar [suggestions will appear] 4. Press Tab to select the first (1st) alternate search engine 5. Hover mouse over that first alternate search engine (or any other alternate engine!!!) 6. Press Enter Result: Firefox opens a page with search results in default search engine Expectations: It should open a page with search results in the 1st alternate engine, because I chose it in Step 4 Note: IRL this can happen accidentally; I make so complex STR because I want them to be reliable

Bug 1222971 - don't let mouseover/mouseout remove the selectedButton that was set, r?florian

Comment on attachment 8688960 [details] MozReview Request: Bug 1222971 - don't let mouseover/mouseout remove the selectedButton that was set, r?florian https://reviewboard.mozilla.org/r/25473/#review22949 This patch lets us search with a non-default engine that isn't highlighted. Steps to reproduce: 1. Press tab twice to select the second one-off button. 2. Hover the first one-off button (the higlight moves to engine 1 but engine 2 is still selected). 3. Move the mouse out (at this point, nothing is highlighted anymore; I think the highlight should move back to engine 2) ::: browser/components/search/content/search.xml:750 (Diff revision 1) > - <property name="selectedButton" onget="return this._selectedButton;"> > + <property name="selectedButton" readonly="true" onget="return this._selectedButton;"> I'm confused by this readonly property with a setter.

(In reply to Florian Quèze [:florian] [:flo] from comment #2) > This patch lets us search with a non-default engine that isn't highlighted. If I select 1st alternate engine with Tab, then hover the 2nd alternate engine with mouse - then: 1) What should be displayed in "suggestions status"? (Search in <engine name>, but what engine?) 2) What should happen when I press Enter? Wasn't that thought through in bug 1142334? Imo, this could be fixed by 2 types of highlight: default OS's - for selected with Tab and lightgray - for hovered. While "status" should only display "Search in <engine selected with Tab, if it was pressed. If it wasn't, then - hovered engine>"

Comment on attachment 8688960 [details] MozReview Request: Bug 1222971 - don't let mouseover/mouseout remove the selectedButton that was set, r?florian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25473/diff/1-2/

(In reply to Florian Quèze [:florian] [:flo] from comment #2) > Comment on attachment 8688960 [details] > MozReview Request: Bug 1222971 - don't let mouseover/mouseout remove the > selectedButton that was set, r?florian > > https://reviewboard.mozilla.org/r/25473/#review22949 > > This patch lets us search with a non-default engine that isn't highlighted. > > Steps to reproduce: > 1. Press tab twice to select the second one-off button. > 2. Hover the first one-off button (the higlight moves to engine 1 but engine > 2 is still selected). > 3. Move the mouse out (at this point, nothing is highlighted anymore; I > think the highlight should move back to engine 2) Fixed. Because of the ordering of mouseout and mouseover, this is slightly hacky. ... though actually, thinking about it, I think I can use relatedTarget to do better here. > ::: browser/components/search/content/search.xml:750 > (Diff revision 1) > > - <property name="selectedButton" onget="return this._selectedButton;"> > > + <property name="selectedButton" readonly="true" onget="return this._selectedButton;"> > > I'm confused by this readonly property with a setter. Oops, fixed.

(In reply to arni2033 from comment #3) > (In reply to Florian Quèze [:florian] [:flo] from comment #2) > > This patch lets us search with a non-default engine that isn't highlighted. > If I select 1st alternate engine with Tab, then hover the 2nd alternate > engine with mouse - then: > 1) What should be displayed in "suggestions status"? (Search in <engine > name>, but what engine?) The keyboard-selected button's engine name. > 2) What should happen when I press Enter? > Wasn't that thought through in bug 1142334? It was. Maybe I misunderstand what you're asking? > Imo, this could be fixed by 2 > types of highlight: default OS's - for selected with Tab and lightgray - for > hovered. This was considered in several of the bugs filed about this. I'm pretty sure UX considered it too and rejected it. I can't think of any other UI that has this, nor would it be exposable through e.g. accessibility APIs, so I don't really think this is a good idea myself, either. > While "status" should only display > "Search in <engine selected with Tab, if it was pressed. If it wasn't, then > - hovered engine>" That's what the current patch here does. It was specified in bug 1142334, as was the answer to your first question.

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to arni2033 from comment #3) > > (In reply to Florian Quèze [:florian] [:flo] from comment #2) > > > This patch lets us search with a non-default engine that isn't highlighted. > > If I select 1st alternate engine with Tab, then hover the 2nd alternate > > engine with mouse - then: > > 1) What should be displayed in "suggestions status"? (Search in <engine > > name>, but what engine?) > > The keyboard-selected button's engine name. The patch shows the name of the mouse-hovered engine, and I think that's the correct behavior. > > Imo, this could be fixed by 2 > > types of highlight: default OS's - for selected with Tab and lightgray - for > > hovered. > > This was considered in several of the bugs filed about this. I'm pretty sure > UX considered it too and rejected it. There used to be mockups with 2 different highlight colors. I don't remember this ever reaching a state that wasn't even more confusing than what we have now.

Comment on attachment 8688960 [details] MozReview Request: Bug 1222971 - don't let mouseover/mouseout remove the selectedButton that was set, r?florian https://reviewboard.mozilla.org/r/25473/#review22983 Looks reasonable and seems to work well, r=me. ::: browser/components/search/content/search.xml:1386 (Diff revision 2) > - if (target.localName != "button") > + if (target.localName != "button") { I'm not excited by this coding-style-only change, especially when the surrounding code (eg. 3 lines later, or beginning of the click event handler) doesn't follow this style, but it's not really a problem either.

Comment on attachment 8688960 [details] MozReview Request: Bug 1222971 - don't let mouseover/mouseout remove the selectedButton that was set, r?florian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25473/diff/2-3/

https://reviewboard.mozilla.org/r/25473/#review22991 I'm fine with this version too. ::: browser/components/search/content/search.xml:1397 (Diff revisions 2 - 3) > - // we have no idea if the mouse left us for greener pastures, > + // check if we're moving to a different button or not: I would prefer 'a different one-off' (more specific than 'a different button'). ::: browser/components/search/content/search.xml:1398 (Diff revisions 2 - 3) > - // I mean, other buttons, or not. Just set it to the selected > + if (!event.relatedTarget || This can be merged with the above if.

(In reply to :Gijs Kruitbosch from comment #6) > It was. Maybe I misunderstand what you're asking? Thanks for explanation. If my comments look strange - that's likely because I don't fully understand what's happening. I asked all that because: 1) original report doesn't imply those questions 2) I don't have enough time to read the diffs 3) bug 1139655 has been verified, but product behavior was still incorrect (I thought smth went wrong)

https://hg.mozilla.org/mozilla-central/rev/9d449237638a https://hg.mozilla.org/mozilla-central/rev/5b8f767ea1f8

I have reproduced this bug with Firefox nightly 45.0a1(build id:20151108030417)on windows 7(64 bit) I have verified as fixed this bug with Firefox aurora 45.0a2(build id:20160114004007) Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 [testday-20160115]