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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:glob ✱]

Attachment: sec-ui-label-wrapping.png

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.

Comment 1

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

Mark, thoughts?

Comment 2

[Mark Goodwin [:mgoodwin]]

I agree with glob that this looks bad. I think Panos has touched this more recently than me.

Panos, what do you think?

Comment 3

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

We should fix this, but perhaps Nihanth's work in bug 1220481 is already taking care of it?

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Bram's prototypes have new strings that are much shorter and shouldn't wrap, so yes, that bug should take care of this.

Comment 5

[(inactive) Nihanth Subramanya [: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.

Comment 6

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

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.

Comment 7

[:glob ✱]

(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 :)

Comment 8

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

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.

Comment 9

[Deepjyoti Mondal]

I would like to work on this bug.Which files should I look into in order to get started?

Comment 10

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

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!

Comment 11

[Deepjyoti Mondal]

Thanks for the links,I will go through them.

Comment 12

[Deepjyoti Mondal]

Attachment: added css style for propper text wrapping

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.

Comment 13

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

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.

Comment 14

[Deepjyoti Mondal]

Attachment: bug1241746.patch

I updated my previous patch and incorporated the changes u mentioned.

Comment 15

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

Comment on attachment 8765548 [details] [diff] [review]
bug1241746.patch

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

Looks good, thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4db7a29c60dd
Improve the wrapping of the reporting section in about:neterror. r=past

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e283d1187054
Fix padding for RTL locales. rs=Pike

Comment 18

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

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.

Comment 19

[Florian Quèze [:florian]]

(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.

Comment 20

[Deepjyoti Mondal]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4db7a29c60dd
https://hg.mozilla.org/mozilla-central/rev/e283d1187054

Comment 22

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

> 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.