Closed Bug 1207107 Opened 9 years ago Closed 9 years ago

Modernize the UI of aboutCertError.xhtml

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 45
Iteration:
44.3 - Nov 2
Tracking Status
firefox44 --- verified
firefox45 --- verified

People

(Reporter: past, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Keywords: user-doc-needed, Whiteboard: [fxprivacy])

Attachments

(4 files, 7 obsolete files)

207.05 KB, image/png
Details
40 bytes, text/x-review-board-request
dao
: review+
Details
40 bytes, text/x-review-board-request
dao
: review+
Details
40 bytes, text/x-review-board-request
dao
: review+
Details
aboutCertError.xhtml still contains Larry and is in dire need of a fresh coat of paint. Bram has an updated UI spec that can be found here:

http://brampitoyo.github.io/fx-untrusted-connection/

Slight alterations for different error types can be found here:

http://brampitoyo.github.io/fx-untrusted-connection/severe.xhtml
http://brampitoyo.github.io/fx-untrusted-connection/rc4.xhtml

This bug is about applying the common set of changes to the markup and stylesheets of aboutCertError.xhtml. Followup bugs will take care of the particular details of each error. This bug needs to implement the following:

- Display the broken lock icon instead of Larry
- Use the headline: “Your connection is not secure”
- Use the following copy for the main message: “The owner of expired.badssl.com has configured their website improperly. To protect your information from being stolen, Firefox has not connected to this website.”
- Add a “Learn more…” link that takes the user to the SUMO page: https://support.mozilla.org/en-US/kb/tls-error-reports
- Add a “Return to Previous Page” button that does "history.back()" or goes to "about:home"
- Add an “Advanced” button that reveals technical details about the error that are currently under the "Technical Details" section.
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Priority: P1 → P3
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 44.1 - Oct 5
Priority: P3 → P1
Iteration: 44.1 - Oct 5 → 44.2 - Oct 19
Depends on: 1212456
Attached patch about-cert-error.patch (obsolete) — Splinter Review
Literally just an import of http://brampitoyo.github.io/fx-untrusted-connection/.  Still has hardcoded 'technical details' section, no behavior on the 'report errors' checkbox, and other issues.
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
Attached patch Patch v2 (obsolete) — Splinter Review
I've removed a lot of the redundancies, but I think some of the JS is still not needed. Other things that remain are:
1. figure out why the hostname doesn't appear in bold
2. the Advanced button needs to be wider
3. verify that error reporting works
4. make sure that the override link appears in all the right cases and has the updated style

I'll get back to it in the weekend or Monday morning.
Attachment #8671062 - Attachment is obsolete: true
Attached patch changes-for-aboutcerterror.patch (obsolete) — Splinter Review
This fixes points 1 and 2 from Comment 2 (and is meant to be applied on top of the other patch).  It also adds the beginnings of a simple test that we can hopefully enhance as we go.

I'm starting to get a better idea of how the reporting and other neterror features work and have left a couple of comments in the patch.  I wonder if we can cut scope in this bug by moving reporting and the override link into separate work and keeping the current exception popup (which is at parity at least with the current UI).  If that were the case, I think we can still trim out stuff from the JS on this page that was imported from about:neterror that isn't used here.  I'll leave this patch here, and we can discuss further on Monday if you get a chance to look at it.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Created attachment 8678383 [details] [diff] [review]
> changes-for-aboutcerterror.patch
> 
> This fixes points 1 and 2 from Comment 2 (and is meant to be applied on top
> of the other patch).  It also adds the beginnings of a simple test that we
> can hopefully enhance as we go.
> 
> I'm starting to get a better idea of how the reporting and other neterror
> features work and have left a couple of comments in the patch.  I wonder if
> we can cut scope in this bug by moving reporting and the override link into
> separate work and keeping the current exception popup (which is at parity at
> least with the current UI).  If that were the case, I think we can still
> trim out stuff from the JS on this page that was imported from
> about:neterror that isn't used here.  I'll leave this patch here, and we can
> discuss further on Monday if you get a chance to look at it.

Discussed this further, and the plan is to cut the 'report error' functionality from this bug (which means we don't need as much about:neterror stuff imported into the JS for this page).  Also, we are planning to keep the 'add exception' modal workflow for now instead of the '(not secure) go to expired.badssl.com' link.
Attached patch Patch v4 (obsolete) — Splinter Review
Didn't have much time for this today, but I've folded both patches into one and added a fallback for the "Return to previous page" button to go to about:home in case history is empty.
Attachment #8678259 - Attachment is obsolete: true
Attachment #8678383 - Attachment is obsolete: true
(In reply to Panos Astithas [:past] from comment #5)
> Created attachment 8678964 [details] [diff] [review]
> Patch v4
> 
> Didn't have much time for this today, but I've folded both patches into one
> and added a fallback for the "Return to previous page" button to go to
> about:home in case history is empty.

Alright, I'm going to cut out some functionality from this patch and fix up the test a bit
Separated the platform string changes out into another patch since it's not clear if we want to tackle this for v1
Attached patch about-cert-error-v5.patch (obsolete) — Splinter Review
v5 - as far as I'm going to get today.  This is a much smaller patch since it takes comment 4 into account and goes back to the modal exceptions button, along with taking away the error reporting feature.  Hopefully the diff is easier to read now, too.

I've expanded the test coverage a bit and put in stubs for extra test methods I'd like to have that will cover two special cases I see in the code.

TODO:
1) Should 'learn more' link URL be localizable / stored outside of the HTML somewhere?  Looks like similar pages aren't doing that so maybe not.
2) Expand test coverage - there are two particular things I'd like to check, and we should also have some basic integration testing to make sure that pressing 'advanced' works how you'd expect.  There are already separate tests for the modal exceptions stuff so I think we are good there.
3) Review pass through on HTML / CSS to make sure that the import is clean (i.e. no extra elements / css rules that are unused).
4) Code cleanup.. There are things like the endsWith helper that aren't needed anymore (String.prototype.endsWith)
Attachment #8678964 - Attachment is obsolete: true
I've got a try push as a sanity check for the current WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3c6e4b77a0f
This bug blocks bug 1202488 and 1202489, but severe (non-overridable) errors and RC4 error will not use aboutCertError.xhtml. They will use aboutNetError.xhtml.
Attached patch about-cert-error-v6 (obsolete) — Splinter Review
Fixed item 4 and the failing tests on try.
Attachment #8679131 - Attachment is obsolete: true
Attached image certerror-ui.png
screenshot with current patch applied
If we are not landing attachment 8679125 [details] [diff] [review] yet, then we might want to move it to bug 1207146.
(In reply to Panos Astithas [:past] from comment #14)
> If we are not landing attachment 8679125 [details] [diff] [review] yet, then
> we might want to move it to bug 1207146.

Sounds good to me
Comment on attachment 8679435 [details] [diff] [review]
about-cert-error-v6

Review of attachment 8679435 [details] [diff] [review]:
-----------------------------------------------------------------

Dao, I see you've done reviews for aboutCertError in the past.  Would you be able to review this?  We are hoping to get these UI changes in for 44, so if you are not going to be able to look at it this week please let me know.
Attachment #8679435 - Flags: review?(dao)
Comment on attachment 8679435 [details] [diff] [review]
about-cert-error-v6

>--- a/browser/base/content/aboutcerterror/aboutCertError.xhtml
>+++ b/browser/base/content/aboutcerterror/aboutCertError.xhtml
>@@ -3,36 +3,39 @@
> <!DOCTYPE html [
>   <!ENTITY % htmlDTD
>     PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>     "DTD/xhtml1-strict.dtd">
>   %htmlDTD;
>   <!ENTITY % globalDTD
>     SYSTEM "chrome://global/locale/global.dtd">
>   %globalDTD;
>+  <!ENTITY % netErrorDTD
>+    SYSTEM "chrome://global/locale/netError.dtd">
>+  %netErrorDTD;

What netError.dtd strings are you using here and why?

>-    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/warning-16.png"/>
>+    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/warning-16.png" />

Please avoid this change, the trailing space isn't useful.

>+      function toggleVisibility(id)
>+      {
>+        var node = document.getElementById(id);
>+        var toggle = {
>+          '': 'inherit',
>+          'hidden': 'inherit',
>+          'inherit': 'hidden'
>+        };
>+        node.style.visibility = toggle[node.style.visibility];

'': 'inherit' is wrong, and please use double quotes. How about:

>        node.style.visibility = node.style.visibility == "" ? "hidden" : "";

>+        // Get the hostname and add it to the panel
>+        [...document.querySelectorAll('.hostname')].forEach(host => {
>+          host.textContent = document.location.hostname;
>+        });

for (let host of document.querySelectorAll(".hostname")) {
  host.textContent = document.location.hostname;
}

>+function waitForCertErrorLoad(browser) {
>+  return new Promise(resolve => {
>+    info("Waiting for AboutCertErrorLoad event");
>+    browser.addEventListener("AboutCertErrorLoad", function load() {
>+      browser.removeEventListener("AboutCertErrorLoad", load, false, true);
>+      resolve();
>+    }, false, true);
>+  });
>+}

Can you just use the standard DOMContentLoaded event and poke the document to make sure it's about:certerror?

>+  is(advancedDiv.style.visibility, "hidden", "Advanced content should not be visible by default");

>+  is(advancedDiv.style.visibility, "inherit", "Advanced content should be visible by default");

>+  is(aP.style.visibility, "hidden", "Advanced content should not be visible by default");

use getComputedStyle

> #errorTitle {
>-  -moz-margin-start: 80px;
>+  background: url("chrome://browser/skin/cert-error.svg") left 0 no-repeat;

rtl?

>+  -moz-margin-start: -5em;
>+  -moz-padding-start: 5em;

please use margin-inline-start, padding-inline-start

>+/* Pressing the retry button will cause the cursor to flicker from a pointer to
>+ * not-allowed. Override the disabled cursor behaviour since we will never show
>+ * the button disabled as the initial state. */
>+button:disabled {
>   cursor: pointer;
> }

Can we just remove <http://hg.mozilla.org/mozilla-central/annotate/fc706d376f06/toolkit/themes/shared/in-content/common.inc.css#l213>?

Disabled buttons using the not-allowed cursor is unusual and therefore seems more distracting than useful. 

>+#advancedButton {
>+  float: right;
>+  min-width: 150px;
>+}

This is correct for rtl too?

>+/* Advanced section is hidden via inline styles until the link is clicked */
>+#advancedPanel {
>+  background-color: white;

Should probably specify a text color here rather than counting on the inherited color working on white.

>+  border: 1px lightgray solid;
>+  /* Don't use top padding because the default p style has top padding, and it
>+   * makes the overall div look uneven */
>+  padding: 0 12px 12px 12px;
>+  box-shadow: 0 0 4px #ddd;
>+  font-size: 0.9em;
>+  margin-top: 10px;
>+  padding-bottom: 10px;

padding: 0 12px 10px;
Attachment #8679435 - Flags: review?(dao) → review-
Apparently the merge day is tomorrow, so we will need to land the new strings ASAP if we want to get it in for 44
(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8679435 [details] [diff] [review]
> about-cert-error-v6
> 
> >--- a/browser/base/content/aboutcerterror/aboutCertError.xhtml
> >+++ b/browser/base/content/aboutcerterror/aboutCertError.xhtml
> >@@ -3,36 +3,39 @@
> > <!DOCTYPE html [
> >   <!ENTITY % htmlDTD
> >     PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> >     "DTD/xhtml1-strict.dtd">
> >   %htmlDTD;
> >   <!ENTITY % globalDTD
> >     SYSTEM "chrome://global/locale/global.dtd">
> >   %globalDTD;
> >+  <!ENTITY % netErrorDTD
> >+    SYSTEM "chrome://global/locale/netError.dtd">
> >+  %netErrorDTD;
> 
> What netError.dtd strings are you using here and why?
> 

It's using the errorReporting.learnMore and weakCryptoAdvanced.title but we could add new strings into the cert error dtd for these and remove the reference to netError.dtd
 
> >-    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/warning-16.png"/>
> >+    <link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/warning-16.png" />
> 
> Please avoid this change, the trailing space isn't useful.
> 
> >+      function toggleVisibility(id)
> >+      {
> >+        var node = document.getElementById(id);
> >+        var toggle = {
> >+          '': 'inherit',
> >+          'hidden': 'inherit',
> >+          'inherit': 'hidden'
> >+        };
> >+        node.style.visibility = toggle[node.style.visibility];
> 
> '': 'inherit' is wrong, and please use double quotes. How about:
> 
> >        node.style.visibility = node.style.visibility == "" ? "hidden" : "";
> 

Updated, this was just copied from the mockup but the proposal is better

> >+        // Get the hostname and add it to the panel
> >+        [...document.querySelectorAll('.hostname')].forEach(host => {
> >+          host.textContent = document.location.hostname;
> >+        });
> 
> for (let host of document.querySelectorAll(".hostname")) {
>   host.textContent = document.location.hostname;
> }

Updated

> 
> >+function waitForCertErrorLoad(browser) {
> >+  return new Promise(resolve => {
> >+    info("Waiting for AboutCertErrorLoad event");
> >+    browser.addEventListener("AboutCertErrorLoad", function load() {
> >+      browser.removeEventListener("AboutCertErrorLoad", load, false, true);
> >+      resolve();
> >+    }, false, true);
> >+  });
> >+}
> 
> Can you just use the standard DOMContentLoaded event and poke the document
> to make sure it's about:certerror?

Pretty sure the normal load event don't fire for these error pages, which is why aboutNetError dispatches an AboutNetErrorLoad event.  When I tried to just wait for load in the test it never fired.  I'm not sure about DOMContentLoaded and if the test could listen for that.

> >+  is(advancedDiv.style.visibility, "hidden", "Advanced content should not be visible by default");
> 
> >+  is(advancedDiv.style.visibility, "inherit", "Advanced content should be visible by default");
> 
> >+  is(aP.style.visibility, "hidden", "Advanced content should not be visible by default");
> 
> use getComputedStyle
> 

Switched to is_element_hidden / is_element_visible

> > #errorTitle {
> >-  -moz-margin-start: 80px;
> >+  background: url("chrome://browser/skin/cert-error.svg") left 0 no-repeat;
> 
> rtl?
> 
> >+  -moz-margin-start: -5em;
> >+  -moz-padding-start: 5em;
> 
> please use margin-inline-start, padding-inline-start
> 
> >+/* Pressing the retry button will cause the cursor to flicker from a pointer to
> >+ * not-allowed. Override the disabled cursor behaviour since we will never show
> >+ * the button disabled as the initial state. */
> >+button:disabled {
> >   cursor: pointer;
> > }
> 
> Can we just remove
> <http://hg.mozilla.org/mozilla-central/annotate/fc706d376f06/toolkit/themes/
> shared/in-content/common.inc.css#l213>?
> 
> Disabled buttons using the not-allowed cursor is unusual and therefore seems
> more distracting than useful. 
> 

Fine with me, although this would affect all in content pages and that targets more than just buttons (colorpickers and menulists also).  Since we are attempting to land this late in the cycle maybe it'd be safest to keep it as-is and file a follow up to make that change.

> >+#advancedButton {
> >+  float: right;
> >+  min-width: 150px;
> >+}
> 
> This is correct for rtl too?
> 
> >+/* Advanced section is hidden via inline styles until the link is clicked */
> >+#advancedPanel {
> >+  background-color: white;
> 
> Should probably specify a text color here rather than counting on the
> inherited color working on white.
> 
> >+  border: 1px lightgray solid;
> >+  /* Don't use top padding because the default p style has top padding, and it
> >+   * makes the overall div look uneven */
> >+  padding: 0 12px 12px 12px;
> >+  box-shadow: 0 0 4px #ddd;
> >+  font-size: 0.9em;
> >+  margin-top: 10px;
> >+  padding-bottom: 10px;
> 
> padding: 0 12px 10px;

I'll update the CSS
Bug 1207107 - Strings for aboutCertError design update;r=dao
Attachment #8680298 - Flags: review?(dao)
Bug 1207107 - Design update for aboutCertError;r=dao
Attachment #8680299 - Flags: review?(dao)
Bug 1207107 - Remove old strings for aboutCertError;r=dao
Attachment #8680300 - Flags: review?(dao)
Thanks for the review Dao!  I've updated the patch as you suggested, except where there are other notes.  The first patch is meant to land before the merge tomorrow.  Part 2 could be uplifted, and part 3 (string removal) can land after merge and ride the train.

Also, I believe that we can use DOMContentLoaded instead of the AboutCertErrorLoad event, so I'll update that and push a change.
Comment on attachment 8680299 [details]
MozReview Request: Bug 1207107 - Design update for aboutCertError;r=dao

Bug 1207107 - Design update for aboutCertError;r=dao
Comment on attachment 8680300 [details]
MozReview Request: Bug 1207107 - Remove old strings for aboutCertError;r=dao

Bug 1207107 - Remove old strings for aboutCertError;r=dao
Attachment #8679125 - Attachment is obsolete: true
Comment on attachment 8680298 [details]
MozReview Request: Bug 1207107 - Strings for aboutCertError design update;r=dao

Please add a l10n note for certerror.introPara that explains #1 and the span element. I'm actually not even sure what #1 is good for; can we remove it and just use <span class='hostname'/>?
Attachment #8680298 - Flags: review?(dao) → review+
Attachment #8680300 - Flags: review?(dao) → review+
I landed the string update patch after removing the redundant #1 bit.
Comment on attachment 8680299 [details]
MozReview Request: Bug 1207107 - Design update for aboutCertError;r=dao

(In reply to Brian Grinstead [:bgrins] from comment #19)
> > >+/* Pressing the retry button will cause the cursor to flicker from a pointer to
> > >+ * not-allowed. Override the disabled cursor behaviour since we will never show
> > >+ * the button disabled as the initial state. */
> > >+button:disabled {
> > >   cursor: pointer;
> > > }
> > 
> > Can we just remove
> > <http://hg.mozilla.org/mozilla-central/annotate/fc706d376f06/toolkit/themes/
> > shared/in-content/common.inc.css#l213>?
> > 
> > Disabled buttons using the not-allowed cursor is unusual and therefore seems
> > more distracting than useful. 
> > 
> 
> Fine with me, although this would affect all in content pages and that
> targets more than just buttons (colorpickers and menulists also).  Since we
> are attempting to land this late in the cycle maybe it'd be safest to keep
> it as-is and file a follow up to make that change.

Can you please file the bug and add a comment to the rule with a reference to that bug?

> <button id="returnButton" autocomplete="off" onclick="goBack(this);" autofocus="true" style="margin-left: 0;">&certerror.returnToPreviousPage.label;</button>

What's style="margin-left: 0;" doing here?
Attachment #8680299 - Flags: review?(dao) → review+
Blocks: 1219861
Thanks for the reviews!  I moved the inline margin-left to the CSS file and changed it to margin-inline-start and also filed Bug 1219861 to handle the button styling for in-content pages

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbd2b7eaf70
Keywords: leave-open
Attachment #8679435 - Attachment is obsolete: true
Comment on attachment 8680298 [details]
MozReview Request: Bug 1207107 - Strings for aboutCertError design update;r=dao

Approval Request Comment
[Feature/regressing bug #]: New design for bad certificate error page
[User impact if declined]: The new about:certerror won't ship in 44
[Describe test coverage new/current, TreeHerder]: Tests are in part 2
[Risks and why]: This is just the string change, see part 2
[String/UUID change made/needed]: This is the string change for the feature, so adding new strings to aboutCertError.dtd
Attachment #8680298 - Flags: approval-mozilla-aurora?
Comment on attachment 8680299 [details]
MozReview Request: Bug 1207107 - Design update for aboutCertError;r=dao

Approval Request Comment
[Feature/regressing bug #]: New design for bad certificate error page
[User impact if declined]: The new about:certerror won't ship in 44
[Describe test coverage new/current, TreeHerder]: Added a test for about:certerror
[Risks and why]: It's a new design, so there's a chance for visual regressions.  The strings and design has been through product reviews but the implementation could have bugs.  It's a fairly shallow visual design change and it's not replacing the cert exception workflow so that at least limits the surface area 
[String/UUID change made/needed]: None here, see part 1
Attachment #8680299 - Flags: approval-mozilla-aurora?
Keywords: leave-open
Depends on: 1220080
https://hg.mozilla.org/mozilla-central/rev/511f709bd650
https://hg.mozilla.org/mozilla-central/rev/8e4fa465e2c6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment on attachment 8680298 [details]
MozReview Request: Bug 1207107 - Strings for aboutCertError design update;r=dao

Cool new design, taking it. Should be in the first 44 devedition release.
Attachment #8680298 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8680299 [details]
MozReview Request: Bug 1207107 - Design update for aboutCertError;r=dao

Cool new design, taking it. Should be in the first 44 devedition release.
Attachment #8680299 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: user-doc-needed
https://reviewboard.mozilla.org/r/23609/#review21317

Kinda makes me sad we aren't using the info-pages.css stylesheet that was created for this kind of page...
Depends on: 1220481
Blocks: 1097111
https://reviewboard.mozilla.org/r/23609/#review21321

::: browser/themes/shared/aboutCertError.css:71
(Diff revision 2)
> +  background-color: var(--in-content-primary-button-background-hover) !important;

Additionally, there's already a class name available for blue buttons. It's "primary". You can use it to avoid style duplication.

::: browser/themes/shared/incontent-icons/cert-error.svg:4
(Diff revision 2)
> +<svg version="1.1"

This SVG needs cleanup, I wish we could set up a tool to clean up our SVGs (SVGO seems like a great candidate).
> ::: browser/themes/shared/incontent-icons/cert-error.svg:4
> (Diff revision 2)
> > +<svg version="1.1"
> 
> This SVG needs cleanup, I wish we could set up a tool to clean up our SVGs
> (SVGO seems like a great candidate).

Filed bug 1220483.
QA Contact: paul.silaghi
Depends on: 1220781
Depends on: 1221084
The new UI looks ok, marking verified fixed considering the follow-up bugs.
45.0a1 (2015-11-02), 44.0a2 (2015-11-02) win 7.
Status: RESOLVED → VERIFIED
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Depends on: 1232258
Depends on: 1297630
No longer depends on: 1297630
Blocks: 1324204
Depends on: 1336352
You need to log in before you can comment on or make changes to this bug.