Closed Bug 960354 Opened 11 years ago Closed 11 years ago

Multi-select drop down menus don't update properly


(Firefox for Metro Graveyard :: Browser, defect, P2)

28 Branch
Windows 8.1


(firefox28 verified, firefox29 verified)

Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified


(Reporter: jimm, Assigned: jimm)



(Whiteboard: [beta28] [defect] p=2)


(2 files, 1 obsolete file)

1) open up a form interface with a multiselect drop down
2) select an item in the list
3) select another item
4) select the previous item to unselect

result: the form element selection will update, but the popup menu item will remain selected.
Whiteboard: [triage] → [triage] [defect] p=0
Attached file testcase
Attached patch patch v.1 (obsolete) — Splinter Review
This was pretty messed up, I'm not sure what we were doing before hand. Cleaned things up so these selects are working. 

Also working a a multiselect test which I'll post in a bit.
Assignee: nobody → jmathies
Attachment #8361261 - Flags: review?(mbrubeck)
Hey Jim, can you provide a point estimate.
Blocks: metrov1it22
No longer blocks: metrov1backlog
Flags: needinfo?(jmathies)
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [triage] [defect] p=0 → [beta28] [defect] p=0
Comment on attachment 8361261 [details] [diff] [review]
patch v.1

Review of attachment 8361261 [details] [diff] [review]:

Looks good!  Minor style nits below:

::: browser/metro/base/content/helperui/SelectHelperUI.js
@@ +83,5 @@
> +
> +    // Setup pre-selected items. Note, this has to happen after show.
> +    this._listbox.clearSelection();
> +    let item;
> +    while (item = selectedItems.pop()) {

Could you use "for (let item of selectedItems)" instead?  (Just to be more idiomatic, and possibly avoid extra malloc/free from mutating the array.)

@@ +144,5 @@
> +            }
> +            // Fix up selected items - richlistbox will clear selection on click events
> +            // so we need to set selection on the items the user has previously chosen.
> +            this._listbox.clearSelection();
> +            for (let index = 0; index < this._listbox.childNodes.length; index++) {

for (let node of this._listbox.childNodes)
Attachment #8361261 - Flags: review?(mbrubeck) → review+
Attached patch patch v.2Splinter Review
updated per comments plus a test.
Attachment #8361261 - Attachment is obsolete: true
Attachment #8361293 - Flags: review?(mbrubeck)
Flags: needinfo?(jmathies)
Comment on attachment 8361293 [details] [diff] [review]
patch v.2

Review of attachment 8361293 [details] [diff] [review]:

::: browser/metro/base/content/helperui/SelectHelperUI.js
@@ +83,5 @@
> +
> +    // Setup pre-selected items. Note, this has to happen after show.
> +    this._listbox.clearSelection();
> +    let item;
> +    for (let item of selectedItems) {

oops, nixed |let item;| locally.
Hey Jim, can you provide a point estimate.
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=2
Attachment #8361293 - Flags: review?(mbrubeck) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 960999
For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.
Kamil, please verify this is fixed in the latest Aurora build.
Flags: needinfo?(kamiljoz)
Went through the following verification without any issues. Used the following build:

Before verifying the issue, reproduced the problem with the following build:

- Ensured that the multi-select drown down menu is being updated correctly
- Ensured that you can select/deselect multiple items without any issues
- Ensured that the multi-select drop down appears every single time you tap on the list box
- Ensured that you can scroll through the multi-select drop down without issues (scrolling up/down)
- Ensured that the multi-select drop down menu works with both touch and touchpad
- Ensured that all of the above test cases worked with different variations of snapped view

Found that bug that can be fixed to greatly improve the user experience. Created Bug # 970178
Flags: needinfo?(kamiljoz)
Also found the following issue when I was going through verification:
- Bug #970181
You need to log in before you can comment on or make changes to this bug.