Policy: Hide "Add Exception" on certificate error

VERIFIED FIXED in Firefox 60

Status

()

enhancement
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

60 Branch
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox60+ verified, firefox61 verified)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
Hide the Add Exception button
(Assignee)

Updated

a year ago
Depends on: 1442719
Comment hidden (mozreview-request)

Comment 3

a year ago
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 6

a year ago
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/#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-
(Assignee)

Comment 7

a year ago
(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 8

a year ago
mozreview-review
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 11

a year ago
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/#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

a year ago
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
(Assignee)

Comment 15

a year ago
Posted patch Follow-upSplinter Review

Comment 16

a year ago
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
(Assignee)

Comment 17

a year ago
[Tracking Requested - why for this release]:
Summary: Policy: Disable "Add Exception" on certificate error → Policy: Hide "Add Exception" on certificate error

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca550bc9020b
https://hg.mozilla.org/mozilla-central/rev/4a473ea3d1bf
https://hg.mozilla.org/mozilla-central/rev/62839ba7b41c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.