960354 - Multi-select drop down menus don't update properly

Status: VERIFIED FIXED

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

Description

[Jim Mathies [:jimm]]

str:
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.

Comment 1

[Jim Mathies [:jimm]]

Attachment: testcase

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

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.

Comment 3

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point estimate.

Comment 4

[Matt Brubeck (:mbrubeck)]

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)

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.2

updated per comments plus a test.

Comment 6

[Jim Mathies [:jimm]]

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.

Comment 7

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point estimate.

Comment 8

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/50a2220b70bf

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50a2220b70bf

Comment 10

[Manuela Muntean [Away]]

For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88443d38a05f

Comment 12

[u279076]

Kamil, please verify this is fixed in the latest Aurora build.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through the following verification without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-09-00-40-02-mozilla-aurora/

Before verifying the issue, reproduced the problem with the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-15-00-40-03-mozilla-aurora/

- 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

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Also found the following issue when I was going through verification:
- Bug #970181