1105704 - Fix UI issues with SSL error reporting

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

SSL error reporting has recently landed on Nightly, but the page has some UI issues that should be fixed.

- Font size of links is too big
currently is 20px, should be 1em

- The triangle next to »Report…« is too large
This can be fixed by applying this CSS to the arrow (though there's probably a more elegant way of doing this):
display: inline-block;
transform: translate(0.15em, 0.09em) scale(0.8, 0.5);

- The entire content jumps when opening the error reporting
The rest of the page shouldn't move when the popup opens.


Demo page:
https://pinningtest.appspot.com

Comment 1

[Mark Goodwin [:mgoodwin]]

We also have an issue where "Learn more" takes you to https://support.mozilla.org/kb/certificate-pinning-reports - even if the error is not a pin error. It probably makes sense to address these together.

Comment 2

[Mark Goodwin [:mgoodwin]]

Attachment: bug1105704-ssl-error-report-UI-issues.patch

Applied the CSS fix to the down arrow.

Added some conditional logic to the SUMO link; the generic KB article can live at https://support.mozilla.org/kb/tls-error-reports with the existing document (https://support.mozilla.org/kb/certificate-pinning-reports) only shown in the specific case of a pinning error.

I still need to look at the "jumping"

Comment 3

[:Cykesiopka]

If it isn't too difficult to fix, it would be nice to have the arrow change direction depending on whether the UI is showing or not.

It might be a personal thing, but it's a little bit jarring to have the arrow never change directions - I'm used to arrows in other parts of Firefox and my OS change direction based on whether it's hiding something.

Thanks!

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Cykesiopka from comment #3)
> I'm used to arrows in other parts of Firefox
> and my OS change direction based on whether it's hiding something.

I'm not against this idea (I'm happy either way) but we have other places in the fx ui where this doesn't happen (e.g. the arrow at the end of the awesomebar). Philipp, what's your take on this?

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Mark Goodwin [:mgoodwin] from comment #4)
> (In reply to Cykesiopka from comment #3)
> > I'm used to arrows in other parts of Firefox
> > and my OS change direction based on whether it's hiding something.
> 
> I'm not against this idea (I'm happy either way) but we have other places in
> the fx ui where this doesn't happen (e.g. the arrow at the end of the
> awesomebar). Philipp, what's your take on this?

We generally don't flip arrows for bubbles like these and neither do the platforms (at least Windows and OS X), so I think not flipping it here is the right way to do it.

Comment 6

[Dan Clarke [:overbythere]]

To help alleviate the jumping of the box when clicking on the "Report this error" the following CSS could be added:

#errorPageContainer { position: relative; }
#certificateErrorReportingPanel { position: absolute; top:auto; bottom:auto; margin-top:10px; }

This should leave the box underneath, without moving all of the content. It may require the user to scroll on smaller resolution screens, but not much more than they would have to usually.

Hope this helps!

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: bug1105704-ssl-error-report-UI-issues.patch

(In reply to Dan Clarke [:overbythere] from comment #6)
> Hope this helps!

It does, thanks Dan. Patch updated with your suggestion.

Philipp, does this seem like a suitable fix? If so, who would be a suitable reviewer?

Comment 8

[:Cykesiopka]

(In reply to Mark Goodwin [:mgoodwin] from comment #4)
> I'm not against this idea (I'm happy either way) but we have other places in
> the fx ui where this doesn't happen (e.g. the arrow at the end of the
> awesomebar). Philipp, what's your take on this?

(In reply to Philipp Sackl [:phlsa] Currently behind on reviews & needinfos from comment #5)
> We generally don't flip arrows for bubbles like these and neither do the
> platforms (at least Windows and OS X), so I think not flipping it here is
> the right way to do it.

That sounds fair. Thanks for the consideration!

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8532005 [details] [diff] [review]
bug1105704-ssl-error-report-UI-issues.patch

The new popup behavior works fine with this patch (no jumping), but the font size of the links is still too large (should be 1em).

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: bug1105704-ssl-error-report-UI-issues.patch

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #9)
> Comment on attachment 8532005 [details] [diff] [review]
> the font
> size of the links is still too large (should be 1em).

Fixed

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8535631 [details] [diff] [review]
bug1105704-ssl-error-report-UI-issues.patch

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

I prefer to leave the CSS review to Dão

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8535631 [details] [diff] [review]
bug1105704-ssl-error-report-UI-issues.patch

Thanks Mark, that looks much better!
Once this is reviewed, can we uplift it to wherever the reporting feature landed first?

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8535631 [details] [diff] [review]
bug1105704-ssl-error-report-UI-issues.patch

>+            var learnMoreLink = document.getElementById('learnMoreLink');
>+            // nssFailure2 also gets us other non-overrideable errors. Choose
>+            // a "learn more" link based on description:
>+            if (getDescription().contains('mozilla_pkix_error_key_pinning_failure')) {
>+              learnMoreLink.href = 'https://support.mozilla.org/kb/certificate-pinning-reports';
>+            }

Please use let and double quotes.

> div#certificateErrorReporting a,
> div#certificateErrorReportingPanel a {
>   color: #0095DD;
>+  font-size: 1em;
> }

The underlining problem is font-size: 1.25rem from here:
http://hg.mozilla.org/mozilla-central/annotate/b836016d82b5/toolkit/themes/shared/in-content/common.inc.css#l379

We should remove that.

Also, setting the color here seems redundant.

> span.downArrow {
>   font-size: 0.9em;
>+  display: inline-block;
>+  transform: translate(0.15em, 0.09em) scale(0.8, 0.5);
> }

Please reduce the font-size and use scaleY to squeeze the arrow.

> div#certificateErrorReportingPanel {
>   /* Hidden until the link is clicked */
>   display: none;
>   background-color: 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;
>-  position: relative;
>+  position: absolute;
>   width: 75%;
>   left: 34%;

This looks like it isn't RTL aware...

>   font-size: 0.9em;
>-  top: 8px;
>+  top: auto;
>+  bottom: auto;
>+  margin-top: 10px;
> }

auto should be the initial value for top/bottom and can be omitted.

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch

Rebased and addressed review comments.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8554278 [details] [diff] [review]
Patch

>+div#certificateErrorReportingPanel:-moz-locale-dir(ltr) {
>   left: 34%;
>-  font-size: 0.9em;
>-  top: 8px;
>+}
>+
>+div#certificateErrorReportingPanel:-moz-locale-dir(rtl) {
>+  right: 34%;
> }

-moz-locale-dir doesn't work in HTML documents, as far as I know. I think you need to use -moz-dir.

Comment 17

[Tim Nguyen :ntim]

Attachment: Patch v2

Comment 18

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/d56201330a5b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures. Please verify that this is green on Try before pushing again.
https://hg.mozilla.org/integration/fx-team/rev/e86e11592da1

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1805464&repo=fx-team
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1805465&repo=fx-team

Comment 20

[Tim Nguyen :ntim]

Attachment: Patch v3

So the culprit was using let instead of var, probably because it's block-scoped. This was causing the initPage() function not to work.
Anyways, switched back to var, since the rest of the file also used var. Also did a couple of fixes for rtl.

Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a378f454b52

Comment 21

[Tim Nguyen :ntim]

Attachment: Patch v3.1

Err, uploaded wrong patch.

Comment 22

[Mark Goodwin [:mgoodwin]]

(In reply to Tim Nguyen [:ntim] from comment #20)
> So the culprit was using let instead of var, probably because it's
> block-scoped. This was causing the initPage() function not to work.
> Anyways, switched back to var, since the rest of the file also used var.

Sorry, my fault. I'd addressed comment 13 and not done a try run...

Thanks for picking this up, by the way.

Comment 23

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Unpolished style for reporting mechanism.

Comment 24

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/ed2ce8c2801f

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

These polish issues, while noticeable once pointed out, don't seem critical enough to track. I'm happy to take the fix on Aurora. I will leave it up to Sylvestre to review whether we can take a fix for Beta at this point.

Comment 28

[Tim Nguyen :ntim]

Hi jaws,
Can you help me create a branch patch ? I don't have the branch cloned on my computer.
Can the branch patch also remove this line [0] please ?
Thanks.

[0]: http://hg.mozilla.org/mozilla-central/annotate/b836016d82b5/toolkit/themes/shared/in-content/common.inc.css#l382

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for beta

Approval Request Comment
[Feature/regressing bug #]: polish bug for bug 846489
[User impact if declined]: various UI issues with the SSL error page (see comment #1)
[Describe test coverage new/current, TreeHerder]: on mozilla-central for over a month
[Risks and why]: none expected, straightforward import branch patch, no manual rebasing necessary
[String/UUID change made/needed]: none

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8569447 [details] [diff] [review]
Patch for beta

Fix has been on m-c for a month and it's early enough in Beta to take an UI polish fix. Beta+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b6bec5f7ff7

Comment 32

[Bogdan Maris, Desktop QA]

Attachment: Dotted rectangle stuck on Try Again button

Reproduced the initial issue on Firefox 36 beta 10 and verified that the changes from comment 0 are present in Firefox 37 beta 4 on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32bit. 

I see that on Windows, 'Try again' button is highlighted with the dotted rectangle (see attachment). If I hit tab it will disappear clicking anywhere. Should I log a new issue on this?

Comment 33

[Tim Nguyen :ntim]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #32)
> Created attachment 8575268 [details]
> Dotted rectangle stuck on Try Again button
> 
> Reproduced the initial issue on Firefox 36 beta 10 and verified that the
> changes from comment 0 are present in Firefox 37 beta 4 on Windows 7 64-bit,
> Mac OS X 10.9.5 and Ubuntu 14.04 32bit. 
> 
> I see that on Windows, 'Try again' button is highlighted with the dotted
> rectangle (see attachment). If I hit tab it will disappear clicking
> anywhere. Should I log a new issue on this?

This will be handled by bug 1136645.

Comment 34

[Bogdan Maris, Desktop QA]

Great, thanks Tim. Marking this bug as verified fixed.