1432759 - "Remove selected" should be greyed out if no item is selected in the site data manager

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Johann Hofmann [:johannh]]

Go to about:preferences#privacy and open the site data manager without selecting a site. The "Remove selected" button should be grayed out.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

https://reviewboard.mozilla.org/r/216398/#review222178


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/tests/browser_siteData2.js:182
(Diff revision 1)
> +    let firstSite = sitesList.querySelector(`richlistitem`);
> +    firstSite.click();
> +  }
> +
> +  function assertRemoveSelectedButtonEnabled() {
> +    let frameDoc = win.gSubDialog._topDialog._frame.contentDocument;

Error: 'frameDoc' is already declared in the upper scope. [eslint: no-shadow]

Comment 3

[Michael Kohler [:mkohler]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

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

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

https://reviewboard.mozilla.org/r/216398/#review222668

I think this looks good, I just had a question about the test...

::: browser/components/preferences/in-content/tests/browser_siteData2.js:133
(Diff revision 2)
>        removeBtn.doCommand();
>      }
>    }
>  });
>  
> +// Test selecting site button state

I wonder if it would be less verbose to just integrate the assertions from this test into one of the other tests, e.g. the "Test selecting and removing partial sites" test below, that way you could also check

- does the button get disabled when removing an item?
- does the button get disabled when removing all items?

What do you think?

::: browser/components/preferences/in-content/tests/browser_siteData2.js:177
(Diff revision 2)
> +  await BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +
> +  function selectOneEntry() {
> +    frameDoc = win.gSubDialog._topDialog._frame.contentDocument;
> +    let sitesList = frameDoc.getElementById("sitesList");
> +    let firstSite = sitesList.querySelector(`richlistitem`);

Nit: please use double quoutes here

Comment 5

[Johann Hofmann [:johannh]]

(Also thanks for working on this!)

Comment 6

[Michael Kohler [:mkohler]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

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

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

https://reviewboard.mozilla.org/r/216398/#review222928

r=me with the nit fixed

Thank you!

::: browser/components/preferences/in-content/tests/browser_siteData2.js:248
(Diff revision 3)
>      hosts.forEach(host => {
>        let site = sitesList.querySelector(`richlistitem[host="${host}"]`);
>        if (site) {
>          site.click();
> +        is(removeBtn.disabled, false, "Should enable the removeSelected button");
>          removeBtn.doCommand();

Nit: After this command, the removeBtn should be disabled, right? Can you add an assertion for that?

Comment 8

[Michael Kohler [:mkohler]]

Comment on attachment 8946453 [details]
Bug 1432759 - 'Remove selected' should be greyed out if no item is selected in the site data manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216398/diff/3-4/

Comment 9

[Michael Kohler [:mkohler]]

Kind of screwed up try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11210ea442d3ef881ef0fb55e6277382656ee85f

Setting the checkin-needed keyword anyway, feel free to not take it and let me run another one.

Comment 10

[Johann Hofmann [:johannh]]

Those should be all unrelated :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f37162b61839
'Remove selected' should be greyed out if no item is selected in the site data manager r=johannh

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f37162b61839

Comment 13

[roxleitan]

Verified fixed using Nightly 61.0a1(2018-03-13) and Beta 60.0b3 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.