Closed Bug 1309935 Opened 8 years ago Closed 7 years ago

Add ability to find within select dropdown when over 40 elements

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
relnote-firefox --- 53+
firefox53 --- fixed

People

(Reporter: jaws, Assigned: tmaklebust, Mentored)

References

(Depends on 3 open bugs, Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 7 obsolete files)

Bug 1309935 - Add ability to find within select dropdown when over 40 elements.
58 bytes, text/x-review-board-request
jaws
: review+
mconley
: review+
Details
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
See https://bug1091592.bmoattachments.org/attachment.cgi?id=8776005 for a spec. This should add a search box to the dropdown that filters the visible options.

We will need to keep current keyboard behavior while adding this feature. If focus is given to the filter then keypresses should filter the results while adding characters to the query box.
Mentor: mconley, jaws
Attachment #8800835 - Attachment is obsolete: true
Attachment #8800836 - Attachment is obsolete: true
Attachment #8800837 - Attachment is obsolete: true
Attachment #8800838 - Attachment is obsolete: true
Attachment #8800839 - Attachment is obsolete: true
Attachment #8800840 - Attachment is obsolete: true
Attachment #8800841 - Attachment is obsolete: true
Sorry for the delay here, I've been hitting a "500 Internal Server Error" since Friday when I try to publish my review comments.
I copy/pasted my review comments from mozreview since these won't publish (bug 1315395). Sorry for formatting issues.

    You will need a layout peer to review the changes to nsXULPopupManager. Please add r?enndeakin to your commit message to flag Neil for review on your next go-round.

    Since you're only reading one pref from "dom.forms.", you can use the simpler API and remove these two lines.

    When you actually need to read the value of the pref, you can call
    Services.prefs.getBoolPref("dom.forms.selectSearch") which will return the value of the pref.

    We should add to this test to simulate a search.

    It looks like your editor removed the trailing whitespace from lines that you're not editing. We should remove these, so it's correct that you did this, but these extra trailing whitespace should be removed by a separate bug so it will be easier to track down why a file changed in the future when looking back.

    For instance, if we removed the trailing whitespace throughout the file as part of this changeset, it may be confusing to people who see that this comment changed when a search box was added to the form, since the comment has nothing to do with the search box.

nsXULPopupManager::UpdateSearchListeners()

	  if (content && content->IsXULElement(nsGkAtoms::textbox)) {

		  newTarget = content;

	  }

    You should add an assertion here so that this will crash if the search box is not the first child of the menupopup.

    if (...) {
    } else {
      NS_ASSERTION(false, "search box expected to be first child of menupopup");
    }

——————>|aEvent->GetType(eventType);

	if (eventType.EqualsLiteral("blur") ||

    nit, use spaces for indentation, not tabs.

    Please undo all the changes to test_menulist.xul since they're only whitespace changes and not needed for this bug.

    Please undo all the changes to xul_selectcontrol.js since they're only whitespace changes and not needed for this bug.

    Ditto for menu.xml

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

const {classes: Cc, utils: Cu, interfaces: Ci} = Components;

    nit, only one blank line above this const declaration and only one blank line after the getService call.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

    nit, please remove these extra blank lines.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

                          parentElement = null, isGroupDisabled = false, adjustedTextSize = -1) {

                          parentElement = null, isGroupDisabled = false, adjustedTextSize = -1, addSearch = true) {

    nit, place adjustedTextSize = -1, addSearch = true) { on a new line since it is getting really wide now.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

    nit, please remove this added blank line.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

  if (prefs.getBoolPref("selectSearch") && addSearch && element.childElementCount > minimumElements) {

    nit, use Services.prefs.getBoolPref("dom.forms.selectSearch") here and remove the getService call above.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

          prevCaption.setAttribute("hidden", (allHidden ? "true" : "false"));

    You can use the .hidden property instead of the attribute.

    Just do,
    prevCaption.hidden = allHidden;

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

            prevCaption.setAttribute("hidden", (allHidden ? "true" : "false"));

    Ditto.

toolkit/modules/SelectParentHelper.jsm (Diff revision 1)

      if (itemLabel.includes(input) || itemTooltip.includes(input)) {

        currentItem.setAttribute("hidden", "false");

        allHidden = false;

      } else {

        currentItem.setAttribute("hidden", "true");

      }

    currentItem.hidden = itemLabel.includes(input) || itemTooltip.includes(input);
    allHidden = allHidden && currentItem.hidden;

    (that last line above is written such that once allHidden becomes false it will stay false until one of the lines above explicitly resets it to true).
Attachment #8806875 - Flags: review?(mconley)
Attachment #8806875 - Flags: review?(jaws)
Attachment #8806875 - Flags: review-
Attachment #8806875 - Flags: review?(enndeakin)
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

The idea seems fine but I don't think we should be hard-coding the searchfield handling into the menu code. We definitely don't want to assert in UpdateSearchListeners when the textbox isn't there since that will asser/crash whenever any other menu is opened.

One one hand though it might be ok to just disable key events when any textbox in any menu is focused, and instead of using UpdateSearchListeners at all, just check for a textbox in the focus event handler and update the key events then. I'm concerned that there will be addon compatibility issues with this or with your patch as is.

A better approach I think would be to move all the special handling into SelectParentHelper.jsm and just change nsMenuPopupFrame::AttributeChanged so that it responds to changes to the ignorekeys attribute. Then the js module can change the ignorekeys attribute when needed.

The changes here will mostly break keyboard only users.
 - if someone scrolls such that the textbox is scrolled out of view, the textbox will no longer be visible without using the mouse
 - since the only item in the menu that can be focused is the textbox, there is no means of blurring it, so one cannot navigate with the keyboard anymore. (apart from pressing F6 which isn't as obvious)
 - once the textbox is focused, a keyboard only user can no longer close the menu by pressing Escape.
 - pressing Enter when the textbox is focused should do something I think

I'm not sure what the answer to all of these are; perhaps some keys will need to still be handled while the textbox is focused.

Also, with a textbox in the menu, you will probably need to change the level of the popup for ContentSelectDropdown to 'parent' so that IME popups will appear on top properly. This will need to be verified on all platforms. (and I think the macos dashboard will appear incorrectly as well).
Attachment #8806875 - Flags: review?(enndeakin) → review-
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review92744

neil's feedback is good too. looking forward to your next version of this patch.

::: browser/base/content/test/general/browser_selectpopup.js:505
(Diff revision 2)
> +  // input of 0 will filter these out.
> +  EventUtils.synthesizeKey("O", {}, win);

the comment says 0 (zero) but the code says O (capital O)

::: browser/base/content/test/general/browser_selectpopup.js:507
(Diff revision 2)
> +  currentOption = selectPopup.childNodes[2];
> +  is(currentOption.hidden, false, "First option should be visible");

this is not actually the first option so the comment is wrong.

instead of having a currentOption variable that points at various elements, maybe you should just use selectPopup.childNodes[n].hidden in its place

::: toolkit/modules/SelectParentHelper.jsm:255
(Diff revision 2)
> +    //  lower case for comparison
> +    let itemLabel = currentItem.getAttribute("label").toLowerCase();
> +    let itemTooltip = currentItem.getAttribute("title").toLowerCase();
> +
> +    // If search input is empty, all options should be shown
> +    if (input=="") {

I think we require spaces around binary operators. You should run ./mach eslint <path-to-file> to see what might need fixing.

Either way, we usually would just write this as `if (!input) { ... }`
Attachment #8806875 - Flags: review?(jaws) → review-
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review93226

Clearing review until jaws' and Enn's feedback is addressed.
Attachment #8806875 - Flags: review?(mconley)
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review92744

> the comment says 0 (zero) but the code says O (capital O)

Good catch here.  I think this comment will be dropped, because we're using a different CONTENT page from last time.

> this is not actually the first option so the comment is wrong.
> 
> instead of having a currentOption variable that points at various elements, maybe you should just use selectPopup.childNodes[n].hidden in its place

I was refering to childNodes[2] as the first option because childNodes[1] is a Group header and not a selectable option.
I'll add a comment to clear up the content structure here and use selectPopup.childNodes[n].hidden to get rid of the currentOption variable.

> I think we require spaces around binary operators. You should run ./mach eslint <path-to-file> to see what might need fixing.
> 
> Either way, we usually would just write this as `if (!input) { ... }`

Eslint did not show any errors for this file when I ran before submitting this revision, should I add the spaces anyways?
I'll modify this line to use !input, instead.
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review93716

Hey there, great idea !
I just threw some ideas that would help this be even more helpful. 
Feel free to ignore them if you think they're not that helpful.
Thanks

::: toolkit/modules/SelectParentHelper.jsm:234
(Diff revision 2)
> +}
> +
> +function onSearchInput() {
> +  let searchObj = this;
> +  // Get input from search field, set to all lower case for comparison
> +  let input = searchObj.value.toLowerCase();

What would be great would be to replace diacritics, here on the search string and on item label and tooltip value (in l.251), with their non-diacritics matching characters (e.g. é->e , à->a, ù->u, ... ).

This would really benefits for lots of languages that uses diacritics (french, german, turkish, ...).

::: toolkit/modules/SelectParentHelper.jsm:272
(Diff revision 2)
> +            prevCaption.hidden = allHidden;
> +        }
> +        prevCaption = null;
> +        allHidden = true;
> +      }
> +      if (itemLabel.includes(input) || itemTooltip.includes(input)) {

Maybe we could have some kind of fuzzy search here ?
(In reply to Tyler Maklebust from comment #15)
> Eslint did not show any errors for this file when I ran before submitting
> this revision, should I add the spaces anyways?
> I'll modify this line to use !input, instead.

Those rules just landed and apparently weren't in there when you were testing. I had already reviewed them so I knew they were on their way. See bug 1316882 if you're interested.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #16)
> Comment on attachment 8806875 [details]
> Bug 1309935 - Add ability to find within select dropdown when over 40
> elements
> 
> https://reviewboard.mozilla.org/r/90168/#review93716
> 
> Hey there, great idea !
> I just threw some ideas that would help this be even more helpful. 
> Feel free to ignore them if you think they're not that helpful.
> Thanks
> 
> ::: toolkit/modules/SelectParentHelper.jsm:234
> (Diff revision 2)
> > +}
> > +
> > +function onSearchInput() {
> > +  let searchObj = this;
> > +  // Get input from search field, set to all lower case for comparison
> > +  let input = searchObj.value.toLowerCase();
> 
> What would be great would be to replace diacritics, here on the search
> string and on item label and tooltip value (in l.251), with their
> non-diacritics matching characters (e.g. é->e , à->a, ù->u, ... ).

Let's do this work in a follow-up bug since this one here already has quite a bit going in to it.
 
> This would really benefits for lots of languages that uses diacritics
> (french, german, turkish, ...).
> 
> ::: toolkit/modules/SelectParentHelper.jsm:272
> (Diff revision 2)
> > +            prevCaption.hidden = allHidden;
> > +        }
> > +        prevCaption = null;
> > +        allHidden = true;
> > +      }
> > +      if (itemLabel.includes(input) || itemTooltip.includes(input)) {
> 
> Maybe we could have some kind of fuzzy search here ?

Something that allows for a character distance like Levenshtein? We do have NLP.jsm, http://searchfox.org/mozilla-central/source/toolkit/modules/NLP.jsm#30
> Let's do this work in a follow-up bug since this one here already has quite a bit going in to it.

Sure

> Something that allows for a character distance like Levenshtein? We do have NLP.jsm, http://searchfox.org/mozilla-central/source/toolkit/modules/NLP.jsm#30

Looks great, I guess this would handle diacritics characters too.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18)
> > Something that allows for a character distance like Levenshtein? We do have NLP.jsm, http://searchfox.org/mozilla-central/source/toolkit/modules/NLP.jsm#30
> 
> Looks great, I guess this would handle diacritics characters too.

Yeah, actually in a way it would by default, but it doesn't scale well. A word like 'naïve' would have a distance of 1 from 'naive', but 'diakrī́nō' has a distance of 3 from 'diakrino'.
Attachment #8806875 - Flags: review?(enndeakin)
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review96582

Great work on this. This looks mostly okay. I have some style nits, and I think I've found a bug. <option> elements can be hidden by content setting display: none on them. I think when you filter (and stop filtering), you're not taking that fact into account. Or am I misreading?

::: browser/base/content/test/general/browser_selectpopup.js:493
(Diff revision 3)
>        let bpBottom = parseFloat(cs.paddingBottom) + parseFloat(cs.borderBottomWidth);
> +      let selectedOption = 60;
> +
> +      if (Services.prefs.getBoolPref("dom.forms.selectSearch")) {
> +        // Use option 61 instead of 60, as a search field has been added to the top
> +        // of the popup when selectPopup was opened, after option 60 was selected

Maybe this could be made clearer as "the 60th option element is actually the 61st child, since the first child is the searching input field."

::: toolkit/modules/SelectParentHelper.jsm:279
(Diff revision 3)
>          item.classList.add("contentSelectDropdown-ingroup")
>        }
>      }
>    }
> +
> +  let minimumElements = 40;

Let's make this a const, and perhaps toss it up to the top of this module.

::: toolkit/modules/SelectParentHelper.jsm:306
(Diff revision 3)
> +          break;
> +        case "ArrowDown":
> +        case "Enter":
> +        case "Tab":
> +          searchbox.blur();
> +          if (searchbox.nextSibling.localName=="menuitem" &&

Nit: spaces on either side of ==.

::: toolkit/modules/SelectParentHelper.jsm:307
(Diff revision 3)
> +        case "ArrowDown":
> +        case "Enter":
> +        case "Tab":
> +          searchbox.blur();
> +          if (searchbox.nextSibling.localName=="menuitem" &&
> +              searchbox.nextSibling.hidden == false) {

`!searchbox.nextSibling.hidden`

::: toolkit/modules/SelectParentHelper.jsm:311
(Diff revision 3)
> +          if (searchbox.nextSibling.localName=="menuitem" &&
> +              searchbox.nextSibling.hidden == false) {
> +            menulist.menuBoxObject.activeChild = searchbox.nextSibling;
> +          } else {
> +            var currentOption = searchbox.nextSibling;
> +            while (currentOption && (currentOption.localName!="menuitem" ||

Spaces on either side of !=.

::: toolkit/modules/SelectParentHelper.jsm:312
(Diff revision 3)
> +              searchbox.nextSibling.hidden == false) {
> +            menulist.menuBoxObject.activeChild = searchbox.nextSibling;
> +          } else {
> +            var currentOption = searchbox.nextSibling;
> +            while (currentOption && (currentOption.localName!="menuitem" ||
> +                  currentOption.hidden == true)) {

|| currentOption.hidden) is fine.

::: toolkit/modules/SelectParentHelper.jsm:359
(Diff revision 3)
> +    let itemTooltip = currentItem.getAttribute("title").toLowerCase();
> +
> +    // If search input is empty, all options should be shown
> +    if (!input) {
> +      currentItem.hidden = false;
> +    } else if (currentItem.localName=="menucaption") {

Spaces on either side of ==.

::: toolkit/modules/SelectParentHelper.jsm:360
(Diff revision 3)
> +
> +    // If search input is empty, all options should be shown
> +    if (!input) {
> +      currentItem.hidden = false;
> +    } else if (currentItem.localName=="menucaption") {
> +      if (prevCaption!=null) {

Spaces on either side of !=.

::: toolkit/modules/SelectParentHelper.jsm:375
(Diff revision 3)
> +        }
> +        prevCaption = null;
> +        allHidden = true;
> +      }
> +      if (itemLabel.includes(input) || itemTooltip.includes(input)) {
> +        currentItem.hidden = false;

How do we decern whether or not we had hidden the menuitem, or the menuitem is hidden due to the <option> having display: none; on it?
Attachment #8806875 - Flags: review?(mconley) → review-
In SelectParentHelper.jsm we know if the option was hidden by display:none; by referencing the option.display property at line 237. We could add another attribute to the item to state whether it was hidden by content, and thus to not unhide it.
>     item->SetIgnoreKeys(aIgnoreKeys ? eIgnoreKeys_True : eIgnoreKeys_False);

The default value of ignorekeys for the select popup isn't true or false (it is currently 'handled' or after bug 1313130, it will be 'shortcuts', so this will need to be taken into account.
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review98622

Thanks tmacklebust! A few questions and minor things below, but this is looking great. Glad we were able to address the "hidden by content" problem.

::: browser/base/content/test/general/browser_selectpopup.js:494
(Diff revision 4)
>        let bpBottom = parseFloat(cs.paddingBottom) + parseFloat(cs.borderBottomWidth);
> +      let selectedOption = 60;
>  
> -      is(selectPopup.childNodes[60].getBoundingClientRect().bottom,
> +      if (Services.prefs.getBoolPref("dom.forms.selectSearch")) {
> +        // Use option 61 instead of 60, as the 60th option element is actually the
> +	      // 61st child, since the first child is now the search input field.

Busted alignment here.

::: browser/base/content/test/general/browser_selectpopup.js:574
(Diff revision 4)
> +}
> +
> +// This test checks the functionality of search in select elements with groups
> +// and a large number of options.
> +add_task(function* test_select_search() {
> +  if (Services.prefs.getBoolPref("dom.forms.selectSearch")) {

Instead of testing if this pref is set and only running the test of it is, we should probably make sure the pref is set to true, and reset it if necessary after the test. This will help us avoid breakage and regressions during the window of time where this patch is landed but the feature is not enabled.

You can do the above like so:

```JavaScript
add_task(function* test_select_search() {
  yield SpecialPowers.pushPrefEnv({
    set: [
      ["dom.forms.selectSearch", true],
    ],
  });

  // Do test ...
  
  // Now reset the pref back to what it was originally.
  yield SpecialPowers.popPrefEnv();
});

::: modules/libpref/init/all.js:1193
(Diff revision 4)
>  
>  // Enables requestAutocomplete DOM API on forms.
>  pref("dom.forms.requestAutocomplete", false);
>  
> +// Enable search in <select> dropdowns (more than 40 options)
> +pref("dom.forms.selectSearch", true);

Did we decide at some point that this would be enabled by default upon landing?

::: toolkit/modules/SelectParentHelper.jsm:348
(Diff revision 4)
> +  // Get all items in dropdown (could be options or optgroups)
> +  let menupopup = searchObj.parentElement;
> +  let menuItems = menupopup.querySelectorAll("menuitem, menucaption");
> +
> +  // Flag used to detect any group headers with no visible options.
> +  //  These group headers should be hidden.

Nit - alignment

::: toolkit/modules/SelectParentHelper.jsm:351
(Diff revision 4)
> +
> +  // Flag used to detect any group headers with no visible options.
> +  //  These group headers should be hidden.
> +  let allHidden = true;
> +  // Keep a reference to the previous group header (menucaption) to go back
> +  //  and set to hidden if all options within are hidden.

Nit: alignment

::: toolkit/modules/SelectParentHelper.jsm:358
(Diff revision 4)
> +
> +  for (let currentItem of menuItems) {
> +    // Make sure we don't show any options that were hidden by page content
> +    if (!currentItem.hiddenByContent) {
> +      // Get label and tooltip (title) from option and change to
> +      //  lower case for comparison

Nit: alignment
Attachment #8806875 - Flags: review?(mconley) → review-
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review98630

Looks good, should be ready after this land round of feedback is addressed.

::: browser/base/content/test/general/browser_selectpopup.js:494
(Diff revision 4)
>        let bpBottom = parseFloat(cs.paddingBottom) + parseFloat(cs.borderBottomWidth);
> +      let selectedOption = 60;
>  
> -      is(selectPopup.childNodes[60].getBoundingClientRect().bottom,
> +      if (Services.prefs.getBoolPref("dom.forms.selectSearch")) {
> +        // Use option 61 instead of 60, as the 60th option element is actually the
> +	      // 61st child, since the first child is now the search input field.

This alignment is busted because a Tab character got introduced, please use only spaces.

::: browser/base/content/test/general/browser_selectpopup.js:578
(Diff revision 4)
> +add_task(function* test_select_search() {
> +  if (Services.prefs.getBoolPref("dom.forms.selectSearch")) {
> +    const pageUrl = "data:text/html," + escape(PAGECONTENT_GROUPS);
> +    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, pageUrl);
> +
> +    yield* performSelectSearchTests(window);

What does yield* do? I think you can omit the * here.

::: toolkit/modules/SelectParentHelper.jsm:295
(Diff revision 4)
> +    // Add a search text field as the first element of the dropdown
> +    let searchbox = element.ownerDocument.createElement("textbox");
> +    searchbox.setAttribute("type", "search");
> +    searchbox.addEventListener("input", onSearchInput);
> +    searchbox.addEventListener("focus", onSearchFocus);
> +    searchbox.addEventListener("blur", onSearchBlur);

Comment 12 says "since the only item in the menu that can be focused is the textbox, there is no means of blurring it, so one cannot navigate with the keyboard anymore. (apart from pressing F6 which isn't as obvious)".

Did you verify that the onSearchBlur function is being called?
Attachment #8806875 - Flags: review?(jaws) → review-
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review98630

> What does yield* do? I think you can omit the * here.

Omitting the * seems to work fine, too, and I've removed it for the next revision.  I was following the format for calling test functions used with other tests in this file.

> Comment 12 says "since the only item in the menu that can be focused is the textbox, there is no means of blurring it, so one cannot navigate with the keyboard anymore. (apart from pressing F6 which isn't as obvious)".
> 
> Did you verify that the onSearchBlur function is being called?

I checked and onSearchBlur is being called when one of the keyboard methods of leaving search is used, which takes focus away from the text field.
(In reply to Tyler Maklebust from comment #27)
> Comment on attachment 8806875 [details]
> Bug 1309935 - Add ability to find within select dropdown when over 40
> elements.
> 
> https://reviewboard.mozilla.org/r/90168/#review98630
> 
> > What does yield* do? I think you can omit the * here.
> 
> Omitting the * seems to work fine, too, and I've removed it for the next
> revision.  I was following the format for calling test functions used with
> other tests in this file.

Okay, no problem. Thanks for removing it. The * on a function definition marks it as a 'generator function', so it is usually used with function*() {}.
 
> > Comment 12 says "since the only item in the menu that can be focused is the textbox, there is no means of blurring it, so one cannot navigate with the keyboard anymore. (apart from pressing F6 which isn't as obvious)".
> > 
> > Did you verify that the onSearchBlur function is being called?
> 
> I checked and onSearchBlur is being called when one of the keyboard methods
> of leaving search is used, which takes focus away from the text field.

Great, thanks for verifying.
Hey Tyler, can you upload an updated patch here?
Flags: needinfo?(maklebus)
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review96582

> How do we decern whether or not we had hidden the menuitem, or the menuitem is hidden due to the <option> having display: none; on it?

This appears to be addressed now in the latest patch.

tmacklebust - would you mind marking the issues that have been fixed as "Fixed" and "Drop"ping the ones that don't apply or have been pushed back against?
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review100760

I'm pretty happy with this. I've got a nit on how we're importing Services.jsm, but otherwise, I think this is great.

Want to double check with jaws though that we want this enabled by default.

::: modules/libpref/init/all.js:1192
(Diff revision 6)
>  
>  // Enables requestAutocomplete DOM API on forms.
>  pref("dom.forms.requestAutocomplete", false);
>  
> +// Enable search in <select> dropdowns (more than 40 options)
> +pref("dom.forms.selectSearch", true);

jaws - are we sure we want to land this enabled by default?

::: toolkit/modules/SelectParentHelper.jsm:14
(Diff revision 6)
>  ];
>  
>  const {utils: Cu} = Components;
>  const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm");
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");

We can just use Cu here. Try:

const {Services} = Cu.import("resource://gre/modules/Services.jsm"), like on line 12.
Attachment #8806875 - Flags: review?(mconley) → review+
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review100760

> jaws - are we sure we want to land this enabled by default?

Do we know of bugs that would push us to disable it? The only bug that I know of right now is that the popup flips as the contents are hidden. We should diasble the flipping if peopel are searching.

Seeing it flip like that in the project video, I'm inclined to land this feature disabled, and then once we fix the flipping issue then we can enable it.
Comment on attachment 8806875 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/90168/#review100790

r=me, I agree with mconley. Let's change that pref to 'false' by default for now until we can fix the flipping issue.
Attachment #8806875 - Flags: review?(jaws) → review+
Flags: needinfo?(mconley)
Comment on attachment 8827618 [details]
Bug 1309935 - Add ability to find within select dropdown when over 40 elements.

https://reviewboard.mozilla.org/r/105236/#review106086
Attachment #8827618 - Flags: review?(jaws) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/544ad41d3dcf
Add ability to find within select dropdown when over 40 elements. r=jaws
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/544ad41d3dcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Release Note Request (optional, but appreciated)
[Why is this notable]: Cool new user facing feature
[Affects Firefox for Android]: Don't think so?
[Suggested wording]: Add ability to find within select dropdown menu when over 40 elements
[Links (documentation, blog post, etc)]: 

I guess this won't ship with 53.
relnote-firefox: --- → ?
Depends on: 1332280
Flags: needinfo?(tmaklebust)
Depends on: 1332294
(In reply to Sylvestre Ledru [:sylvestre] from comment #42)
> I guess this won't ship with 53.

Yeah, this is currently disabled by default. I suspect we won't want to relnote this until it's enabled (see bug 1332301).

Should we go ahead and clear that relnote flag?
No longer blocks: 1332301
Flags: needinfo?(sledru)
I don't mind keeping it until it is activated :)
Flags: needinfo?(sledru)
I feel like the searchbox should show up as soon as the select box has a scrollbar.
Depends on: 1337021
Depends on: 1337023
Depends on: 1337025
See Also: → 1335483
See Also: → 1300784
Depends on: 1337034
Depends on: 1343569
No longer depends on: 1337034
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: