Closed Bug 1105704 Opened 5 years ago Closed 5 years ago

Fix UI issues with SSL error reporting

Categories

(Firefox :: Security, defect)

36 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Tracking Status
firefox35 --- unaffected
firefox36 - wontfix
firefox37 - verified
firefox38 - verified

People

(Reporter: phlsa, Assigned: ntim)

References

Details

Attachments

(3 files, 6 obsolete files)

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
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.
Assignee: nobody → mgoodwin
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"
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!
(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?
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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!
(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?
Attachment #8530481 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8532005 - Flags: feedback?(philipp)
(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 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).
Flags: needinfo?(philipp)
Attachment #8532005 - Flags: feedback?(philipp) → feedback-
(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
Attachment #8532005 - Attachment is obsolete: true
Attachment #8535631 - Flags: review?(felipc)
Attachment #8535631 - Flags: feedback?(philipp)
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
Attachment #8535631 - Flags: review?(felipc)
Attachment #8535631 - Flags: review?(dao)
Attachment #8535631 - Flags: review+
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?
Attachment #8535631 - Flags: feedback?(philipp) → feedback+
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.
Attachment #8535631 - Flags: review?(dao)
Attachment #8535631 - Flags: review-
Attachment #8535631 - Flags: feedback?(philipp)
Attachment #8535631 - Flags: feedback+
Attachment #8535631 - Flags: feedback?(philipp)
Duplicate of this bug: 1118925
Attached patch Patch (obsolete) — Splinter Review
Rebased and addressed review comments.
Assignee: mgoodwin → ntim007
Attachment #8535631 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8554278 - Flags: review?(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.
Attachment #8554278 - Flags: review?(dao) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8554278 - Attachment is obsolete: true
Attachment #8555202 - Flags: review?(dao)
Attachment #8555202 - Flags: review?(dao) → review+
Keywords: checkin-needed
Attached patch Patch v3 (obsolete) — Splinter Review
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
Attachment #8555202 - Attachment is obsolete: true
Attachment #8555418 - Flags: review+
Attached patch Patch v3.1Splinter Review
Err, uploaded wrong patch.
Attachment #8555418 - Attachment is obsolete: true
Attachment #8555422 - Flags: review+
(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.
Keywords: checkin-needed
[Tracking Requested - why for this release]: Unpolished style for reporting mechanism.
https://hg.mozilla.org/mozilla-central/rev/ed2ce8c2801f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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.
Flags: qe-verify+
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
Flags: needinfo?(jaws)
Attached patch Patch for betaSplinter Review
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
Attachment #8569447 - Flags: approval-mozilla-beta?
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+
Attachment #8569447 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(ntim007)
(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.
Flags: needinfo?(ntim007)
Great, thanks Tim. Marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.