Closed
Bug 1442183
Opened 7 years ago
Closed 7 years ago
Only one website in the SiteDataManager list can be selected
Categories
(Firefox :: Settings UI, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: roxana.leitan, Assigned: johannh)
References
(Blocks 1 open bug)
Details
(Whiteboard: [storage-v2])
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
prathiksha
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
2.24 KB,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
This will be fixed by bug 1438147, but I guess it makes sense to track it individually.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [storage-v2][triage]
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Priority: P3 → P2
Assignee | ||
Updated•7 years ago
|
Summary: In Settings list can be selected only one website → Only one website in the SiteDataManager list can be selected
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P3
Whiteboard: [storage-v2][triage] → [storage-v2]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
mozreview-review |
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);
Attachment #8966859 -
Flags: review?(prathikshaprasadsuman) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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!
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99b6961d14a2
Allow multiple selection in the site data manager list. r=prathiksha
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Comment 9•7 years ago
|
||
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
Attachment #8966859 -
Flags: approval-mozilla-beta?
Comment 10•7 years ago
|
||
Comment on attachment 8966859 [details]
Bug 1442183 - Allow multiple selection in the site data manager list.
makes sense. approved for 60.0b13
Attachment #8966859 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment hidden (obsolete) |
Comment 12•7 years ago
|
||
backout |
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
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 13•7 years ago
|
||
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.
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 14•7 years ago
|
||
(I'll make a patch)
Assignee | ||
Comment 15•7 years ago
|
||
This applies cleanly on beta for me (and works). Ryan, does that work for you?
Thanks!
Flags: needinfo?(ryanvm)
Attachment #8968143 -
Flags: review+
Comment 16•7 years ago
|
||
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.
Flags: needinfo?(ryanvm)
Comment 17•7 years ago
|
||
bugherder uplift |
Comment 18•7 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•