1332280 - The search select dropdown should not flip while using the built-in search

Status: NEW

(Core :: Layout: Form Controls, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1309935 added support for searching within the search dropdown but it was disabled upon landing because the search dropdown currently flips while searching.

We should disable flipping of the popup while searching. Once this is done we should be able to enable the feature on Nightly.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I wasn't able to test the patch for this bug yet, so if you can while you review it that would be awesome. If not I can test it later.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

https://reviewboard.mozilla.org/r/105794/#review106782

I'm afraid this doesn't build:

```
 0:30.20 In file included from /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin14.5.0/layout/xul/Unified_cpp_layout_xul1.cpp:56:
 0:30.20 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:77:7: error: use of undeclared identifier 'flip'; did you mean 'nsGkAtoms::flip'?
 0:30.20   if (flip.EqualsLiteral("none")) {
 0:30.20       ^~~~
 0:30.20       nsGkAtoms::flip
 0:30.20 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtomList.h:392:9: note: 'nsGkAtoms::flip' declared here
 0:30.20 GK_ATOM(flip, "flip")
 0:30.20         ^
 0:30.20 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtoms.h:30:48: note: expanded from macro 'GK_ATOM'
 0:30.20 #define GK_ATOM(_name, _value) static nsIAtom* _name;
 0:30.21                                                ^
 0:30.21 In file included from /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin14.5.0/layout/xul/Unified_cpp_layout_xul1.cpp:56:
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:77:11: error: member reference type 'nsIAtom *' is a pointer; maybe you meant to use '->'?
 0:30.21   if (flip.EqualsLiteral("none")) {
 0:30.21       ~~~~^
 0:30.21           ->
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:77:12: error: no member named 'EqualsLiteral' in 'nsIAtom'
 0:30.21   if (flip.EqualsLiteral("none")) {
 0:30.21       ~~~~ ^
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:80:7: error: use of undeclared identifier 'flip'; did you mean 'nsGkAtoms::flip'?
 0:30.21   if (flip.EqualsLiteral("both")) {
 0:30.21       ^~~~
 0:30.21       nsGkAtoms::flip
 0:30.21 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtomList.h:392:9: note: 'nsGkAtoms::flip' declared here
 0:30.21 GK_ATOM(flip, "flip")
 0:30.21         ^
 0:30.21 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtoms.h:30:48: note: expanded from macro 'GK_ATOM'
 0:30.21 #define GK_ATOM(_name, _value) static nsIAtom* _name;
 0:30.21                                                ^
 0:30.21 In file included from /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin14.5.0/layout/xul/Unified_cpp_layout_xul1.cpp:56:
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:80:11: error: member reference type 'nsIAtom *' is a pointer; maybe you meant to use '->'?
 0:30.21   if (flip.EqualsLiteral("both")) {
 0:30.21       ~~~~^
 0:30.21           ->
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:80:12: error: no member named 'EqualsLiteral' in 'nsIAtom'
 0:30.21   if (flip.EqualsLiteral("both")) {
 0:30.21       ~~~~ ^
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:83:7: error: use of undeclared identifier 'flip'; did you mean 'nsGkAtoms::flip'?
 0:30.21   if (flip.EqualsLiteral("slide")) {
 0:30.21       ^~~~
 0:30.21       nsGkAtoms::flip
 0:30.21 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtomList.h:392:9: note: 'nsGkAtoms::flip' declared here
 0:30.21 GK_ATOM(flip, "flip")
 0:30.21         ^
 0:30.21 /Users/mikeconley/Projects/mozilla-central/dom/base/nsGkAtoms.h:30:48: note: expanded from macro 'GK_ATOM'
 0:30.21 #define GK_ATOM(_name, _value) static nsIAtom* _name;
 0:30.21                                                ^
 0:30.21 In file included from /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin14.5.0/layout/xul/Unified_cpp_layout_xul1.cpp:56:
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:83:11: error: member reference type 'nsIAtom *' is a pointer; maybe you meant to use '->'?
 0:30.21   if (flip.EqualsLiteral("slide")) {
 0:30.21       ~~~~^
 0:30.21           ->
 0:30.21 /Users/mikeconley/Projects/mozilla-central/layout/xul/nsMenuPopupFrame.cpp:83:12: error: no member named 'EqualsLiteral' in 'nsIAtom'
 0:30.21   if (flip.EqualsLiteral("slide")) {
 0:30.21       ~~~~ ^
 0:30.21 9 errors generated.
 ```

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Hey Neil, with the attached patch the popup still flips once its size allows it to, though I am trying to disable this behavior while typing in the search box.

In nsMenuPopupFrame.cpp, should I be changing mVFlip instead of mFlip? 

STR:
In about:config, set dom.forms.selectSearch=true
Open http://webdev.cse.msu.edu/~beachjar/capstone/test.html
Scroll the page so that the Long Search (60 items) is located towards the bottom of the screen
Click on the <select> button
The popup should open above the <select> button
Click in the search field, and type "op0"
Items in the popup that don't match "op0" should hide

ER:
The popup should remain above the <select> anchor

AR:
The popup flips vertically to be anchored below the <select> button

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey jaws - were you hoping to get this landed even without Enn's feedback in comment 6? Or were you hoping to work that feedback into this patch?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

I need to get Enn's feedback incorporated in to the patch. It doesn't work as-of-yet.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

https://reviewboard.mozilla.org/r/105794/#review107582

Okay, in that case, clearing review flag since this patch isn't done yet.

Comment 10

[Neil Deakin]

So flip="none" will disable flipping and cause the popup to layout using only the alignment values, 'align_start' in this case, so the popup will always appear below.

But perhaps what should happen is that disabling flipping while open should leave it where is is. That would invole using the current value of mHFlip/mVFlip is FlipOrResize instead of comparing values.

A workaround might be to set the popup's position attribute to the value of alignmentPosition when disabling the flip, and clearing on blur.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

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

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

The attached patch does as Bram suggested on bug 1332301, though it is not without its own caveats as disabling flipping means that menus opened near the edge of the screen may be too small to be useful.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8828360 [details]
Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.

https://reviewboard.mozilla.org/r/105794/#review111694

::: toolkit/modules/SelectParentHelper.jsm:50
(Diff revision 4)
>  
>      // Set the maximum height to show exactly MAX_ROWS items.
>      let menupopup = menulist.menupopup;
>      let firstItem = menupopup.firstChild;
> -    while (firstItem && firstItem.hidden) {
> +    while (firstItem && firstItem.hidden &&
> +           firstItem.localName != "textbox") {

Maybe add a comment that this textbox we're checking for is the searchbox.

::: toolkit/modules/SelectParentHelper.jsm:64
(Diff revision 4)
> +    if (menupopup.firstChild &&
> +        menupopup.firstChild.localName == "textbox") {

I'm not super jazzed about reaching into the DOM and determining we're searching just by the structure of the nodes. I wonder if an attribute on the menupopup makes more sense - populateChildren could set that.

What do you think?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

This bug is wontfix, as bug 1332294 is the new route.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

After further thought, I think we may still need to do this. Even after bug 1332294 is implemented the dropdown will still flip when its size is small enough to show below the button. If the menu opens on top of the button we should prevent it from flipping below the button once the size is smaller.