1113747 - New search UI breaks if too many open search providers are offered

Status: VERIFIED FIXED

(Firefox :: Search, defect, P4)

Description

[Alexander Ploner]

Attachment: Broken search UI (Mac OS X 10.10.1)

Steps to reproduce:
1. Open a website that offers many open search providers: http://de.pons.com/
2. Press the magnifier icon in the search input.

Expected result:
The open search providers should be displayed in a scrollable list.

Actual result:
The list is not scrollable and the UI is broken. You can see it in the screen shot.

Comment 1

[dilspi72-mozilla]

(In reply to Alexander Ploner from comment #0)
> Created attachment 8539412 [details]
> Broken search UI
...
> Actual result:
> The list is not scrollable and the UI is broken. You can see it in the
> screen shot.

here the list do not shown when doing that on the pons-page. Only drop-downs the right shadow show. On other pages (like wordpress-blogs, for e.g. https://dilspi.wordpress.com/logbook/ ) are able to show them on the list and are search-able after doing so.
So i speculate (without knowing) that the issue is on pons side.
(win7, nightly 37.0a1)

Comment 2

[dilspi72-mozilla]

(In reply to Alexander Ploner from comment #0)
...
> 1. Open a website that offers many open search providers: http://de.pons.com/
...
on other parts of ponds-page (like: http://de.pons.com/shop/tuerkisch/ ) the drop-down shows

Comment 3

[Alexander Ploner]

(In reply to dilspi72-mozilla from comment #1)
> On other pages (like wordpress-blogs, for e.g.
> https://dilspi.wordpress.com/logbook/ ) are able to show them on the list
> and are search-able after doing so.
> So i speculate (without knowing) that the issue is on pons side.

I had a look into the HTML code of that page. There are no problems with it.

The problem is that there are too many different search providers. That amount can't be handled by the new search UI until now. PONS offers a search provider for each possible translation direction on their site (e.g. German<->English, English<->Italian, Italian<->German, ...) but of course they are allowed to do so :).

The behavior is a bit different depending on the OS. I'll add screen shots for Windows 8 and Ubuntu 14.04. On Mac OS X it's hard to select a specific entry, because hovering an element with the mouse doesn't work properly (the blue hover effect jumps up and down if you move the mouse only a bit). On Windows and Ubuntu selecting an entry works but the entries are cut off at the end of the screen.

Comment 4

[Alexander Ploner]

Attachment: Cut off search UI (Ubuntu 14.04)

Comment 5

[Alexander Ploner]

Attachment: Cut off search UI (Windows 8.1)

Comment 6

[dilspi72-mozilla]

Attachment: win7sp1-64bit-Bug1113747_01.png

confirmed here \o/

Alexander, please take my sorry.
Haven't got it first, but now i can confirm this issue on FireFox nightly 37.0a1 with an english windows 7 64bit sp1.

The effect of this issue is here, that if there are too many to list the list refuse to show it's content and here on Windows only paint the right drop-down shadow.

Maybe its relevant, so i add the 
graphic-cards (Nvidia GTX970) 
driver-version (9.18.13.4475).

Comment 7

[Alice0775 White]

[Tracking Requested - why for this release]: UI broken

Comment 8

[Florian Quèze [:florian]]

Interesting test case, thanks! Realistically the earliest this could be fixed is for Firefox 36 (and more likely 37 or 38).

Comment 9

[Elbart]

Regression-window for the Win7-issue:
Last good revision: 035a951fc24a (2014-12-08)
First bad revision: f1f48ccb2d4e (2014-12-09)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=035a951fc24a&tochange=f1f48ccb2d4e

Last good revision: 2a61df4eaa2d
First bad revision: 24ba8274ed60
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a61df4eaa2d&tochange=24ba8274ed60

Though I cannot reproduce it reliably. In one session the dropdown is shown, after a restart of the browser it's not shown. I started each revision three times, and judged it good when the dropdown is shown in all three sessions.

PS: Is it a bug that the "Add XYZ" and "Change Search Settings"-buttons can't be reached by keyboard?

Comment 10

[Florian Quèze [:florian]]

(In reply to Elbart from comment #9)
> Regression-window for the Win7-issue

Could the Windows specific issue be investigated in a different bug while we keep this bug for the general case of the new search panel not showing lots of open search engines properly?

> PS: Is it a bug that the "Add XYZ" and "Change Search Settings"-buttons
> can't be reached by keyboard?

Yes. We have bug 1102038 on file for the settings button, I don't think we have a bug on file for the "Add <engine name>" list.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Not critical enough to block the release. Not tracking.

Comment 12

[Florian Quèze [:florian]]

Bug 1102511 is vaguely related (case where lots of engines are installed).

Philipp, do you like the suggestion in comment 0 "The open search providers should be displayed in a scrollable list."?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Just chatted with Madhava/Chad about this, we concluded that it would be best to collapse multiple engines into a single "Add N Providers" scrollable submenu if the number of offered provided is greater than e.g. 5.

I.e. if there are 3 engines, it would be the same as the current UI:
[Normal Popup]
Add Foo1
Add Foo2
Add Foo3

But if there are 6 engines, instead you see:
[Normal Popup]
Add search engine >

That when clicked results in:
[Normal Popup]
Add search engine > [ Foo1 ]
                    [ Foo2 ]
                    [ Foo3 ]
                    [ Foo4 ]
                    [ Foo5 ]
                    [ Foo6 ]

and that list is scrollable if it overflows the screen, like other menus.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Oops, didn't mean to clear the needinfo.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Sounds good!
I assume that the "Add search engine" line would have the same appearance (style and height) as the current "Add XYZ" items.

Comment 16

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #13)
> Just chatted with Madhava/Chad about this, we concluded that it would be
> best to collapse multiple engines into a single "Add N Providers" scrollable
> submenu

> That when clicked results in:
> [Normal Popup]
> Add search engine > [ Foo1 ]
>                     [ Foo2 ]

I'm not sure if you are suggesting the label of the submenu to be "Add N Providers" (localizable plural string) or "Add search engine" (always the same string).

Comment 17

[Florian Quèze [:florian]]

Also, if we are going for a solution that requires a new string, it's wontfix for both 36 and 37.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

"Add search engine" seems fine and is probably easiest. We should get the string into 38.

Comment 19

[Florian Quèze [:florian]]

Attachment: WIP

Non-working WIP; just here to illustrate how the new string will be used.

Comment 20

[Florian Quèze [:florian]]

Attachment: Strings for prelanding in 38

Comment 21

[:Gijs (he/him)]

Comment on attachment 8567097 [details] [diff] [review]
Strings for prelanding in 38

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

::: browser/locales/en-US/chrome/browser/search.properties
@@ +23,5 @@
>  cmd_showSuggestions=Show Suggestions
>  cmd_showSuggestions_accesskey=S
>  
> +# LOCALIZATION NOTE (cmd_addFoundEngine): %S is replaced by the name of
> +# a search engine offered by a web page. This is displayed as menuitems

Nit: "x is <verb>ed as y" where x is singular and y is plural does not make grammatical sense.

Comment 22

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/8eff2d358321

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/8eff2d358321

Comment 26

[happyboy.bn]

works for me

Comment 27

[happyboy.bn]

works for me . nightly 30.0a1(2015-03-20)

Comment 28

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

This puts everything in a new popup menu as suggested in the previous comments.
Note that adding a new popup menu means that the popup* handlers pick up events from that too, so I added a check to ensure the popup* handlers only pick up events on the main panel.

Let me know what you think is the best way to possibly get rid of the duplication when creating the button.

Comment 29

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot when there are 7 items

Comment 30

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot when there enough items to overflow the menu

Comment 31

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v1.1

Removes the border on the first addengine-item when it's in the menu.

Comment 32

[(inactive) Nihanth Subramanya [:nhnt11]]

By the way:
a) I don't know why mousedown events need to be prevented to receive click events on Linux. I just made it prevent the event only for one-off items, I think that's fine.
b) I haven't done the CSS for Windows/Linux yet. I'll fire a try build so that I can try it out in my VMs.

Comment 33

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v1.2

Comment 34

[(inactive) Nihanth Subramanya [:nhnt11]]

Patch v1.2 ports the CSS part to the Windows and Linux versions of searchbar.css, but they are still untested.

Comment 35

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Adds the "+" badge to the menu button's icon, and in the process, fixes the icon for 2dppx.

Comment 36

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot when there are 7 items, patch v2 applied

With the "+" badge, and with that extra border removed.

Comment 37

[Florian Quèze [:florian]]

Comment on attachment 8611584 [details] [diff] [review]
Patch v2

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

Thanks for looking at this. The patch is going in the right direction and the code looks reasonable, but I've found a few issues when testing it locally:
- I expect the "Add search engine" submenu to open on hover, like other menus with submenus. Maybe you would get this behavior for free if you used a 'menuitem' instead of a 'button'?
- After opening and closing the submenu, the suggestion tree doesn't get resized correctly anymore (I haven't found exact steps to reproduce, it seemed to happen at least half the time during my testing of the patch), and the clearing the content of the search input box no longer closes the search panel (happens all the time).
- the > icon used here doesn't match other menus with submenus (at least on Mac).

Comment 38

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

Turns out that the autocomplete binding was receiving popup* events from the menupopup, and that was breaking stuff. This adds a preventDefault() to all those menupopup events, which fixes the issues.

This also updates the arrow icon to be identical to the icon on submenus in context menus.

I tried making the button a menu instead, so that we get open-on-hover for free, but for some reason it doesn't work at all - hovering on the menu doesn't even highlight the item (clicking it opens the popup though). I'll need to investigate further, but I thought I'd get some feedback on the rest of it in the meantime.

I've got try builds for Linux and Windows ready, I'll be testing them out next.

Comment 39

[(inactive) Nihanth Subramanya [:nhnt11]]

I meant stopPropagation() in the previous comment, not preventDefault().

Comment 40

[Florian Quèze [:florian]]

Comment on attachment 8612452 [details] [diff] [review]
Patch v3

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

::: browser/base/content/urlbarBindings.xml
@@ +1248,5 @@
> +            // menupopups, causing it to break after the menupopup has been
> +            // shown - so we need to prevent these events from propagating.
> +            let listener = function(aEvent) {
> +              aEvent.stopPropagation();
> +            }

nit: missing ';'. This could be an arrow function btw.

Comment 41

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Both screen shots look good!

Nit: Can we have the arrow inverted when the »Add search engine« item is hovered/selected?

Also, I forget and I can't find it in the thread right now: what was the number of engines where this starts to trigger?

Comment 42

[Florian Quèze [:florian]]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #41)

> Also, I forget and I can't find it in the thread right now: what was the
> number of engines where this starts to trigger?

See comment 13.

Comment 43

[(inactive) Nihanth Subramanya [:nhnt11]]

After many attempts to get it to work using a "menu" element instead of a button with type="menu", I feel like it would be unproductive to keep trying to get that to work. Would you be ok with:
a) Having it as it is without a hover interaction, or
b) Manually setting a timer to open the popup on mouseover?

The difficulty with using a "menu" item is that for some reason nothing happens when it's hovered over, but you can click it to open the popup. i.e. it behaves the same as a button. I expected to get the hover behavior "for free" with that but I guess not. I looked at the bookmarks menu code, and couldn't find anything obvious that I was missing.

Comment 44

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #43)
> After many attempts to get it to work using a "menu" element instead of a
> button with type="menu", I feel like it would be unproductive to keep trying
> to get that to work. Would you be ok with:
> a) Having it as it is without a hover interaction, or
> b) Manually setting a timer to open the popup on mouseover?
> 
> The difficulty with using a "menu" item is that for some reason nothing
> happens when it's hovered over, but you can click it to open the popup. i.e.
> it behaves the same as a button. I expected to get the hover behavior "for
> free" with that but I guess not. I looked at the bookmarks menu code, and
> couldn't find anything obvious that I was missing.

Can you attach a WIP patch?

Comment 45

[(inactive) Nihanth Subramanya [:nhnt11]]

Gijs, attachment 8612452 [details] [diff] [review] is pretty much the same as what I have locally. I basically changed the XUL "button" to a "menu" element instead, and tried some other stuff to prevent the mouse/popup events from propagating to the autocomplete binding (I have reason to believe it swallows popup events from any children of the panel, both in this bug and bug 1110771). Nothing I tried worked so I reverted all those changes.

Comment 46

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #45)
> Gijs, attachment 8612452 [details] [diff] [review] is pretty much the same
> as what I have locally. I basically changed the XUL "button" to a "menu"
> element instead, and tried some other stuff to prevent the mouse/popup
> events from propagating to the autocomplete binding (I have reason to
> believe it swallows popup events from any children of the panel, both in
> this bug and bug 1110771). Nothing I tried worked so I reverted all those
> changes.

... right, I'm asking for a patch with the changes. It seems to me that this should be a "menuitem" element, not a "menu" element, but it's hard to tell exactly what should happen without seeing what you're doing.

Comment 47

[Florian Quèze [:florian]]

(removing needinfo until comment 46 is answered)

Comment 49

[Justin Dolske [:Dolske]]

Nihanth: do we know if this patch is going to help with the CPU-usage issue in bug 1251977? (Previously marked as a dupe of this one, and then de-duped.)

Comment 50

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Justin Dolske [:Dolske] from comment #49)
> Nihanth: do we know if this patch is going to help with the CPU-usage issue
> in bug 1251977? (Previously marked as a dupe of this one, and then de-duped.)

I don't have enough context to answer this at the moment (I'm on vacation, forgot to update my BMO real name). I can take a look when I find time, but it would probably be prudent to get someone else to look at this.

Comment 51

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

Nihanth can you take this on? It does look like a current issue and bug 1251977 depends on it.

Comment 52

[(inactive) Nihanth Subramanya [:nhnt11]]

I won't be able to get to this until Monday. Leaving the needinfo? as a reminder to myself.

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dolske, this came up while reviewing bug 1251977. Do we have a fix in the works for this that could fix the other bug and uplifted to Fx50?

Comment 54

[Panos Astithas (he/him) [:past] (please ni?)]

Drew, do you have any spare cycles to finish this patch?

Comment 55

[Drew Willcoxon :adw]

Sure, I can take a look.

Comment 56

[Drew Willcoxon :adw]

Attachment: Bug 1113747 - New search UI breaks if too many open search providers are offered.

Based on an earlier patch by Nihanth Subramanya <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/86196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86196/

Comment 57

[Drew Willcoxon :adw]

Some notes:

* I tried making the button a menuitem instead of a button[type=menu], but that didn't look or behave properly, as I think Nihanth had found.  So this does use a a button[type=menu], same as his patch.

* Because of the button[type=menu], I had to handle opening the popup myself -- didn't get it for free.  So I use a small timeout to emulate how real menu popups open and close after a small delay when moused over.  We do that in other places.  The one I'm familiar with is Places UI for bookmark folders.  You can really tell the difference with vs. without the delay.

* <button>s in a menupopup don't get keyboard handling for free apparently.  In fact I couldn't figure out how to get them or the popup to respond to keypresses at all.  I tried using menuitems instead, and that works well.

* This uses -moz-appearance: menuarrow to get the real menu arrow look, as Nihanth's patch does.  Unfortunately the color is hardcoded deep down in Gecko, so it's not possible, from what I could find, to change its color when the button is selected/highlighted.  For that reason, I chose to use the settings button highlight color for the menu button.  It's light gray, so the black arrow looks good in both the highlighted and unhighlighted states.  Except on Linux, where the settings button uses the Highlight/HighlighText colors.

Comment 58

[Florian Quèze [:florian]]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

https://reviewboard.mozilla.org/r/86196/#review87348

The comments below are only details so I would r+, but I haven't been able to actually try the patch due to bitrot in browser/components/search/content/search.xml (sorry for the delay in getting to this review :-( ). So for now it would only be f+, which I can't set in reviewboard.

::: browser/components/search/content/search.xml:1381
(Diff revision 1)
> -            document.getAnonymousElementByAttribute(this, "anonid", "add-engines");
> -          while (addEngineList.firstChild)
> -            addEngineList.firstChild.remove();
>  
>            const kXULNS =
>              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Should we move this const down, closer to where it's actually used?

::: browser/components/search/content/search.xml:1436
(Diff revision 1)
>  
>            // Ensure we can refer to the settings button by ID:
>            let settingsEl = document.getAnonymousElementByAttribute(this, "anonid", "search-settings");
>            settingsEl.id = this.id + "-anon-search-settings";
>  
>            let dummyItems = enginesPerRow - (oneOffCount % enginesPerRow || enginesPerRow);

eg. here (for kXULNS)

::: browser/components/search/content/search.xml:1526
(Diff revision 1)
> +
> +        if (this.compact || !gBrowser.selectedBrowser.engines) {
> +          return;
> +        }
> +
> +        let engines = gBrowser.selectedBrowser.engines;

nit: I would move the kXULNS constant definition before this line; once we are sure it's actually going to be used.

::: browser/components/search/content/search.xml:1576
(Diff revision 1)
> +        // engines, the list is the add-engines vbox.  Otherwise it's the
> +        // menupopup created earlier.  In the latter case, create menuitem
> +        // elements instead of buttons, because buttons don't get keyboard
> +        // handling for free inside menupopups.
> +        for (let engine of engines) {
> +          let eltType = tooManyEngines ? "menuitem" : "button";

nit: this line can move outside of the loop

::: browser/themes/osx/searchbar.css:226
(Diff revision 1)
>    color: HighlightText;
>  }
>  
> +.addengine-item[type=menu][selected],
> +.addengine-item[type=menu][open] {
> +  background-color: #d3d3d3;

Is it ok to hardcode this color here? #d3d3d3 doesn't seem to be used in any other osx css file.

::: browser/themes/osx/searchbar.css:262
(Diff revision 1)
>    .addengine-item:not([image]) {
>      list-style-image: url("chrome://browser/skin/[e-mail]");
>    }
>  }
>  
> +.addengine-item[type=menu] dropmarker {

Is there no reasonable way to avoid the descendant selector here?

Comment 59

[Drew Willcoxon :adw]

Working on a test before I post an updated patch.

Comment 60

[Drew Willcoxon :adw]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86196/diff/1-2/

Comment 61

[Drew Willcoxon :adw]

Florian, the interdiff is going to be useless since the latest revision is based on a different tree.  Sorry about that.  This revision also adds a test.

(In reply to Florian Quèze [:florian] [:flo] from comment #58)

> Should we move this const down, closer to where it's actually used?

Done

> nit: I would move the kXULNS constant definition before this line; once we
> are sure it's actually going to be used.

Done

> nit: this line can move outside of the loop

Done

> Is it ok to hardcode this color here? #d3d3d3 doesn't seem to be used in any
> other osx css file.

Ah, this must have changed since I started the patch.  It's now var(--arrowpanel-dimmed-further), so I changed it to that.

> Is there no reasonable way to avoid the descendant selector here?

Done

Comment 62

[Florian Quèze [:florian]]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

https://reviewboard.mozilla.org/r/86196/#review89622

The code looks good, and thanks for adding a test :-).

One thing I noticed while testing is that the right and left arrow keys (to open and close a submenu) aren't handled (this is what I use instead of enter when navigating through menus with the keyboard).

Also, the test doesn't cover keyboard navigation.

Comment 63

[Drew Willcoxon :adw]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86196/diff/2-3/

Comment 64

[Florian Quèze [:florian]]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

https://reviewboard.mozilla.org/r/86196/#review93074

Thanks for the updated patch. The left arrow (to close the submenu) is still not handled.

::: browser/components/search/test/browser_tooManyEnginesOffered.js:66
(Diff revisions 2 - 3)
> +  // Press the Right arrow key.  The submenu should open.
> +  promise = promiseEvent(buttonPopup, "popupshown");
> +  EventUtils.synthesizeKey("VK_RIGHT", {});
> +  yield promise;
> +
> +  Assert.ok(menuButton.open, "Submenu should be open");

Here, add a check for VK_LEFT (which currently doesn't work), and then use ENTER to reopen.

::: browser/components/search/test/browser_tooManyEnginesOffered.js:68
(Diff revisions 2 - 3)
> +  EventUtils.synthesizeKey("VK_RIGHT", {});
> +  yield promise;
> +
> +  Assert.ok(menuButton.open, "Submenu should be open");
> +
> +  // Pres the Esc key.  The submenu should close.

typo: Pres*s*

Comment 65

[Drew Willcoxon :adw]

Handling the left arrow key isn't straightforward because the popup eats it when it's open.  You can set ignorekeys=true to make the popup not eat it, but then you lose the up/down arrow key behavior that selects adjacent menu items.  You can set ignorekeys=handled but that doesn't do any good because as I say the popup handles -- consumes -- the left arrow key.

(For reference, search for DOM_VK_LEFT here: https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp)

I tried setting ignorekeys=true and then forwarding non-left keys to the popup by calling popup.handleEvent, but handleEvent isn't defined on it.  I tried QI'ing it and its popup box object to nsIDOMEventListener first, but they don't implement that interface.

There's no scriptable method (on PopupBoxObject.webidl) that looks like it would help here.  Maybe we could add one.

But I don't think it's worth spending any more time and energy on this.  This bug has got to be a small corner case, and keyboard handling within the popup is probably a non-majority case within that, and pressing left to close the popup once it's open is a sub-case within that.  You can press esc to close the popup.

Comment 66

[Florian Quèze [:florian]]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

https://reviewboard.mozilla.org/r/86196/#review93454

Thanks for the explanation. I agree it's not worth spending more time and energy on this corner case, my previous review comment was mostly because from looking at the patch handling the right key without the left one looked like an oversight. You may want to add a comment near the code handling the right key to explain why the left one is unfortunately not handled. There's still the 'Pres' -> 'Press' typo to fix.

Comment 67

[Drew Willcoxon :adw]

Thanks Florian.

Comment 68

[Drew Willcoxon :adw]

Comment on attachment 8801443 [details]
Bug 1113747 - New search UI breaks if too many open search providers are offered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86196/diff/3-4/

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e92691272d3
New search UI breaks if too many open search providers are offered. r=florian

Comment 70

[Drew Willcoxon :adw]

Attachment: eslint follow-up patch

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/481290e80b9d
ESlint followup a=bustage DONTBUILD

Comment 72

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

https://hg.mozilla.org/mozilla-central/rev/3e92691272d3
https://hg.mozilla.org/mozilla-central/rev/481290e80b9d

Comment 73

[Panos Astithas (he/him) [:past] (please ni?)]

I don't think this needs to remain open any more.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Is this worth an uplift to Aurora52/Beta51? If so, please nominate :)

Comment 75

[Eric Rahm [:erahm]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #74)
> Is this worth an uplift to Aurora52/Beta51? If so, please nominate :)

Maybe not yet, it's the source of a large amount of logspam.

Comment 76

[Drew Willcoxon :adw]

I think it would probably be OK considering that it would have a lot of time to bake since the merge just happened.  At least for Aurora.  I'd have to think more about whether it's OK for Beta.  Probably -- but the patch did make some non-trivial changes to the searchbar, and maybe there's some problematic corner case we haven't found yet.

Once bug 1318790 lands I'll nominate for at least Aurora.  I'll leave the needinfo until then as a reminder.

Comment 77

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 20.0a1 on Windows 10 x64.
I can confirm this issue is fixed on Fx 53.0a1 (20161120030205). 
I verified on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04.

Comment 78

[Drew Willcoxon :adw]

Attachment: Aurora combined patch

This patch includes both commits that landed in this bug.

Bug 1319151 is a follow-up to this bug that must also be accepted on Aurora, if this bug is accepted.

Approval Request Comment
[Feature/regressing bug #]: fx34-searchui (bug 1088660)
[User impact if declined]: Web pages that offer many search plugins can break Firefox's searchbar popup
[Describe test coverage new/current, TreeHerder]: New automated test, part of this patch
[Risks and why]: Fairly low risk: the changes are non-trivial and could potentially break some aspects of the searchbar if there are new bugs, but this has an automated test and a lot of time to bake on Aurora and then Beta
[String/UUID change made/needed]: None

Comment 79

[Drew Willcoxon :adw]

It's probably better to not land this on Beta.  It would probably be OK, but I don't think there's a big reason to risk regressing it.

Comment 80

[Julien Cristau [:jcristau]]

I'm not convinced we should uplift this to 52.  This bug is several years old, the fix is non-trivial, we can probably live with it for another cycle?  Happy to hear arguments for taking this anyway...

Comment 81

[Drew Willcoxon :adw]

Yeah, that's a fair argument.  It would be OK (with me) not to take it, too.

Comment 82

[Julien Cristau [:jcristau]]

Comment on attachment 8813479 [details] [diff] [review]
Aurora combined patch

let's keep this riding the trains, and ship with 53

Comment 83

[Tanner Filip [:tanner]]

This appears to have introduced bug 1323525 on Linux.

Comment 84

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Based on the tracking flags I am closing this issue as verified. Cheers!

Comment 85

[Eric Tsai (no more review request accepted)]

https://webcompat.com/issues/5612 is broken by this bug, not sure if there is any related bug?

Comment 86

[Mark Banner (:standard8)]

(In reply to Eric Tsai from comment #85)
> https://webcompat.com/issues/5612 is broken by this bug, not sure if there
> is any related bug?

Note: this has been filed as bug 1356131.