1218971 - RC4 error page 'advanced' section floats oddly

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Attachment: rc4-advanced.png

STR:

Open https://rc4.badssl.com/
Click the 'advanced' link
Notice that the advanced info is floating off the right (see screenshot)

Comment 1

[Masatoshi Kimura [:emk]]

I thought the styling will be fixed by bug 1207107. But looks like they didn't think the RC4 error page does not use aboutCertError.xhtml.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> I thought the styling will be fixed by bug 1207107. But looks like they
> didn't think the RC4 error page does not use aboutCertError.xhtml.

Yeah, they are loading different HTML / CSS.  In this case aboutNetError.css will need to be updated.

I'm not sure why it's using left: 34% / right: 0 / width: 75%.  Seems like it should use width: 100% and not set left/right here [0], although I haven't seen designs for this page.

[0] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/aboutNetError.css#121

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Widen the RC4 error page 'advanced' section

I also used the icon from bug 1207107.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8682202 [details] [diff] [review]
Widen the RC4 error page 'advanced' section

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

In general, I don't know if we need to correct this. If we do then we should probably also fix SSLv3 warnings, as well as the certificate error reporting pane [1].

[1] http://i.imgur.com/C2ABKn4.png https://dh512.badssl.com/


On about:neterror we seem to have an older down-arrow next to a link. If we want to sync the design with about:certerror then we can make the pane 100% wide and turn the link into a button. At least the error reporting looks quite strange though when doing that.

::: browser/themes/shared/aboutNetError.css
@@ +58,1 @@
>  }

I think that's a better icon, yes. Why don't we use the same icon for SSLv3?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Huh, I thought this was a legitimate bug, but it appears that we have looked too long on the new design of about:certerror and the old design doesn't make sense anymore. I think Tanvi called the about:neterror panel a "popup" or "drop-down" the other day and I even corrected her :)

Bram, what do you think about Tim's comment 4?

Comment 6

[Bram Pitoyo [:bram]]

It would be great if reporting is is triggered by checking/unchecking the checkbox, without the need to manually click a button that says “Report”. So then, instead of needing to expand a separate area that contains a description, a checkbox and a button, we can replace it with one checkbox that says “Report errors like this to Mozilla”.

Like so: http://imgur.com/vlSjxvj

However, this would require adding a new feature? So it might not be possible to do in time for the next release.

If we turn the “Report this error” link into a button and expand the pane 100%, the pane will look a bit strange because it’s functionally different from the pane under the “Advanced” button. But visually, it looks alright.

So we should go with this solution for now. But we should aim to add a functionality like “Check the checkbox to send a report” in the future.

Thoughts?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Sounds like a good plan to me.

For the followup work, what about the lost description though (it explains precisely what will happen) and the "Learn more" link? Do we feel they don't add much value any more?

Comment 8

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1082849c4a5

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Modernize weak crypto/SSLv3 error page

* Changed the "Advanced" link to a button.
* Implemented "Report errors like this to Mozilla" checkbox.
* Aligned the design of RC4/SSLv3 error pages with about:certerror.

Comment 10

[Masatoshi Kimura [:emk]]

Test links:
SSLv3: https://sslv3.dshield.org/
RC4: https://rc4.badssl.com/
Expired certificate: https://expired.badssl.com/
Non-overridable SSL error: https://dh512.badssl.com/
Non-SSL error: https://test.invalid/
TLS intolerant (maybe fixed in the future): https://adman.you.gr/

Comment 11

[Bram Pitoyo [:bram]]

(In reply to Panos Astithas [:past] from comment #7)
> […] what about the lost description

If we want to explain what happens when the report is sent, we should include the explanation in the checkbox label, like so:

http://imgur.com/EDyMQyu

I think that the reasoning doesn’t need to be any longer than “help Mozilla identify and block malicious sites”, but if we want to say more things, we can make the label a bit longer.

What do you think?


> […] the "Learn more" link?

In my original design (http://brampitoyo.github.io/fx-untrusted-connection/), the “Learn more” link is located immediately under the short explanation.

And the error code + technical information is always located under the “Advanced” button.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8687553 [details] [diff] [review]
Modernize weak crypto/SSLv3 error page

Richard should at least take a look at the error reporting, to make sure it covers all the cases we care about.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Bram Pitoyo [:bram] from comment #11)
> What do you think?

Agreed on both points (explanation + "learn more" link)!

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8687553 [details] [diff] [review]
Modernize weak crypto/SSLv3 error page

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

Thanks for the patch!

Unfortunately I didn't have the time to do a full review today, but I think I have enough comments to not keep you waiting and blocking your progress. The goBack functionality needs to be modified as I mention below and Bram's points in comment 11 should be addressed anyway before we proceed.

::: browser/base/content/aboutNetError.xhtml
@@ +60,5 @@
>          return decodeURIComponent(matches[1]);
>        }
>  
> +      function goBack(buttonEl)
> +      {

I have changed the way the "Go Back" button works in bug 1221084. Can you do the same changes here?

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Modernize weak crypto/SSLv3 error page, v2

* Changed the report checkbox label.
* Moved longdesc into the Advanced panel.
* Reflected changes from bug 1221084.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8688934 [details] [diff] [review]
Modernize weak crypto/SSLv3 error page, v2

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

This looks great, even though I have a bunch of comments below. I believe the next review round should be the last one.

::: browser/base/content/aboutNetError.xhtml
@@ -121,5 @@
> -
> -          if (panel.style.display == "block") {
> -            // send event to trigger telemetry ping
> -            var event = new CustomEvent("AboutNetErrorUIExpanded", {bubbles:true});
> -            document.dispatchEvent(event);

This is useful telemetry data to keep around, but have it trigger on the "Advanced" button from now on.

::: browser/base/content/content.js
@@ +472,5 @@
>    onAboutNetError: function (event, documentURI) {
>      let elmId = event.originalTarget.getAttribute("id");
> +    if (elmId == "returnButton") {
> +      let ownerDoc = event.originalTarget.ownerDocument;
> +      sendAsyncMessage("Browser:CertExceptionError", {

Using the same message as in the about:certerror case may save us from copying some code, but it creates an implicit dependency between the two pages and also affects the Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE telemetry histogram.

I could see an argument for the latter being a good thing if we want that metric to count both interactions, which are now quite similar (and the weak crypto errors could be considered as related to certificate errors). But I would like someone who knows what we want to track with these telemetry measurements tell me that this is indeed the case. Richard?

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +199,5 @@
>  <button id='getMeOutOfHereButton'>&securityOverride.getMeOutOfHereButton;</button>
>  <button id='exceptionDialogButton'>&securityOverride.exceptionButtonLabel;</button>
>  ">
>  
> +<!ENTITY errorReporting.automatic2 "Report errors like this help Mozilla identify and block malicious sites">

Missing "to": "Report errors like this to help Mozilla identify and block malicious sites"

::: browser/themes/shared/aboutNetError.css
@@ +117,5 @@
> +  display: none;
> +}
> +
> +#reportCertificateError,
> +#reportSentMessage {

Nit: merge #certificateErrorReporting and #learnMoreContainer here as well.

@@ +156,5 @@
>    padding: 0 12px 12px 12px;
>    box-shadow: 0 0 4px #ddd;
>    font-size: 0.9em;
>    position: absolute;
> +  width: 100%;

100% makes the panel slightly larger than the sections above it. Just remove the width altogether and let the box be automatically sized.

@@ +161,1 @@
>    margin-top: 10px;

This margin feels too much now. Let's remove it.

@@ +161,5 @@
>    margin-top: 10px;
>  }
>  
> +div#weakCryptoAdvancedPanel {
> +  left: 0;

This entire rule is now redundant and can be removed.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

One of keeler or rbarnes should know about the telemetry question from comment 16: does it make sense to use Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE for both the "Go Back" button in about:certerror and about:neterror or would we want to count them separately? Up to this point there was no "Go Back" in about:neterror, but this patch unifies the design of the two pages (for the weak crypto & sslv3 cases of about:neterror).

Comment 18

[Dana Keeler (she/her) [:keeler]]

The nsISecurityUITelemetry situation isn't perfect, but I think there is some use to having WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE only apply to about:certerror (that way, we can get a rough count of the relative frequency of users adding exceptions vs. clicking that button (although, this doesn't count users who simply navigate elsewhere)).

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Modernize weak crypto/SSLv3 error page, v3

Resolved review comments.

Richard, please review the error reporting part.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: interdiff

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8690366 [details] [diff] [review]
Modernize weak crypto/SSLv3 error page, v3

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

Looks good to me, thanks!

Comment 22

[Masatoshi Kimura [:emk]]

Richard has no activity on Bugzilla since Nov. 15.
https://bugzilla.mozilla.org/page.cgi?id=user_activity.html&action=run&who=rlb%40ipv.sx&from=2015-11-01&to=2015-12-01&group=when
Is he on vacation? How can I go forward?

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Sorry for the delay here, he may be on vacation, I'm not sure. In any case, I wanted him to take a look just to see if we want to add error reporting for more cases than we handle at the moment. This patch however already expands that list, so we can just file separate bugs if Richard determines that we need more. Let's land this.

Comment 24

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1705a63d9614

Comment 25

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58b1a1776af

Comment 26

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37bf13c5669274040dcc5f858186c0a3edcb3713
Bug 1218971 - Modernize weak crypto/SSLv3 error page. r=past

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37bf13c56692

Comment 28

[Paul Silaghi, QA [:pauly]]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> * Implemented "Report errors like this to Mozilla" checkbox.
This is bit confusing. I'm able to check the option, then to uncheck it. What should the user understand when he unchecks the option - that the report is no longer sent? When I check the option, "sending report" appears for ~ 1 second, but I may not notice that. I suggest making the "Report errors like this to Mozilla - report sent" grayed out after checking.

Comment 29

[Masatoshi Kimura [:emk]]

(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> This is bit confusing. I'm able to check the option, then to uncheck it.
> What should the user understand when he unchecks the option - that the
> report is no longer sent?

If the user leaves the option checked, Reports will be automatically sent for future errors. If the user unchecks the option, No report will be automatically sent in the future.

> When I check the option, "sending report" appears
> for ~ 1 second, but I may not notice that. I suggest making the "Report
> errors like this to Mozilla - report sent" grayed out after checking.

Sounds good to me.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> When I check the option, "sending report" appears
> for ~ 1 second, but I may not notice that. I suggest making the "Report
> errors like this to Mozilla - report sent" grayed out after checking.

That will look a lot like a disabled checkbox, which it isn't. At any rate, let's discuss improvements in a followup bug.

Comment 31

[Paul Silaghi, QA [:pauly]]

RC4 error page 'advanced' section is aligned correctly.
Verified fixed FF 45.0a1 (2015-12-04) Win 7, OS X 10.11, Ubuntu 14.04.