1222971 - mouse over search engine changes search engine (too easy inadvertently NOT TO use different engine for searches)

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[arni2033]

Attachment: video 1 - too easy inadvertently NOT TO use different engine for searches.webm

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

Comment 1

[:Gijs (he/him)]

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

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

Comment 2

[Florian Quèze [: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.

Comment 3

[arni2033]

(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 4

[:Gijs (he/him)]

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/

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[:Gijs (he/him)]

(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.

Comment 7

[Florian Quèze [:florian]]

(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 8

[Florian Quèze [: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/#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 9

[:Gijs (he/him)]

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/

Comment 10

[Florian Quèze [:florian]]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9d449237638a

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5b8f767ea1f8

Comment 13

[arni2033]

(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)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Kazi Nuzhat Tasnem]

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]