Closed Bug 592904 Opened 9 years ago Closed 9 years ago

Hide the Form Assistant select UI filter box for small select lists

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(fennec2.0b3+)

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: mfinkle, Assigned: wesj)

Details

Attachments

(1 file, 1 obsolete file)

If a select only has a few items, or all the items are visible on screen, we do not need the filter UI. It is just confusing for small lists.

For large lists, it is still confusing for users who don't know what it does. We should add a simple placeholder hint: "filter list" or "search list"
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b3+
Another idea is to collapse the Select UI when the user picks an item from a single selection combobox. The FormHelper bar should still be present so the forward/back buttons can be used.
Assignee: nobody → wjohnston
Attached patch Patch - Tweak filterbar (obsolete) — Splinter Review
This addresses things in the first comment. Only shows the filter bar if we overflow (note that doesn't necessarily mean the list is long, it one element slightly overflow it will be shown), and adds some placeholder text reading "Filter List".
Attachment #487031 - Flags: review?(mark.finkle)
Comment on attachment 487031 [details] [diff] [review]
Patch - Tweak filterbar

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

> var SelectHelperUI = {
>   _list: null,
>   _selectedIndexes: null,
>-
>+  _initialized : false,
>+  

Drop the _initialized and just call SelectHelperUI.init() here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#504

>+  init: function() {
>+    if (this._initialized)
>+      return;

Remove the if check too

>+
>+    this._initialized = true;
>+    this._panel.addEventListener("overflow", this, true);

Do we need this attached all the time? I think we add / remove event listeners in show() / hide(). Can you do the same?

>-    this._panel.height = this._panel.getBoundingClientRect().height;
>+    this.showFilter = this.showFilter;

Kinda weird to see this. Add a comment about why we are cranking the setter here. Or reorganize the code to make this easier to understand.

>+    let height = this._panel.getBoundingClientRect().height;
>+    this._panel.height = height;

Why make the local here?

>+          <textbox id="select-helper-textbox" class="search-bar content-navigator-item" oncommand="SelectHelperUI.filter(this.value)"
>+                   placeholder="&select-helper.emptytext;" type="search" flex="1" hidden="true"/>

I don't see select-helper.emptytext in a DTD. And can we name it selectHelper.emptytext

r- for some small fixes
Attachment #487031 - Flags: review?(mark.finkle) → review-
Attached patch Patch v2Splinter Review
Nits addressed
Attachment #487031 - Attachment is obsolete: true
Attachment #488207 - Flags: review?(mark.finkle)
Attachment #488207 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb2][has-patch]
pushed:
http://hg.mozilla.org/mobile-browser/rev/8b3cd5a5cfcc

The filter box is hidden if the contents of the list don't overflow the list (need to be panned)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb2][has-patch]
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.