Policy: Hide "Add Exception" on certificate error

VERIFIED FIXED in Firefox 60

Status

()

enhancement
VERIFIED FIXED
Last year
Last year

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

60 Branch
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60+ verified, firefox61 verified)

Details

Attachments

(3 attachments)

Hide the Add Exception button
Depends on: 1442719

Comment 3

Last year
mozreview-review
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review238572


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

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


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


::: browser/base/content/test/about/browser_aboutCertError.js:348
(Diff revision 1)
> +
> +  for (let useFrame of [false, true]) {
> +    let tab = await openErrorPage(BAD_CERT, useFrame);
> +    let browser = tab.linkedBrowser;
> +
> +    let message = await ContentTask.spawn(browser, {frame: useFrame}, async function({frame}) {

Error: 'message' is assigned a value but never used. [eslint: no-unused-vars]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review238884

::: browser/base/content/aboutNetError.xhtml:373
(Diff revision 2)
>  
>              // set the checkbox
>              checkbox.checked = !!options.automatic;
>            }
> +          if (options && options.disableAddExceptionButton) {
> +            document.getElementById("exceptionDialogButton").disabled = true;

I don't see what good disabling the button but keeping it visible does. There's nothing that the user can do to enable the button so it will just be a tease.
Attachment #8964393 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8964393 [details]
> Bug 1450761 - Add pref to disable the Add Exception button on certificate
> error pages.
> 
> https://reviewboard.mozilla.org/r/233116/#review238884
> 
> ::: browser/base/content/aboutNetError.xhtml:373
> (Diff revision 2)
> >  
> >              // set the checkbox
> >              checkbox.checked = !!options.automatic;
> >            }
> > +          if (options && options.disableAddExceptionButton) {
> > +            document.getElementById("exceptionDialogButton").disabled = true;
> 
> I don't see what good disabling the button but keeping it visible does.
> There's nothing that the user can do to enable the button so it will just be
> a tease.

This has been the UX direction that we're following for Policies. Every policy is disabling items instead of hiding them, so this is just to keep consistent with the other policies.
Comment on attachment 8964394 [details]
Bug 1450761 - Policy to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233118/#review238888
Attachment #8964394 - Flags: review?(jaws) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> > Comment on attachment 8964393 [details]
> > Bug 1450761 - Add pref to disable the Add Exception button on certificate
> > error pages.
> > 
> > https://reviewboard.mozilla.org/r/233116/#review238884
> > 
> > ::: browser/base/content/aboutNetError.xhtml:373
> > (Diff revision 2)
> > >  
> > >              // set the checkbox
> > >              checkbox.checked = !!options.automatic;
> > >            }
> > > +          if (options && options.disableAddExceptionButton) {
> > > +            document.getElementById("exceptionDialogButton").disabled = true;
> > 
> > I don't see what good disabling the button but keeping it visible does.
> > There's nothing that the user can do to enable the button so it will just be
> > a tease.
> 
> This has been the UX direction that we're following for Policies. Every
> policy is disabling items instead of hiding them, so this is just to keep
> consistent with the other policies.

In the prefs we include a string explaining why some items are disabled but we don't do that on this screen. We should bring this up with UX.
:amin is out on PTO until tomorrow and I understand that this has a high priority to land soon so this can be uplifted to Beta.

I talked to :shorlander and he agreed with hiding the button in this case:
"[U]nless we are going to add some way to explain why it is disabled, I agree that a it should be hidden and not disabled ("Greyed out"). Although that does make the "Advanced --> Show Info that does nothing" flow kind of strange. Anyway, future improvements aside, I agree we shouldn't show a disabled button that has no context around why it is disabled."
Comment on attachment 8964393 [details]
Bug 1450761 - Add pref to disable the Add Exception button on certificate error pages.

https://reviewboard.mozilla.org/r/233116/#review239310

r=me if you switch to hiding instead of disabling.
Attachment #8964393 - Flags: review- → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

Last year
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca550bc9020b
Add pref to disable the Add Exception button on certificate error pages. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a473ea3d1bf
Policy to disable the Add Exception button on certificate error pages. r=jaws

Comment 16

Last year
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62839ba7b41c
Follow-up, remove check that was added in the previous version of the patch, but is not needed anymore. r=fgomes
[Tracking Requested - why for this release]:
Summary: Policy: Disable "Add Exception" on certificate error → Policy: Hide "Add Exception" on certificate error
We tested this on the latest nightly with JSON policy format and it is verified as fixed.
With this policy, "Add Exception" option on certificate error pages can be disabled (hidden).

We will retest this with adm policy format when ready.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.