1247176 - Modernized aboutCertError UI alignment

Status: RESOLVED INVALID

(Firefox :: General, defect, P3)

Description

[Franziskus Kiefer [:franziskus]]

The alignment of the new aboutCertError UI is sometimes a bit off. [1] for example puts the text in errorPageContainer on top of the page. This is probably because it takes the hidden content of advancedPanel into account when calculating its position.

[1] https://www.w3schools.com/

Comment 1

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

It also manifests on https://wrong.host.badssl.com/ if the window is sufficiently small.

Comment 2

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

Tempted to mark it as P4 as it isn't that common or that big of a deal, but setting it to P3 for now. Might be fixed by bug 1220481. Nihanth can you test once you have a working patch there?

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

I'll keep this in mind. Bram's prototypes have this issue too - there is no minimum margin-top on the content. I tried adding a ton of text to the "advanced" box and can reproduce. On those prototypes, this results in the text overlapping the striped thing at the top, which is uglier.

Bram, can you weigh in on this? Should we just set a minimum margin? Or just vertically center the main warning and show the "advanced" box below it? I think it'd be good to fix this in bug 1220481, it's very related.

Comment 4

[Bram Pitoyo [:bram]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)
> Should we just set a minimum margin? Or just
> vertically center the main warning and show the "advanced" box below it?

My original intention was:
* On wide and medium/tablet layouts, vertically center the main warning panel
* On narrow/phone layout, align main warning panel to top, and give it a minimum margin

Unfortunately, I wasn’t able to figure the CSS out on my own. Can you assist?

Comment 5

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

This seems fixed after bug 1220481.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Reopening it because it's not fully fixed yet - Gijs wanted to keep the full fix separate from bug 1220481 and I will be uploading a patch to this bug soon. Marco, I'm reverting the priority/iteration/flags changes too, I assume that's the right thing to do when reopening a bug.

Comment 7

[Marco Mucci [:MarcoM]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #6)
> Reopening it because it's not fully fixed yet - Gijs wanted to keep the full
> fix separate from bug 1220481 and I will be uploading a patch to this bug
> soon. Marco, I'm reverting the priority/iteration/flags changes too, I
> assume that's the right thing to do when reopening a bug.

Hi Nihanth.  Yes, that's perfect.  Thanks very much for the changes.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: MozReview Request: Bug 1247176 - Fix alignment issues in about:certerror. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/49059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49059/

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/49059/#review45869

This patch positions the advanced panel absolutely below the main error detail content. This fixes this bug, and coincidentally bug 1246162 along with it. It also fixes the positioning of the cert error debug info.

I thought these small changes all fit the bug title (vs. splitting the cert error debug info positioning into a different bug). Let me know if you disagree.

::: browser/themes/shared/aboutNetError.css:86
(Diff revision 1)
>     * makes the overall div look uneven */
>    padding: 0 12px 12px 12px;
>    box-shadow: 0 0 4px #ddd;
>    font-size: 0.9em;
>    margin-top: 24px;
> +  margin-bottom: 24px;

Seems like this bottom margin doesn't work for the absolute-positioned element. The reason I added it was because the advanced panel was terminating flush with the bottom of the viewport, which looks bad. I'm not sure what the best way to do this is, any ideas?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8745625 [details]
MozReview Request: Bug 1247176 - Fix alignment issues in about:certerror. r=Gijs

https://reviewboard.mozilla.org/r/49059/#review46083

I can't figure out how to reproduce this as filed, so I don't know how to review this change. I don't see a problem loading either wrong.host.badssl.com or w3schools.com over SSL and looking at the advanced panel. I can see the other bug (the content shifts when unhiding the panel) but I don't see this bug (the content is NOT shifted to the top when the page loads initially and the panel is still hidden - it looks centered to me on current nightly). What issue remains here as described in the summary/comment #0 ?

It feels like you can't really have both (1) centered content that doesn't take hidden stuff into account and (2) non-shifting content if you then add content... and (3) spacing to accommodate absolutely-positioned content when shown - at least, not without adding oodles of JS to dynamically add padding to the non-absolutely-positioned content, or something.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

The reason I uploaded the patch here is because the patch is essentially the fix for the original bug. BUT I guess the effect of not having |position: absolute| on the advanced panel no longer manifests the way described in comment #0, so I'll close this and finish the patch on bug 1246162 instead.