1442183 - Only one website in the SiteDataManager list can be selected

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID:20180301024724

[Affected versions]:
Nightly 60.0a1

[Affected platforms]:
Mac OS X 10.12, Windows 10 x64

[Preconditions]:
Have some websites in Settings list (more than 2)

[Steps to reproduce]:
1.Launch Nightly 60.0a1 with a new profile
2.Open Settings dialogue from about:preferences#privacy - Cookies and Site Data section
3.Press Shift+mouse click and select more than one website from the Settings list

[Expected result]:
The websites should be selected and highlighted

[Actual result]:
Only one website can be selected and highlighted

[Note]:
The issue is not reproducible in Exceptions dialogue list

Comment 1

[Johann Hofmann [:johannh]]

This will be fixed by bug 1438147, but I guess it makes sense to track it individually.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1442183 - Allow multiple selection in the site data manager list.

Review commit: https://reviewboard.mozilla.org/r/235538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235538/

Comment 3

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccea321197a7a92a1dc56531b658057d04ec2975

Comment 4

[:prathiksha]

Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.

https://reviewboard.mozilla.org/r/235538/#review241484

Looks good to me. :)

::: browser/components/preferences/siteDataSettings.js:262
(Diff revision 1)
>      this._buildSitesList(this._sites);
>      this._list.clearSelection();
>    },
>  
>    onClickRemoveSelected() {
> -    let selected = this._list.selectedItem;
> +    for (let selected of this._list.selectedItems) {

I think we can avoid the loop here and just have this._removeSiteItems(this._list.selectedItems);

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235538/diff/1-2/

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.

https://reviewboard.mozilla.org/r/235538/#review241484

> I think we can avoid the loop here and just have this._removeSiteItems(this._list.selectedItems);

Hah, great catch, thank you!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99b6961d14a2
Allow multiple selection in the site data manager list. r=prathiksha

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/99b6961d14a2

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.

Approval Request Comment
[Feature/Bug causing the regression]: New feature
[User impact if declined]: Slight usability issues in the site data manager as described in bug 1453589
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Works in Nightly for me
[Needs manual test from QE? If yes, steps to reproduce]: Open the site data manager in about:preferences. Try to select and delete multiple sites at once (e.g. through shift+click or cmd+click).
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not at all
[Why is the change risky/not risky?]: It's a very straightforward patch (a few line of code + tests) and any unlikely fallout would be contained to the front-end of that dialog.
[String changes made/needed]: None

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.

makes sense.  approved for 60.0b13

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/092683f139e9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for a variety of ESLint failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=173698550&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/f2f47730725e00bbfbfdbf0934895b29823c8c16

Comment 13

[Johann Hofmann [:johannh]]

Yeah that whole test directory moved around, unfortunately. I think it's sensible to land this in beta without the test, that test is mostly meant as a regression test so that we don't lose the functionality. It should be absolutely sufficient to do a quick manual verification that multi-select works in beta.

Comment 14

[Johann Hofmann [:johannh]]

(I'll make a patch)

Comment 15

[Johann Hofmann [:johannh]]

Attachment: Patch for beta

This applies cleanly on beta for me (and works). Ryan, does that work for you?

Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I'm not crazy about dropping the test given that we'll be living with ESR60 for the next year, but if that's the best option in your opinion, oh well I guess.

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da52dbcbd71c

Comment 18

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 60.0b12, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 61.0a1 (build ID:20180415220108) and Fx 60.0b13, on Windows 10, mac OS 10.13 and Ubunut 14.04 LTS.