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

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Search
P4
normal
Rank:
45
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: Alexander Ploner, Assigned: adw)

Tracking

({regression})

37 Branch
Firefox 53
regression
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox34 wontfix, firefox35 wontfix, firefox36- wontfix, firefox37- wontfix, firefox38 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 verified)

Details

(Whiteboard: [ui][fxsearch], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments, 6 obsolete attachments)

393.52 KB, image/png
Details
182.56 KB, image/png
Details
389.06 KB, image/png
Details
351.10 KB, image/png
Details
999 bytes, patch
Gijs
: review+
Details | Diff | Splinter Review
299.49 KB, image/png
phlsa
: feedback+
Details
79.44 KB, image/png
phlsa
: feedback+
Details
8.77 KB, patch
florian
: feedback+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
florian
: review+
Details | Review
1.03 KB, patch
Details | Diff | Splinter Review
27.33 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8539412 [details]
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

3 years ago
(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

3 years ago
(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
(Reporter)

Comment 3

3 years ago
(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.
(Reporter)

Updated

3 years ago
Attachment #8539412 - Attachment description: Broken search UI → Broken search UI (Mac OS X 10.10.1)
(Reporter)

Comment 4

3 years ago
Created attachment 8539516 [details]
Cut off search UI (Ubuntu 14.04)
(Reporter)

Comment 5

3 years ago
Created attachment 8539517 [details]
Cut off search UI (Windows 8.1)

Comment 6

3 years ago
Created attachment 8539573 [details]
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

3 years ago
[Tracking Requested - why for this release]: UI broken
Blocks: 1088660
Status: UNCONFIRMED → NEW
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
Ever confirmed: true
Keywords: regression
Interesting test case, thanks! Realistically the earliest this could be fixed is for Firefox 36 (and more likely 37 or 38).
status-firefox34: affected → wontfix
status-firefox35: affected → wontfix
tracking-firefox35: ? → ---

Comment 9

3 years ago
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?
(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.
Not critical enough to block the release. Not tracking.
tracking-firefox36: ? → -
tracking-firefox37: ? → -
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."?
Flags: needinfo?(philipp)
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.
Points: --- → 1
Flags: qe-verify+
Flags: needinfo?(philipp)
Flags: firefox-backlog+
Oops, didn't mean to clear the needinfo.
Points: 1 → 3
Flags: needinfo?(philipp)
Sounds good!
I assume that the "Add search engine" line would have the same appearance (style and height) as the current "Add XYZ" items.
Flags: needinfo?(philipp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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).
Flags: needinfo?(gavin.sharp)
Also, if we are going for a solution that requires a new string, it's wontfix for both 36 and 37.
"Add search engine" seems fine and is probably easiest. We should get the string into 38.
Flags: needinfo?(gavin.sharp)
status-firefox36: affected → wontfix
status-firefox37: affected → wontfix
status-firefox38: --- → affected
Created attachment 8567096 [details] [diff] [review]
WIP

Non-working WIP; just here to illustrate how the new string will be used.
Assignee: nobody → florian
Created attachment 8567097 [details] [diff] [review]
Strings for prelanding in 38
Attachment #8567097 - Flags: review?(gijskruitbosch+bugs)

Comment 21

3 years ago
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.
Attachment #8567097 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/8eff2d358321
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8eff2d358321
See Also: → bug 1138058

Updated

3 years ago
Duplicate of this bug: 1140871
Duplicate of this bug: 1138058
See Also: bug 1138058

Comment 26

2 years ago
works for me

Comment 27

2 years ago
works for me . nightly 30.0a1(2015-03-20)
Depends on: 1149398
No longer depends on: 1149398
Priority: -- → P4
Whiteboard: [fxsearch][searchui]

Updated

2 years ago
Rank: 45
Whiteboard: [fxsearch][searchui] → [ui][fxsearch]
Created attachment 8611528 [details] [diff] [review]
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.
Assignee: florian → nhnt11
Attachment #8567096 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8611528 - Flags: review?(florian)
Created attachment 8611530 [details]
Screenshot when there are 7 items
Created attachment 8611531 [details]
Screenshot when there enough items to overflow the menu
Created attachment 8611537 [details] [diff] [review]
Patch v1.1

Removes the border on the first addengine-item when it's in the menu.
Attachment #8611528 - Attachment is obsolete: true
Attachment #8611528 - Flags: review?(florian)
Attachment #8611537 - Flags: review?(florian)
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.
Created attachment 8611543 [details] [diff] [review]
Patch v1.2
Attachment #8611543 - Flags: review?(florian)
Patch v1.2 ports the CSS part to the Windows and Linux versions of searchbar.css, but they are still untested.
Attachment #8611531 - Flags: feedback?(philipp)
Attachment #8611530 - Flags: feedback?(philipp)
Created attachment 8611584 [details] [diff] [review]
Patch v2

Adds the "+" badge to the menu button's icon, and in the process, fixes the icon for 2dppx.
Attachment #8611537 - Attachment is obsolete: true
Attachment #8611543 - Attachment is obsolete: true
Attachment #8611537 - Flags: review?(florian)
Attachment #8611543 - Flags: review?(florian)
Attachment #8611584 - Flags: review?(florian)
Created attachment 8611585 [details]
Screenshot when there are 7 items, patch v2 applied

With the "+" badge, and with that extra border removed.
Attachment #8611530 - Attachment is obsolete: true
Attachment #8611530 - Flags: feedback?(philipp)
Attachment #8611585 - Flags: feedback?(philipp)
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).
Attachment #8611584 - Flags: review?(florian) → feedback+
Created attachment 8612452 [details] [diff] [review]
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.
Attachment #8611584 - Attachment is obsolete: true
Attachment #8612452 - Flags: feedback?(florian)
I meant stopPropagation() in the previous comment, not preventDefault().
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.
Attachment #8612452 - Flags: feedback?(florian) → feedback+
Attachment #8611585 - Flags: feedback?(philipp) → feedback+
Attachment #8611531 - Flags: feedback?(philipp) → feedback+
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?
(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.
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.
Flags: needinfo?(florian)

Comment 44

2 years ago
(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?
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

2 years ago
(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.
(removing needinfo until comment 46 is answered)
Flags: needinfo?(florian)

Updated

2 years ago
Blocks: 1214284

Updated

2 years ago
See Also: → bug 1251977
Duplicate of this bug: 1251977
Depends on: 1251977
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.)
Flags: needinfo?(nhnt11)

Updated

a year ago
Blocks: 1251977
No longer depends on: 1251977
(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.
Flags: needinfo?(nhnt11)
Nihanth can you take this on? It does look like a current issue and bug 1251977 depends on it.
Flags: needinfo?(nhnt11)
I won't be able to get to this until Monday. Leaving the needinfo? as a reminder to myself.

Comment 53

11 months ago
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?
Flags: needinfo?(dolske)

Updated

11 months ago
Flags: needinfo?(dolske) → needinfo?(past)
Drew, do you have any spare cycles to finish this patch?
Flags: needinfo?(past) → needinfo?(adw)
(Assignee)

Comment 55

11 months ago
Sure, I can take a look.
Assignee: nhnt11 → adw
Flags: needinfo?(nhnt11)
Flags: needinfo?(adw)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

10 months ago
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.
(Assignee)

Updated

10 months ago

Comment 58

10 months ago
mozreview-review
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/search-engine-placeholder@2x.png");
>    }
>  }
>  
> +.addengine-item[type=menu] dropmarker {

Is there no reasonable way to avoid the descendant selector here?
Attachment #8801443 - Flags: review?(florian)
(Assignee)

Comment 59

10 months ago
Working on a test before I post an updated patch.
Comment hidden (mozreview-request)
(Assignee)

Comment 61

10 months ago
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

10 months ago
mozreview-review
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.
Attachment #8801443 - Flags: review?(florian)
Comment hidden (mozreview-request)

Comment 64

9 months ago
mozreview-review
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*
Attachment #8801443 - Flags: review?(florian)
(Assignee)

Comment 65

9 months ago
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.
Flags: needinfo?(florian)

Comment 66

9 months ago
mozreview-review
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.
Attachment #8801443 - Flags: review+
(Assignee)

Comment 67

9 months ago
Thanks Florian.
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Flags: needinfo?(florian)

Comment 69

9 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e92691272d3
New search UI breaks if too many open search providers are offered. r=florian
(Assignee)

Comment 70

9 months ago
Created attachment 8811404 [details] [diff] [review]
eslint follow-up patch

Comment 71

9 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/481290e80b9d
ESlint followup a=bustage DONTBUILD

Comment 72

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e92691272d3
https://hg.mozilla.org/mozilla-central/rev/481290e80b9d
I don't think this needs to remain open any more.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Is this worth an uplift to Aurora52/Beta51? If so, please nominate :)
status-firefox38: affected → wontfix
status-firefox50: --- → wontfix
status-firefox51: --- → fix-optional
status-firefox52: --- → fix-optional
status-firefox53: --- → fixed
Flags: needinfo?(adw)
Depends on: 1318790
(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.
(Assignee)

Comment 76

9 months ago
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.
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.
status-firefox53: fixed → verified
Flags: qe-verify+
(Assignee)

Updated

9 months ago
Depends on: 1319151
(Assignee)

Updated

9 months ago
No longer depends on: 1319151
(Assignee)

Comment 78

9 months ago
Created attachment 8813479 [details] [diff] [review]
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
Flags: needinfo?(adw)
Attachment #8813479 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 79

9 months ago
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.
(Assignee)

Updated

9 months ago
status-firefox51: fix-optional → wontfix
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...
(Assignee)

Comment 81

9 months ago
Yeah, that's a fair argument.  It would be OK (with me) not to take it, too.
Comment on attachment 8813479 [details] [diff] [review]
Aurora combined patch

let's keep this riding the trains, and ship with 53
Attachment #8813479 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
status-firefox52: fix-optional → wontfix

Updated

8 months ago
Depends on: 1326826
This appears to have introduced bug 1323525 on Linux.

Updated

8 months ago
Depends on: 1328104

Updated

8 months ago
Depends on: 1323525
Based on the tracking flags I am closing this issue as verified. Cheers!
Status: RESOLVED → VERIFIED

Comment 85

4 months ago
https://webcompat.com/issues/5612 is broken by this bug, not sure if there is any related bug?
(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.
You need to log in before you can comment on or make changes to this bug.