Open Bug 1332280 Opened 7 years ago Updated 2 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

People

(Reporter: jaws, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1332280 - Disable flipping of the select dropdown if the dropdown has a search field.
59 bytes, text/x-review-board-request
Details 
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.
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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.
 ```
Attachment #8828360 - Flags: review?(mconley) → review-
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
Flags: needinfo?(enndeakin)
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?
Flags: needinfo?(jaws)
I need to get Enn's feedback incorporated in to the patch. It doesn't work as-of-yet.
Flags: needinfo?(jaws)
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.
Attachment #8828360 - Flags: review?(mconley)
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.
Flags: needinfo?(enndeakin)
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 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?
Attachment #8828360 - Flags: review?(mconley)
This bug is wontfix, as bug 1332294 is the new route.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: jaws → nobody
Status: REOPENED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: