1323420 - Add a pref to control the close button on web permission prompts

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: close-button.patch

In bug 1282768 I attached a patch that adds a close button ("x") to the permission prompts. The team decided to land this behind a pref, disabled by default, in case we decide too late in the beta cycle that we want to provide the 'x'. I'm attaching the original patch here, but it needs to be modified to consult the new pref.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1323420 - Add a pref to control the close button on web permission prompts.

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

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

I had a few spare minutes, thought I'd look at this.

I don't really like this patch as a patch, but thought I'd upload and ask if it's in the direction we want. It definitely takes your patch and puts its changes behind a pref, but I have a feeling I might be missing some context :)

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

https://reviewboard.mozilla.org/r/103134/#review103750

::: browser/app/profile/firefox.js:519
(Diff revision 1)
>  pref("privacy.cpd.siteSettings",            false);
>  pref("privacy.cpd.openWindows",             false);
>  
>  pref("privacy.history.custom",              false);
>  
> +pref("privacy.permissionPrompts.dismissOnClose", false);

Something like showClose would be more accurate: this is about displaying the X button, not controlling dismissal.

::: browser/modules/PermissionUI.jsm:343
(Diff revision 1)
>      let options = this.popupOptions;
>  
>      if (!options.hasOwnProperty('displayURI') || options.displayURI) {
>        options.displayURI = this.principal.URI;
>      }
> -    // Permission prompts are always persistent and don't have a close button.
> +    // Permission prompts are always persistent; the close button is controled by a pref.

Typo: controlled

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

Yup, that's what we want here! Nothing too fancy :)

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

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

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

https://reviewboard.mozilla.org/r/103134/#review105882

You added an 11th test in that file, so there is a (probably small) chance of intermittents. The reason we broke them up in the first place was to make the tests finish in a reasonable time. I hope it's not a problem in this case, but the safest bet would be to put it in _5.js that only has 9 tests so far :)

::: browser/app/profile/firefox.js:522
(Diff revision 2)
>  pref("privacy.cpd.siteSettings",            false);
>  pref("privacy.cpd.openWindows",             false);
>  
>  pref("privacy.history.custom",              false);
>  
> +pref("privacy.permissionPrompts.showCloseButton", false);

Actually, since this pref is checked in toolkit code, we have to set it in all.js instead.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

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

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

https://reviewboard.mozilla.org/r/103134/#review105882

I think we should keep the new test next to the existing close button test, and also there is no Test#8 so it's technically the 10th test ;)

Comment 9

[Johann Hofmann [:johannh]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #8)
> Comment on attachment 8824847 [details]
> Bug 1323420 - Add a pref to control the close button on web permission
> prompts.
> 
> https://reviewboard.mozilla.org/r/103134/#review105882
> 
> I think we should keep the new test next to the existing close button test,
> and also there is no Test#8 so it's technically the 10th test ;)

Why don't you put it in place of Test#8 then? :)

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c698bfb1310
Add a pref to control the close button on web permission prompts. r=past

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Fix eslint fail

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17f55a59a7d2
Fix curly brace positioning for eslint. r=past

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c698bfb1310
https://hg.mozilla.org/mozilla-central/rev/17f55a59a7d2

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Don't forget to request uplift.

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

Approval Request Comment
[Feature/Bug causing the regression]: Puts the close button for permissions prompts behind a pref.
[User impact if declined]: none for now, but we want to be able to switch this pref later.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: Puts a behavior behind a pref, off by default (i.e. preserves existing behavior for now).
[String changes made/needed]: none

Comment 17

[Gerry Chang [:gchang]]

Hi Andrei,
could you help verify if this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Brindusa Tot, Desktop QA]

Hani, please verify this on latest Nightly.

Comment 19

[Hani Yacoub, Desktop QA]

Verified that the button is correctly displayed when preference "privacy.permissionPrompts.showCloseButton" is set on true and not displayed when preference "privacy.permissionPrompts.showCloseButton" is set on false. Verified on Nightly 54.0a1, Build ID: 20170125030214, on OSes: 
Ubuntu 16.04 x64, Windows 10 x 64 and Mac OS X 10.11.

While testing I observed that the "x" button functionality matches the functionality of the "Don't Allow" button, so it's not only closing the permission prompt notification, it's actually Block or Block temporarily the permission(depending if "Remember this decision" is checked or not).

Is this the intended behavior for the "x" button?

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Hani Yacoub from comment #19)
> Is this the intended behavior for the "x" button?

It is.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8824847 [details]
Bug 1323420 - Add a pref to control the close button on web permission prompts.

Verified in m-c, let's uplift to aurora 53.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f370610ec22e

Comment 23

[Hani Yacoub, Desktop QA]

Verified that the button is correctly displayed and the functionality matches the functionality of the "Don't Allow" button, when preference "privacy.permissionPrompts.showCloseButton" is set on true and not displayed when preference "privacy.permissionPrompts.showCloseButton" is set on false. 

Verified on Aurora 53.0a2, Build ID: 20170130004003, on OSes: 
Ubuntu 16.04 x64, Windows 10 x 64 and Mac OS X 10.11.