layout of the "secure connection failed" results in poor wrapping of the word "sites"

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glob, Assigned: djmdeveloper060796, Mentored)

Tracking

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
the "secure connection failed" page has a checkbox with the label "report errors like this to help mozilla identify and block malicious sites".

the label is too long for the allocated space and wraps, however only the word "sites" wraps and it looks .. well, odd.

see attached screenshot for example and a proposed layout adjustment.
Mark, thoughts?
Flags: needinfo?(mgoodwin)
I agree with glob that this looks bad. I think Panos has touched this more recently than me.

Panos, what do you think?
Flags: needinfo?(mgoodwin) → needinfo?(past)
We should fix this, but perhaps Nihanth's work in bug 1220481 is already taking care of it?
Flags: needinfo?(past) → needinfo?(nhnt11)
Bram's prototypes have new strings that are much shorter and shouldn't wrap, so yes, that bug should take care of this.
Flags: needinfo?(nhnt11)
OK, after giving this another thought, I'd like to confirm that we actually want the string change. I didn't realize there was a proposed layout change in the screenshot (I only glanced at it).

Whatever we decide, I'll incorporate it into bug 1220481. Looks like that's going to become a "fix the error pages" bug. Can someone set this bug (and maybe bug 1247176 too) as either depending on or blocking bug 1220481? I'm not sure which one is correct in this context, hah.
It sounds like the proposed layout change might not be all that useful with the shorter string, but I'm setting a dependency on bug 1220481 to make sure we retest this once that one lands.
Depends on: 1220481
Reporter

Comment 7

3 years ago
(In reply to Panos Astithas [:past] from comment #6)
> It sounds like the proposed layout change might not be all that useful with
> the shorter string, but I'm setting a dependency on bug 1220481 to make sure
> we retest this once that one lands.

be sure to test it with languages that have longer strings than english :)
I confirmed that the weird wrapping still occurs, although now only in really small window widths. Therefore it's not a significant issue on desktop, but it may be more glaring on mobile if/when they start using these pages. I'll mark this as a good first bug.
Mentor: past
Whiteboard: [good first bug]
Component: Security: UI → General
Product: Core → Firefox
Assignee

Comment 9

3 years ago
I would like to work on this bug.Which files should I look into in order to get started?
The code that you need to look at is here:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#625

You will probably have to tweak the styles loaded from the CSS here:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/aboutNetError.css

You can try your changes at a site that shows the error page, like this one:

https://self-signed.badssl.com/

If you haven't built Firefox from source before, you will want to read the documentation on MDN:

https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions
https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Good luck!
Assignee

Comment 11

3 years ago
Thanks for the links,I will go through them.
Assignee

Comment 12

3 years ago
I modified the aboutNetError.css file so that the text wraps properly.I tested it by opening a site that produces the bug. The word "sites" is now aligned with the previous line.
Attachment #8765056 - Flags: review?(past)
Comment on attachment 8765056 [details] [diff] [review]
added css style for propper text wrapping

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

Nice work! I have a few comments for various improvements and when you address them I can approve it for landing.

::: browser/base/content/aboutNetError.xhtml
@@ +622,5 @@
>        <div id="certificateErrorReporting">
>          <p>
>            <input type="checkbox" id="automaticallyReportInFuture" />
> +          <label for="automaticallyReportInFuture" id="automaticallyReportInFuture">&errorReporting.automatic2;</label> 
> +        </p>  

These changes only serve to introduce trailing whitespace, so please revert them.

::: browser/themes/shared/aboutNetError.css
@@ +130,4 @@
>    display: none;
>  }
>  
> +#certificateErrorReporting label{

No need for this slow rule, just use #automaticallyReportInFuture (with space before the opening curly) instead. Also remove the empty line that follows please.
Attachment #8765056 - Flags: review?(past)
Assignee

Comment 14

3 years ago
I updated my previous patch and incorporated the changes u mentioned.
Attachment #8765548 - Flags: review?(past)
Attachment #8765056 - Attachment is obsolete: true
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Comment on attachment 8765548 [details] [diff] [review]
bug1241746.patch

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

Looks good, thanks!
Attachment #8765548 - Flags: review?(past) → review+

Comment 16

3 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/4db7a29c60dd
Improve the wrapping of the reporting section in about:neterror. r=past

Comment 17

3 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e283d1187054
Fix padding for RTL locales. rs=Pike
Axel correctly pointed out on IRC that padding-left doesn't work properly for RTL locales, so I changed it to -moz-padding-start. We have a wiki page with similar useful CSS tips at https://wiki.mozilla.org/Firefox/CSS_Tips if you want to learn more.
(In reply to Panos Astithas [:past] from comment #18)
> Axel correctly pointed out on IRC that padding-left doesn't work properly
> for RTL locales, so I changed it to -moz-padding-start.

Should be padding-inline-start, -moz-padding-start has been mass-replaced in bug 1111440.

> We have a wiki page
> with similar useful CSS tips at https://wiki.mozilla.org/Firefox/CSS_Tips

I updated this on the wiki page.
Assignee

Comment 20

3 years ago
Thanks for the wiki link. I would love to go through it.

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4db7a29c60dd
https://hg.mozilla.org/mozilla-central/rev/e283d1187054
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
> Should be padding-inline-start, -moz-padding-start has been mass-replaced in
> bug 1111440.

Oops, I was aware of padding-inline-start, but the MDN page still mentions it as experimental and a footnote confused me about whether it was still disabled by default. I've filed bug 1283066 to change this.
You need to log in before you can comment on or make changes to this bug.