1260820 - Search engine drop down closes when I change search engines by pressing Up/Down

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160326030430
STR:
1. Open about:preferences#search
2. Expand search engine drop down
3. Press Down key

AR:  Search engine drop down collapses
ER:  Search engine drop down should stay visible

This is regression from bug 1122618. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=861971c670e1fe27c8bf43833831d7ce17b46f00&tochange=503ebfe13cd2f079829f4a4c8b7e860b4989403f

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression from 46, let's track this for 48/49 and find an owner for the bug.  
Wontfix for 46/47 as we are a couple of weeks away from the 47 release now. 

Florian, can you take a look here and help find an owner, since you were the reviewer on bug 1122618? Thanks!

Comment 2

[Florian Quèze [:florian]]

I can't reproduce on Mac. Someone else tested on a Linux nightly and couldn't reproduce either. This may be Windows specific.

Comment 3

[Florian Quèze [:florian]]

I can reproduce on Windows.

On Mac when the user moves through the list in the drop down using up/down it highlights the new row but only selects it after pressing enter. On Windows it changes the selection immediately, which causes this code https://hg.mozilla.org/mozilla-central/rev/503ebfe13cd2#l1.33 to run and destroy the drop down to rebuild it.

Comment 4

[Florian Quèze [:florian]]

Attachment: Fix

I'm going to test this patch on Windows before requesting review, but I'm confident it should fix the problem.

Comment 5

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77b96ecf5aa2

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8761155 [details] [diff] [review]
Fix

I tested the patch on Windows and it worked as expected.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8761155 [details] [diff] [review]
Fix

A reluctant r=me as a wallpaper fix, but why does scrolling through that list fire engine-current on Windows but not elsewhere? Can't we just make it not do that? If not, please include a comment before the if() explaining why we need to check this...

Comment 8

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #7)

> why does scrolling through that
> list fire engine-current on Windows but not elsewhere?Can't we just make it
> not do that? If not, please include a comment before the if() explaining why
> we need to check this...

I assumed it was an intended platform specific behavior. On Windows when going up/down in a menulist the highlighted row is selected immediately. On Mac when using up/down, there's a light highlight on the new row (similar to what we have for an hovered row).

I couldn't find code doing anything specific to Windows when looking at the menulist related code, but to be honest I didn't spend much time searching for it, because the patch doesn't seem bad to me: it's an optimization (we avoid rebuilding the whole list of engines) for the general case (ie. when the change has been made from the pref window, which should be the case most of the time).

Comment 9

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/6e0788aa97c74aa11b75654bfc82d4ecb34f502a
Bug 1260820 - Search engine drop down closes when I change search engines by pressing Up/Down, r=Gijs.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e0788aa97c7

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8761155 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1122618 in Firefox 46
[User impact if declined]: going through the dropdown of search engines in the preferences using the keyboard is painful on Windows.
[Describe test coverage new/current, TreeHerder]: landed on central, QA can verify.
[Risks and why]: low risk, the patch is simple, and self contained.
[String/UUID change made/needed]: none.

Comment 12

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Nightly 2016-06-03  on Windows 7 64bit.

Verified fixed on Windows 7 64bit using latest Nightly 50.0a1 (buildID: 20160614030210).

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8761155 [details] [diff] [review]
Fix

Review of attachment 8761155 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes the regression issue of search engine drop down issue and is verified. Take it in aurora & beta.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75033b504c4c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35a4e516308e

Comment 16

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit and Windows 10 64bit using latest Aurora 49.0a2 (buildID: 20160627004013) and Firefox 48 Beta 3 (buildID: 20160623122823).