Closed Bug 1220481 Opened 9 years ago Closed 8 years ago

Make net/cert/download error pages more responsive

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: ntim, Assigned: nhnt11)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(5 files, 5 obsolete files)

58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
299.93 KB, image/png
Details
      No description provided.
Blocks: 1207107
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug] [fxprivacy] [triage]
Priority: -- → P4
Whiteboard: [good first bug] [fxprivacy] [triage] → [good first bug] [fxprivacy]
Hi, I would like to work on this bug.Please assign this bug to me. And can you please provide more information about the bug.
Flags: needinfo?(mmucci)
(In reply to simplyblue24 from comment #1)
> Hi, I would like to work on this bug.Please assign this bug to me. And can
> you please provide more information about the bug.

Hello, thanks very much for interest on working on this bug.  I'm forwarding a 'needinfo' request to Panos, one of our developers, who will be able to provide information on it.
Flags: needinfo?(mmucci) → needinfo?(past)
Thank you for the offer to help, but I think Bram has already started working on this in bug 1217269. Let's ask him to be sure, but I think this bug will be resolved as a duplicate once that bug is fixed.
Flags: needinfo?(past) → needinfo?(bram)
Hi simplyblue24,

If you look at bug 1217269 comment 9, you’ll see that I’ve had some layout positioning problems on info-pages.css

You’re more than welcome to create GitHub pull request that fixes this problem, on my repository:
https://github.com/brampitoyo/fx-untrusted-connection

Would this help you get started?
Flags: needinfo?(bram)
Blocks: 1216897
At the end of last year, I’ve managed to do this. The file itself is left untouched other than the addition of specific classes and IDs that apply only to error pages.

Not only that, I had also managed to make the stylesheet responsive – but this should probably be resolved in another bug.

References:

http://brampitoyo.github.io/fx-untrusted-connection/responsive-network-error.xhtml
http://brampitoyo.github.io/fx-untrusted-connection/responsive-cert-error.xhtml
http://brampitoyo.github.io/fx-untrusted-connection/responsive-blocked-site.xhtml
Depends on: 1217269
Flags: qe-verify?
Priority: P4 → P2
Let's broaden the scope of this bug to implement the changes Bram has prototyped in bug 1217269. See comment 5 for links to the live mockups.
Summary: Use info-pages.css stylesheet for about:certerror → Make net/cert/download error pages more responsive
Whiteboard: [good first bug] [fxprivacy] → [fxprivacy]
Attached patch WIP (obsolete) — Splinter Review
Phew, this took longer than I'd hoped. Bram, could you apply this patch and test out the error pages and let me know if any changes need to be made?

The pages I've been  frequently using to test are:
- Any random page for server not found
- http://itisatrap.org/firefox/its-a-trap.html for blocked site
- https://expired.badssl.com/ for insecure
- https://www.w3schools.com/ for insecure with a lot of content in the advanced panel

I've played around with the error pages' styling, and tried to match as best as I could the goals outlined in bug 1217269. The final result is not exactly as in Bram's prototypes in some cases, because for example I implemented the the spec in bug 1247176 comment 4, and the blocked site UI is fundamentally a bit different from Bram's prototype (among other things, there's no advanced panel).

I'm not asking for a code review yet with this patch. The styles still need some cleaning up, and I don't think I've covered every possible error page while testing. I've made aboutSocialError.xhtml use the new errorPage.css and "pretend" to be a net error page, but I need to figure out how to properly test it. Also, I haven't changed the favicons yet.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8721583 - Flags: feedback?(bram)
Attached patch Actual WIP (obsolete) — Splinter Review
Forgot to export, sorry. This one is more up to date.
Attachment #8721583 - Attachment is obsolete: true
Attachment #8721583 - Flags: feedback?(bram)
Attachment #8721584 - Flags: feedback?(bram)
(In reply to Nihanth Subramanya [:nhnt11] from comment #8)
> Created attachment 8721584 [details] [diff] [review]
> Actual WIP
> 
> Forgot to export, sorry. This one is more up to date.

Please use info-pages.css and it's class names instead of introducing new ones. It was the original goal of this bug.
Hmm, up till now I wasn't aware that info-page.css referred to an existing file. I seem to have missed bug 1217269 comment 5 :(

Anyway, no big deal, I'll merge the styles into that in the next patch.
Thanks so much for working on this, Nihanth. Let me know when it lands, so I can test it on Nightly (in a couple of days, I bet). As soon as I see it, I’ll give it a go and let you know of my feedback.
Since Nihanth seems to want some feedback before landing this, a try build should help test his changes without having to build Firefox. Bram, you should be able to find the builds for all platforms shortly in this location:

https://archive.mozilla.org/pub/firefox/try-builds/pastithas@mozilla.com-0b916c818d59b5a2adc9bb26e2d87e33e04cd678/
Attached patch Patch (obsolete) — Splinter Review
This patch de-duplicates the styles common with info-pages.css. However, I did not merge styles in aboutCertError.css into info-pages.css (as suggested in bug 1217269 comment 5), for the following reasons:

a) I'm not comfortable adding styles specific to browser/ to a stylesheet in toolkit/.
b) I don't expect the new responsive styles to work out of the box for other info pages. Some of them have non-trivial UI (e.g. list of tabs to restore) and we haven't yet prototyped/specified responsive designs for each of them. Better to contain them to the error pages for now.
c) Each of the three error pages have some styles specific to them. We'd need to add these rules somewhere anyway. Adding styles with just one consumer each to a shared stylesheet didn't seem like the right way to go (the stripes at the top for example).

As a compromise, I created a new file, error-pages.css, which imports info-pages.css, and contains styles common to the three error pages. The individual CSS files now import this file and add their own specific styles. Let me know what you think.

Gijs, requesting review from you because you've reviewed two out of the last three bugs that touched aboutCertError.css. Please redirect if you're not the right reviewer.
Attachment #8721584 - Attachment is obsolete: true
Attachment #8721584 - Flags: feedback?(bram)
Attachment #8722310 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8722310 [details] [diff] [review]
Patch

Forgot to mention that in the latest patch, I fixed some layout bugs and fixed up the debug information section - except that the hover style for the "Copy to Clipboard" button still clashes with the background for the panel.

Bram, could you please specify the :hover styling for the copy to clipboard button?

Also, I pushed to try with the new patch. Builds will be available for testing here: https://archive.mozilla.org/pub/firefox/try-builds/nhnt11@gmail.com-f2247d3285b471cfbef2c6232bd0cafde6d943b9/

Finally, I just realized I still haven't tested about:socialerror, so changing the review flag to f?.
Flags: needinfo?(bram)
Attachment #8722310 - Flags: review?(gijskruitbosch+bugs) → feedback?(gijskruitbosch+bugs)
Comment on attachment 8722310 [details] [diff] [review]
Patch

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

I don't understand the aim of this bug, as it was filed without a description, and your comments skip right into "here's why I made some specific decisions within this patch".

I also don't really understand why we're making the duplication problem here worse, and why we're introducing so much new CSS and new images (without removing any old ones, AFAICT? Why do we need new images?).

IMO we should fix bug 1240594 first.

Even besides all that, it seems this patch uses a lot of hardcoded px values for sizing, where it should be using em.

Another general point here seems to be that you're misusing the in-content CSS variables. It would make more sense to use the fact that we're using variables for a number of things, and to adjust *the value of the variables* for a specific set of pages if we want them to use different colors, rather than writing swathes of new CSS that reuses those CSS variables without caring about their naming/meaning. In some other cases it seems like adding the generic in-content classes to the markup would make the page look as desired without new CSS.

::: browser/base/content/aboutcerterror/aboutCertError.xhtml
@@ +62,4 @@
>            var div = document.getElementById("certificateErrorDebugInformation");
>            div.style.display = "none";
> +
> +          node.style.display = node.style.display == "none" ? "block" : "none";

Why was the comment moved, and what does this addition accomplish? Why was it necessary?

@@ +189,5 @@
>              if (div.style.display == "block") {
>                div.style.display = "none";
>              } else {
>                div.style.display = "block";
> +              div.style.top = document.body.scrollHeight + "px";

What is this doing, if we're just restyling this?

::: browser/themes/shared/aboutCertError.css
@@ +6,5 @@
>  
>  body {
> +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> +                                            #fedc00 33%, #fedc00 66%,
> +                                            #f0d000 66%, #f0d000);

Don't define a background image like this without a foreground color.

::: browser/themes/shared/blockedSite.css
@@ +10,5 @@
> +                                            #9b2e2e 66%, #9b2e2e);
> +  background-size: 64px 32px;
> +  background-repeat: repeat-x;
> +  background-color: #b14646;
> +  color: var(--in-content-category-text-selected);

Don't reuse variables just because they have the right value. A variable for "in-content selected category text" should not be applied to the body of an entire page.

::: browser/themes/shared/error-pages.css
@@ +19,5 @@
> +
> +.return-button {
> +  background-color: var(--in-content-primary-button-background);
> +  border: none;
> +  color: var(--in-content-selected-text);

This and the background color don't match up. Why are we deviating from the rest of the in-content pages anyway?

It probably makes more sense to give this button, whatever it is, the "primary" class so that the general in-content common styles apply to it correctly.

@@ +46,5 @@
> +   * makes the overall div look uneven */
> +  padding: 0 12px 10px;
> +  margin: 10px 0 10px;
> +  box-shadow: 0 0 4px #ddd;
> +  font-size: 0.9em;

It's not clear why this entire container needs a different font size from the rest of the page.

@@ +55,5 @@
> +}
> +
> +/* Due to using position: absolute, adding a bottom margin has no effect,
> +   resulting in ugliness when the panel is large enough to extend beyond
> +   the viewport height. This adds space after the panel to compensate. */

Does this just extend the reach of the scrollbars, so to speak? Why are we positioning the panel such that we need to do this at all?

@@ +79,5 @@
> +  }
> +
> +  button {
> +    padding: .66rem;
> +    min-width: 180px;

It's really confusing to keep combining padding in rem with widths in px, and it likely doesn't work well if the root size is actually variable.

@@ +84,5 @@
> +    width: auto;
> +    border: none;
> +    padding: .85rem;
> +    background-color: #e0e2e5;
> +    font-size: 15px;

Don't do this. It's set on the root anyway.

@@ +105,5 @@
> +
> +  body {
> +    padding: 75px 20px 0;
> +    height: 100%;
> +    font-size: 14px;

We should really stop using px-sized fonts on these in-content pages, especially if we're doing a restyle of them. Whatever we do, we shouldn't be making it worse.

@@ +120,5 @@
> +    width: 100%;
> +    border: none;
> +    padding: 1rem;
> +    background-color: #e0e2e5;
> +    font-size: 16px;

In fact, this makes no sense, because now the body size is smaller (max-width is smaller than the previous media query) but the font size increases? That can't be right...

@@ +121,5 @@
> +    border: none;
> +    padding: 1rem;
> +    background-color: #e0e2e5;
> +    font-size: 16px;
> +    font-weight: 300;

Why do we need this? We generally don't change font-weights unless we want text to be bold, and this feels like you're just trying to get whatever rendering you see in some mock (where is that mock?) to match up exactly with what OS X or something is doing. Don't do that.

::: toolkit/themes/shared/in-content/info-pages.inc.css
@@ +12,5 @@
>    min-height: 100vh;
> +  /* Top padding for when the window height is small.
> +     Bottom padding to keep everything centered. */
> +  padding-top: 75px;
> +  padding-bottom: 75px;

This comment is confusing. Why reduce the space available for content when the window height is small? This isn't in a media query either, so it applies when the window height isn't small, too.

@@ +20,5 @@
>    justify-content: center;
>  }
>  
>  .container {
> +  position: relative;

This is scary because it applies to a whole host of other pages rather than just error pages. Why is it in this file, and why do we need it at all?

::: toolkit/themes/shared/incontent-icons/blocked.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="45" height="45" viewBox="0 0 45 45">

This SVG file has issues. Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines .

More generally, I do not understand why we need it. What's wrong with whatever image we're currently using and/or why isn't that being removed?
Attachment #8722310 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
With the alternative approach of getting the pages to look right with the existing CSS and variables, it would also make more sense to split this patch in 3 parts, one for each page, which will make it a lot easier to review. Though I'd still prefer if we fix bug 1240594 first.
I am partly responsible for the lack of context in this bug, since comment 6 where I renamed it wasn't terribly informative. The plan here is to implement the design from bug 1217269, so that Android (and perhaps iOS or B2G) can reuse the error pages and spare us the effort to implement any changes in every product each time. From that perspective, I'm not sure if fixing bug 1240594 first is actually better, as it's goal is orthogonal to this one and it would require someone from the Android team to do the same there before they could reap the benefits of this patch.
(In reply to Panos Astithas [:past] from comment #17)
> I am partly responsible for the lack of context in this bug, since comment 6
> where I renamed it wasn't terribly informative. The plan here is to
> implement the design from bug 1217269, so that Android (and perhaps iOS or
> B2G) can reuse the error pages and spare us the effort to implement any
> changes in every product each time. From that perspective, I'm not sure if
> fixing bug 1240594 first is actually better, as it's goal is orthogonal to
> this one and it would require someone from the Android team to do the same
> there before they could reap the benefits of this patch.

This patch doesn't modify Android, though, and the added CSS files are in browser/ rather than toolkit/, so I don't see how that would benefit android in any way.
I expect the Android team to write the patch that removes their error pages and reuses these responsive ones. I don't have a preference on which patch should move the necessary files from browser/ to toolkit/ however.
Gijs, first of all, sorry for throwing the review at you without providing context, my bad.

The patch is the implementation of the design prototypes given by Bram in comment 5. These designs were worked on in bug 1217269 as Panos mentioned above.

There has been some confusion about the use of the existing info-pages.css stylesheet we have in toolkit. There was talk about merging the styles for the error pages into that, which I was extremely hesitant to do since that affects quite a few more pages as you mentioned (I should have added another rule for the relative positioning of the container in error-pages.css, yeah).

Anyway, I lifted the CSS rules for colours and fonts directly from Bram's prototypes. I'm not very experienced/familiar with writing CSS rules for these, so I just used them as is.

I'll respond to a few of your comments now, and the rest in future patches.

(In reply to :Gijs Kruitbosch from comment #15)
> I also don't really understand why we're making the duplication problem here
> worse,

Are we? As I see it, this patch de-duplicates a bunch of stuff shared by about:neterror and about:certerror (and puts everything in error-pages.css), which should possibly make bug 1240594 slightly easier.

> and why we're introducing so much new CSS and new images (without
> removing any old ones, AFAICT? Why do we need new images?).

Only one new image was added by the patch - the "blocked" icon visible next to the main heading here: https://brampitoyo.github.io/fx-untrusted-connection/responsive-blocked-site.xhtml

As for removing images - maybe we can remove the image that the new icon is replacing. I believe it's in toolkit/ though, and is used in netError.css (which I assume might be used in Thunderbird for example, though I haven't checked).

> ::: browser/base/content/aboutcerterror/aboutCertError.xhtml
> @@ +62,4 @@
> >            var div = document.getElementById("certificateErrorDebugInformation");
> >            div.style.display = "none";
> > +
> > +          node.style.display = node.style.display == "none" ? "block" : "none";
> 
> Why was the comment moved, and what does this addition accomplish? Why was
> it necessary?

Bah, forgot to add a comment for this. It's for making the advanced panel not affect the layout of the page when it's not visible. I set the positioning to absolute to fix bug 1247176, and display: none so that the page doesn't scroll before the panel is shown for the case when the panel has a lot of content. I'll clean this up in the next patch.

> @@ +189,5 @@
> >              if (div.style.display == "block") {
> >                div.style.display = "none";
> >              } else {
> >                div.style.display = "block";
> > +              div.style.top = document.body.scrollHeight + "px";
> 
> What is this doing, if we're just restyling this?

Forgot to add a comment for this too. This puts the debug info after the current contents of the page. This was the original intention for |top: 100%| in the current tree, but if the advanced panel is large enough for the page to scroll, 100% will not be sufficient.

> ::: browser/themes/shared/aboutCertError.css
> @@ +6,5 @@
> >  
> >  body {
> > +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> > +                                            #fedc00 33%, #fedc00 66%,
> > +                                            #f0d000 66%, #f0d000);
> 
> Don't define a background image like this without a foreground color.

This just adds the stripes at the top, I don't understand what it has to do with the foreground color.

> ::: browser/themes/shared/blockedSite.css
> @@ +10,5 @@
> > +                                            #9b2e2e 66%, #9b2e2e);
> > +  background-size: 64px 32px;
> > +  background-repeat: repeat-x;
> > +  background-color: #b14646;
> > +  color: var(--in-content-category-text-selected);
> 
> Don't reuse variables just because they have the right value. A variable for
> "in-content selected category text" should not be applied to the body of an
> entire page.

These are directly from the prototype. Should I replace them with hard-coded equivalent values?

> @@ +46,5 @@
> > +   * makes the overall div look uneven */
> > +  padding: 0 12px 10px;
> > +  margin: 10px 0 10px;
> > +  box-shadow: 0 0 4px #ddd;
> > +  font-size: 0.9em;
> 
> It's not clear why this entire container needs a different font size from
> the rest of the page.

*Shrug* this was the spec I was given.

> @@ +55,5 @@
> > +}
> > +
> > +/* Due to using position: absolute, adding a bottom margin has no effect,
> > +   resulting in ugliness when the panel is large enough to extend beyond
> > +   the viewport height. This adds space after the panel to compensate. */
> 
> Does this just extend the reach of the scrollbars, so to speak? Why are we
> positioning the panel such that we need to do this at all?

Yes. The reason for the absolute positioning is so that the rest of the contents of the page don't jump up when the panel is added.
The contents are vertically center aligned, so showing the panel results in everything getting re-centered, i.e. the rest of the page jumps up.

> @@ +120,5 @@
> > +    width: 100%;
> > +    border: none;
> > +    padding: 1rem;
> > +    background-color: #e0e2e5;
> > +    font-size: 16px;
> 
> In fact, this makes no sense, because now the body size is smaller
> (max-width is smaller than the previous media query) but the font size
> increases? That can't be right...

I'll ask Bram about this.

> @@ +121,5 @@
> > +    border: none;
> > +    padding: 1rem;
> > +    background-color: #e0e2e5;
> > +    font-size: 16px;
> > +    font-weight: 300;
> 
> Why do we need this? We generally don't change font-weights unless we want
> text to be bold, and this feels like you're just trying to get whatever
> rendering you see in some mock (where is that mock?) to match up exactly
> with what OS X or something is doing. Don't do that.

Again, I'll ask Bram.

> ::: toolkit/themes/shared/in-content/info-pages.inc.css
> @@ +12,5 @@
> >    min-height: 100vh;
> > +  /* Top padding for when the window height is small.
> > +     Bottom padding to keep everything centered. */
> > +  padding-top: 75px;
> > +  padding-bottom: 75px;
> 
> This comment is confusing. Why reduce the space available for content when
> the window height is small? This isn't in a media query either, so it
> applies when the window height isn't small, too.

It adds a minimum padding so that the page content doesn't overlap the background stripes at the top. Silly of me to add it in info-pages.inc.css though, I'll move it to error-pages.css.

> @@ +20,5 @@
> >    justify-content: center;
> >  }
> >  
> >  .container {
> > +  position: relative;
> 
> This is scary because it applies to a whole host of other pages rather than
> just error pages. Why is it in this file, and why do we need it at all?

This is for the advanced panel positioning. Bad idea to put it in this file, yeah. I'll move it to error-pages.css.

> ::: toolkit/themes/shared/incontent-icons/blocked.svg
> @@ +1,3 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="45" height="45" viewBox="0 0 45 45">
> 
> This SVG file has issues. Please see
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines .
> 
> More generally, I do not understand why we need it. What's wrong with
> whatever image we're currently using and/or why isn't that being removed?

The prototype introduced this image. If we want to remove the existing one from toolkit/, I've got no problems with doing so, doesn't seem like the new icon would break anything that still uses the old netError.css. (netError.css is what the blocked site page currently uses, try visiting http://itisatrap.org/firefox/its-a-trap.html).


Thanks for the review!
Attached patch Patch v2 (obsolete) — Splinter Review
Okay, I discussed all of this with Bram and have a new patch with some changes.

First of all, let me address a couple of your comments:


> @@ +46,5 @@
> > +   * makes the overall div look uneven */
> > +  padding: 0 12px 10px;
> > +  margin: 10px 0 10px;
> > +  box-shadow: 0 0 4px #ddd;
> > +  font-size: 0.9em;
> 
> It's not clear why this entire container needs a different font size from
> the rest of the page.

This is how the advanced panel has always been styled since it was added, I haven't touched it.

> @@ +79,5 @@
> > +  }
> > +
> > +  button {
> > +    padding: .66rem;
> > +    min-width: 180px;
> 
> It's really confusing to keep combining padding in rem with widths in px,
> and it likely doesn't work well if the root size is actually variable.

I've tried to change everything to use em, and removed unnecessary width rules.

> @@ +105,5 @@
> > +
> > +  body {
> > +    padding: 75px 20px 0;
> > +    height: 100%;
> > +    font-size: 14px;
> 
> We should really stop using px-sized fonts on these in-content pages,
> especially if we're doing a restyle of them. Whatever we do, we shouldn't be
> making it worse.

I've removed all the font size rules. I've changed some of the rules for small screen widths to more closely match Android's netError.css, which is the intention of these responsive designs anyway.

> @@ +121,5 @@
> > +    border: none;
> > +    padding: 1rem;
> > +    background-color: #e0e2e5;
> > +    font-size: 16px;
> > +    font-weight: 300;
> 
> Why do we need this? We generally don't change font-weights unless we want
> text to be bold, and this feels like you're just trying to get whatever
> rendering you see in some mock (where is that mock?) to match up exactly
> with what OS X or something is doing. Don't do that.

This mimics what Android is doing in netError.css. I've kept it around assuming that this clarification will change your mind. If you still feel strongly, let me know.

-------------------

Aside from all this, this patch modifies the SVG images that are used in these pages. The icons for the other info pages have whitespace around the actual image, and this patch adds negative x and y values to the viewboxes of the SVGs to match them. Bram and I settled on this hack after trying to add the margins the "proper" way - if someone is willing to add the margins properly and supply new images, I will of course use them. We decided not to use CSS to do it in order to avoid introducing new error-page-specific rules.

Also, it puts the "Try Again" button in aboutNetError.xhtml into its own button-container div, and accordingly modifies some of the JS code that shows/hides it.

Finally, after looking at Android's netError.css I have moved the rules for the body (and also changed some margin/padding values) with the intention of making things as parallel to that file as possible to make a future merge easy.

Please let me know if there's any more context you're missing, or would like to discuss anything in this patch. Maybe it would help to talk on IRC.

I want to note that I still haven't updated the favicons or tested about:socialerror.

PS. I haven't split the patch in this iteration, because it's late and Bugzilla lets you view changes by file anyway. I'll split them for the next iteration if you wish.
Attachment #8722310 - Attachment is obsolete: true
Flags: needinfo?(bram)
Attachment #8723391 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8723391 [details] [diff] [review]
Patch v2

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

::: browser/themes/shared/blockedSite.css
@@ +10,5 @@
> +
> +#whyForbiddenButton,
> +#reportButton {
> +  background-color: transparent;
> +  color: var(--in-content-category-text-selected);

Please use #fff instead. The --in-content-category-text-selected variable isn't appropriate for this situation.

@@ +39,5 @@
> +#ignoreWarningButton {
> +  -moz-appearance: none;
> +  background: transparent;
> +  border: none;
> +  color: var(--in-content-category-text-selected);

Same here, the variable is unappropriate for the situation

::: browser/themes/shared/error-pages.css
@@ +17,5 @@
> +
> +body.certerror {
> +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> +                                            #fedc00 33%, #fedc00 66%,
> +                                            #f0d000 66%, #f0d000);

I'd prefer moving this specific rule into the appropriate stylesheet.
Not sure what Gijs thinks.

@@ +20,5 @@
> +                                            #fedc00 33%, #fedc00 66%,
> +                                            #f0d000 66%, #f0d000);
> +}
> +
> +body.blockedsite {

Same here.

@@ +92,5 @@
> +  button {
> +    border: none;
> +    padding: 1rem;
> +    background-color: #e0e2e5;
> +    font-weight: 300;

It seems like this is trying to achieve some different styles for Android. It doesn't seem in the right place though, a screen width lower than 959px doesn't necessarily mean Android.

Also, it's not explicit what this rule is doing at first sight (especially why the background/borders/font-weight need to be changed).

@@ +93,5 @@
> +    border: none;
> +    padding: 1rem;
> +    background-color: #e0e2e5;
> +    font-weight: 300;
> +    background-image: none;

I'm pretty sure this has no effect.

::: toolkit/themes/shared/in-content/info-pages.inc.css
@@ +47,1 @@
>  .title-text {

Please only use one of those 2 selectors (if you choose the first one, you need to remove the .title-text class from other files)
Comment on attachment 8723391 [details] [diff] [review]
Patch v2

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

This would be much easier to review if it didn't split by file but if you used a genuine hg copy from the files you're moving styles out into the error-pages.css stylesheet. That would also preserve blame. You could do this one page at a time, making it much clearer where particular styles were coming from or why they are necessary. So basically, cset 1 would be moving things from aboutNetError.css into error-pages.css, and preserving how it currently looks; cset 2 would move things from the other two files (which presumably at that point is a smaller change) and cset 3 would make the actual style changes this patch is trying to make. The patch should be split up conceptually, not by file. You can use hg commit --interactive and/or hg histedit and/or git rebase --interactive to do this.

Even better if you push to mozreview instead of using splinter, which has no functional interdiffs or code move detection.

This seems to mean we will no longer use neterror.css, which is theoretically different on all 3 OSes (but in practice seems to be almost identical...). What OSes did you test this styling on?

I also agree that it's a bit confusing that this patch:
- tries to style things to be useful for android
- doesn't actually modify android
- does have that styling apply to "narrow" desktop places

Note also that the media query applies to windows, not screens (the "only screen" is a little misleading and is about media type, i.e. as opposed to also applying if you printed the page). So if I resize my window to make it narrow, the android-focused styles apply. I don't think that's a good thing.

::: browser/base/content/aboutNetError.xhtml
@@ +522,5 @@
>            <label for="automaticallyReportInFuture" id="automaticallyReportInFuture">&errorReporting.automatic2;</label>
>          </p>
>        </div>
>  
> +      <div id="weakCryptoAdvancedPanel" class="advanced-panel">

When I talk about duplication I mean that we're making identical modifications here and in the other .xhtml files. This is unfortunate; it would make more sense to unify the files.

::: browser/themes/shared/error-pages.css
@@ +6,5 @@
> +
> +body {
> +  background-image: linear-gradient(-45deg, #eeeeee,     #eeeeee 33%,
> +                                            #fbfbfb 33%, #fbfbfb 66%,
> +                                            #eeeeee 66%, #eeeeee);

The rule of thumb for web content is that whenever you specify a background image or color, you specify a foreground color. This is because if you do not specify anything, we will use the default color. That is usually black, but not always. So if you specify an almost-white background color as you do here, and no foreground color, it'll work for you (default foreground color being black), but not for someone with an inverted OS theme, like the dark themes on ubuntu, or custom classic themes on Windows, etc. Those people will get white-on-white which is unreadable.

@@ +17,5 @@
> +
> +body.certerror {
> +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> +                                            #fedc00 33%, #fedc00 66%,
> +                                            #f0d000 66%, #f0d000);

I concur.

::: browser/themes/shared/incontent-icons/cert-error.svg
@@ +2,5 @@
>  
>  <svg version="1.1"
>       xmlns="http://www.w3.org/2000/svg"
>       xmlns:xlink="http://www.w3.org/1999/xlink"
> +     viewBox="-7.5 -7.5 60 60">

I would expect this to increase the net size of the image and thereby decrease the size of the visible icon. Is there no size set for the places where we use this as a background image?

It's also confusing that this adds "margin" around the 45x45 image in an uneven manner, as the top-left corner moves by 7.5 pixels in both the X and Y direction, and the bottom right corner moves by 15 pixels. Why?

We use info.svg in more places than the in-content pages, so ultimately this just seems like the wrong way of fixing this problem (and it seems likely you're regressing the URL bar search suggestion prompt, which uses info.svg) . Fix the pages that include the image to have the correct margin, and don't hack the image like this.
Attachment #8723391 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to Nihanth Subramanya [:nhnt11] from comment #20)
> > ::: browser/base/content/aboutcerterror/aboutCertError.xhtml
> > @@ +62,4 @@
> > >            var div = document.getElementById("certificateErrorDebugInformation");
> > >            div.style.display = "none";
> > > +
> > > +          node.style.display = node.style.display == "none" ? "block" : "none";
> > 
> > Why was the comment moved, and what does this addition accomplish? Why was
> > it necessary?
> 
> Bah, forgot to add a comment for this. It's for making the advanced panel
> not affect the layout of the page when it's not visible. I set the
> positioning to absolute to fix bug 1247176, and display: none so that the
> page doesn't scroll before the panel is shown for the case when the panel
> has a lot of content. I'll clean this up in the next patch.

Can we move this to a separate patch on bug 1247176?

> > ::: browser/themes/shared/aboutCertError.css
> > @@ +6,5 @@
> > >  
> > >  body {
> > > +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> > > +                                            #fedc00 33%, #fedc00 66%,
> > > +                                            #f0d000 66%, #f0d000);
> > 
> > Don't define a background image like this without a foreground color.
> 
> This just adds the stripes at the top, I don't understand what it has to do
> with the foreground color.

I explained my earlier comment in my review, but if this is "just the stripes at the top", there needs to be a comment. When reading the code, it's not clear that this is limited to the top - it applies to the entire body. Is there some other container with a background that stops the stripes from existing elsewhere? How does this work with the advanced panel whose appearance can "stretch" the length of the page, but is absolute and therefore might stretch the body but not the container which is obscuring this stripy background ?

> > ::: browser/themes/shared/blockedSite.css
> > @@ +10,5 @@
> > > +                                            #9b2e2e 66%, #9b2e2e);
> > > +  background-size: 64px 32px;
> > > +  background-repeat: repeat-x;
> > > +  background-color: #b14646;
> > > +  color: var(--in-content-category-text-selected);
> > 
> > Don't reuse variables just because they have the right value. A variable for
> > "in-content selected category text" should not be applied to the body of an
> > entire page.
> 
> These are directly from the prototype. Should I replace them with hard-coded
> equivalent values?

Either that (which is what ntim suggested) or pick a variable that does make semantic sense.

> > ::: toolkit/themes/shared/in-content/info-pages.inc.css
> > @@ +12,5 @@
> > >    min-height: 100vh;
> > > +  /* Top padding for when the window height is small.
> > > +     Bottom padding to keep everything centered. */
> > > +  padding-top: 75px;
> > > +  padding-bottom: 75px;
> > 
> > This comment is confusing. Why reduce the space available for content when
> > the window height is small? This isn't in a media query either, so it
> > applies when the window height isn't small, too.
> 
> It adds a minimum padding so that the page content doesn't overlap the
> background stripes at the top. Silly of me to add it in info-pages.inc.css
> though, I'll move it to error-pages.css.

There's padding on the bottom, too, though, not just the top? Does it need to be 75px high even on smaller window heights? Seems like we lose a lot of space to these stripes which are purely decorative and don't actually tell the user anything, so if the error page appears in an iframe it will be very hard to read/navigate.
(In reply to :Gijs Kruitbosch from comment #23)
> This seems to mean we will no longer use neterror.css, which is
> theoretically different on all 3 OSes (but in practice seems to be almost
> identical...). What OSes did you test this styling on?

I tested it on OS X only, how does this matter for in-content pages?

> I also agree that it's a bit confusing that this patch:
> - tries to style things to be useful for android
> - doesn't actually modify android

All I can offer as a response is that this attempts to make the desktop UI look more like Android, and that it attempts to make a future merging of styles easier.

> - does have that styling apply to "narrow" desktop places
> 
> Note also that the media query applies to windows, not screens (the "only
> screen" is a little misleading and is about media type, i.e. as opposed to
> also applying if you printed the page). So if I resize my window to make it
> narrow, the android-focused styles apply. I don't think that's a good thing.

Please see my next comment.

> ::: browser/base/content/aboutNetError.xhtml
> @@ +522,5 @@
> >            <label for="automaticallyReportInFuture" id="automaticallyReportInFuture">&errorReporting.automatic2;</label>
> >          </p>
> >        </div>
> >  
> > +      <div id="weakCryptoAdvancedPanel" class="advanced-panel">
> 
> When I talk about duplication I mean that we're making identical
> modifications here and in the other .xhtml files. This is unfortunate; it
> would make more sense to unify the files.

Unifying these files is likely not trivial, and in any case, I would like to proceed with this bug considering it has already made a fair bit of progress. I don't think the introduced duplication warrants putting this on hold and unifying the files first, what do you think?

> ::: browser/themes/shared/error-pages.css
> @@ +6,5 @@
> > +
> > +body {
> > +  background-image: linear-gradient(-45deg, #eeeeee,     #eeeeee 33%,
> > +                                            #fbfbfb 33%, #fbfbfb 66%,
> > +                                            #eeeeee 66%, #eeeeee);
> 
> The rule of thumb for web content is that whenever you specify a background
> image or color, you specify a foreground color. This is because if you do
> not specify anything, we will use the default color. That is usually black,
> but not always. So if you specify an almost-white background color as you do
> here, and no foreground color, it'll work for you (default foreground color
> being black), but not for someone with an inverted OS theme, like the dark
> themes on ubuntu, or custom classic themes on Windows, etc. Those people
> will get white-on-white which is unreadable.

The background and foreground colours are specified in common.css (shared by in-content pages). This is only adding the stripes at the top. A background height of 32px is specified immediately after the background-image. I don't see why we need to re-specify the foreground color here.

> ::: browser/themes/shared/incontent-icons/cert-error.svg
> @@ +2,5 @@
> >  
> >  <svg version="1.1"
> >       xmlns="http://www.w3.org/2000/svg"
> >       xmlns:xlink="http://www.w3.org/1999/xlink"
> > +     viewBox="-7.5 -7.5 60 60">
> 
> I would expect this to increase the net size of the image and thereby
> decrease the size of the visible icon. Is there no size set for the places
> where we use this as a background image?

Yes, the size is set, see https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/toolkit/themes/shared/in-content/info-pages.inc.css#37.

> It's also confusing that this adds "margin" around the 45x45 image in an
> uneven manner, as the top-left corner moves by 7.5 pixels in both the X and
> Y direction, and the bottom right corner moves by 15 pixels. Why?

The last two numbers are width and height. If we add a 7.5px margin on all sides, the width and height increase by a total of 15px each. Hence 45->60.

> We use info.svg in more places than the in-content pages, so ultimately this
> just seems like the wrong way of fixing this problem (and it seems likely
> you're regressing the URL bar search suggestion prompt, which uses info.svg)
> . Fix the pages that include the image to have the correct margin, and don't
> hack the image like this.

No, we don't. As far as I can see info.svg is only used in the in-content pages. https://dxr.mozilla.org/mozilla-central/search?q=chrome%3A%2F%2Fglobal%2Fskin%2Ficons%2Finfo.svg&redirect=false&case=false

The URL bar uses a different info.svg found in browser/.

Fixing the other pages would mean redoing other icons in order to *remove* the margins in them. Please compare chrome://browser/skin/session-restore.svg and chrome://global/skin/icons/info.svg. The way I see it, we either remove margins from those or add margins to these ones, and since this bug is about changing error pages to use in-content styles, I vote for adding margins to info.svg and cert-error.svg. As for hacking the image - I'll ask Bram in the next comment.

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #20)
> > > ::: browser/base/content/aboutcerterror/aboutCertError.xhtml
> > > @@ +62,4 @@
> > > >            var div = document.getElementById("certificateErrorDebugInformation");
> > > >            div.style.display = "none";
> > > > +
> > > > +          node.style.display = node.style.display == "none" ? "block" : "none";
> > > 
> 
> [...]
> 
> Can we move this to a separate patch on bug 1247176?

OK.

> > > ::: browser/themes/shared/aboutCertError.css
> > > @@ +6,5 @@
> > > >  
> > > >  body {
> > > > +  background-image: linear-gradient(-45deg, #f0d000,     #f0d000 33%,
> > > > +                                            #fedc00 33%, #fedc00 66%,
> > > > +                                            #f0d000 66%, #f0d000);
> > > 
> 
> [...]
> 
> I explained my earlier comment in my review, but if this is "just the
> stripes at the top", there needs to be a comment. When reading the code,
> it's not clear that this is limited to the top - it applies to the entire
> body. Is there some other container with a background that stops the stripes
> from existing elsewhere? How does this work with the advanced panel whose
> appearance can "stretch" the length of the page, but is absolute and
> therefore might stretch the body but not the container which is obscuring
> this stripy background ?

The image height is limited to 32px by the background-size rule right below the background-image. I'll add a comment.

> > > ::: toolkit/themes/shared/in-content/info-pages.inc.css
> > > @@ +12,5 @@
> > > >    min-height: 100vh;
> > > > +  /* Top padding for when the window height is small.
> > > > +     Bottom padding to keep everything centered. */
> > > > +  padding-top: 75px;
> > > > +  padding-bottom: 75px;
> > > 
> > > This comment is confusing. Why reduce the space available for content when
> > > the window height is small? This isn't in a media query either, so it
> > > applies when the window height isn't small, too.
> > 
> > It adds a minimum padding so that the page content doesn't overlap the
> > background stripes at the top. Silly of me to add it in info-pages.inc.css
> > though, I'll move it to error-pages.css.
> 
> There's padding on the bottom, too, though, not just the top? Does it need
> to be 75px high even on smaller window heights?

The prototype specified a padding of 12.5vh for window heights below 600px. For a 600px window, 12.5vh == 75px, which is how I got the value.
I changed it to a fixed 75px instead of 12.5vh, because a dynamic padding allows the page content to overlap the stripes at the top.
Because the body is a flexbox with the content vertically center-aligned, adding a top padding offsets the vertical alignment. The bottom padding counters that and restores the alignment.
> Note also that the media query applies to windows, not screens (the "only
> screen" is a little misleading and is about media type, i.e. as opposed to
> also applying if you printed the page). So if I resize my window to make it
> narrow, the android-focused styles apply. I don't think that's a good thing.

I figured this is part of the "responsive design" stuff. Bram, can you comment on this? Do we not want the UI to change when the window width is narrow?

> [...] Does [the padding-top for the body] need
> to be 75px high even on smaller window heights? Seems like we lose a lot of
> space to these stripes which are purely decorative and don't actually tell
> the user anything, so if the error page appears in an iframe it will be very
> hard to read/navigate.

I didn't think of the iframe case. Bram, what do you think about this?

Aside from this, could you possibly take a look at those SVGs and see if you can get them to look correct without the hack with the negative x & y values?

Thanks!
Flags: needinfo?(bram)
(In reply to Nihanth Subramanya [:nhnt11] from comment #25)
> (In reply to :Gijs Kruitbosch from comment #23)
> > This seems to mean we will no longer use neterror.css, which is
> > theoretically different on all 3 OSes (but in practice seems to be almost
> > identical...). What OSes did you test this styling on?
> 
> I tested it on OS X only, how does this matter for in-content pages?

Different form elements have different default styling on different OSes, and default font sizes are different. You should ideally test on other OSes as well. I can test on things not available to you if necessary, but it'd be good to at least check on other OSes as you are able.

> > ::: browser/base/content/aboutNetError.xhtml
> > @@ +522,5 @@
> > >            <label for="automaticallyReportInFuture" id="automaticallyReportInFuture">&errorReporting.automatic2;</label>
> > >          </p>
> > >        </div>
> > >  
> > > +      <div id="weakCryptoAdvancedPanel" class="advanced-panel">
> > 
> > When I talk about duplication I mean that we're making identical
> > modifications here and in the other .xhtml files. This is unfortunate; it
> > would make more sense to unify the files.
> 
> Unifying these files is likely not trivial,

What makes you say that?

> and in any case, I would like to
> proceed with this bug considering it has already made a fair bit of
> progress. I don't think the introduced duplication warrants putting this on
> hold and unifying the files first, what do you think?

I think we keep doing this to ourselves and it keeps not being a priority to clean this up, so we keep breaking it (cf. the advanced panel stuff that broke). We punt every time, and we need to stop punting.

> > ::: browser/themes/shared/error-pages.css
> > @@ +6,5 @@
> > > +
> > > +body {
> > > +  background-image: linear-gradient(-45deg, #eeeeee,     #eeeeee 33%,
> > > +                                            #fbfbfb 33%, #fbfbfb 66%,
> > > +                                            #eeeeee 66%, #eeeeee);
> > 
> > The rule of thumb for web content is that whenever you specify a background
> > image or color, you specify a foreground color. This is because if you do
> > not specify anything, we will use the default color. That is usually black,
> > but not always. So if you specify an almost-white background color as you do
> > here, and no foreground color, it'll work for you (default foreground color
> > being black), but not for someone with an inverted OS theme, like the dark
> > themes on ubuntu, or custom classic themes on Windows, etc. Those people
> > will get white-on-white which is unreadable.
> 
> The background and foreground colours are specified in common.css (shared by
> in-content pages).

Yes, but this is an image, which goes on top of the background color, so that isn't relevant.

> This is only adding the stripes at the top. A background
> height of 32px is specified immediately after the background-image. I don't
> see why we need to re-specify the foreground color here.

This is the relevant bit, but this isn't clear from the patch. The padding of 75px is gone in the latest version of the patch, AFAICT (why?), and instead there is padding in em (which likely won't match with the actual 32px height here on different OSes) which lives in different selectors and is duplicated because of the media queries. Maybe it should be a CSS variable, but either way, it should be clear from the selector what's going on here and what that gradient is, where it applies, how it is constrained, and where it does/doesn't appear underneath the rest of the content.

> > ::: browser/themes/shared/incontent-icons/cert-error.svg
> > @@ +2,5 @@
> > >  
> > >  <svg version="1.1"
> > >       xmlns="http://www.w3.org/2000/svg"
> > >       xmlns:xlink="http://www.w3.org/1999/xlink"
> > > +     viewBox="-7.5 -7.5 60 60">
> > 
> > I would expect this to increase the net size of the image and thereby
> > decrease the size of the visible icon. Is there no size set for the places
> > where we use this as a background image?
> 
> Yes, the size is set, see
> https://dxr.mozilla.org/mozilla-central/rev/
> d848a5628d801a460a7244cbcdea22d328d8b310/toolkit/themes/shared/in-content/
> info-pages.inc.css#37.

OK, so decreasing the visual size of the icon glyph in the rendered page is the goal here? Just trying to understand what this is fixing.

> > It's also confusing that this adds "margin" around the 45x45 image in an
> > uneven manner, as the top-left corner moves by 7.5 pixels in both the X and
> > Y direction, and the bottom right corner moves by 15 pixels. Why?
> 
> The last two numbers are width and height. If we add a 7.5px margin on all
> sides, the width and height increase by a total of 15px each. Hence 45->60.

Ugh, should have realized that, sorry.

> > We use info.svg in more places than the in-content pages, so ultimately this
> > just seems like the wrong way of fixing this problem (and it seems likely
> > you're regressing the URL bar search suggestion prompt, which uses info.svg)
> > . Fix the pages that include the image to have the correct margin, and don't
> > hack the image like this.
> 
> No, we don't. As far as I can see info.svg is only used in the in-content
> pages.
> https://dxr.mozilla.org/mozilla-central/
> search?q=chrome%3A%2F%2Fglobal%2Fskin%2Ficons%2Finfo.
> svg&redirect=false&case=false
> 
> The URL bar uses a different info.svg found in browser/.

That page still shows an outdated objdir file that uses it as well; afaict the in-source version now uses a separate png. I'll try to remember to file a bug on DXR.

There are a number of comm-central uses ( https://dxr.mozilla.org/comm-central/search?q=icons%2Finfo.svg&redirect=false&case=false ) so we should be nice and file a bug if we do change the icon.

> Fixing the other pages would mean redoing other icons in order to *remove*
> the margins in them. Please compare
> chrome://browser/skin/session-restore.svg and
> chrome://global/skin/icons/info.svg. The way I see it, we either remove
> margins from those or add margins to these ones, and since this bug is about
> changing error pages to use in-content styles, I vote for adding margins to
> info.svg and cert-error.svg. As for hacking the image - I'll ask Bram in the
> next comment.

I would prefer to remove the margins from the other image(s? are there more?) and fix them to all be the same size, and to have them take up the full viewbox. If that makes them too big, constrain the size we use to include them in pages. It doesn't make sense to have large amounts of transparent space in an icon file. It wouldn't make sense in a png and it makes just as little sense in an svg file.

> > There's padding on the bottom, too, though, not just the top? Does it need
> > to be 75px high even on smaller window heights?
> 
> The prototype specified a padding of 12.5vh for window heights below 600px.
> For a 600px window, 12.5vh == 75px, which is how I got the value.
> I changed it to a fixed 75px instead of 12.5vh, because a dynamic padding
> allows the page content to overlap the stripes at the top.
> Because the body is a flexbox with the content vertically center-aligned,
> adding a top padding offsets the vertical alignment. The bottom padding
> counters that and restores the alignment.

Hm. But all we're trying to do is not overlap the stripes? Then why 75px instead of 32px, if that's how high the stripes are?
(as noted earlier, it seems the 75px has disappeared from this version of the patch? 5em isn't really better though...)
(In reply to Nihanth Subramanya [:nhnt11] from comment #26)
> […] Do we not want the UI to change when the window width is
> narrow?

We do want the UI to change when the window is narrowed down in order to help keep the page usable. Images should be hidden, titles should be set smaller so it doesn’t consume too much space, and buttons should stack on top of each other rather than try to crowd the horizontal space.

If we’re talking about what looks good on desktop, the breakpoints should have been something like:

Narrow (stacked buttons): 0–480px
Medium (image hidden): 480–800px
Wide: > 800px

Since we’re planning to eventually deploy this style to Android, I had to tweak it so that it targets these device sizes as much as possible:

Narrow: phone (portrait)
Medium: phone (landscape), tablet (portrait)
Wide: tablet

But maybe my earlier breakpoint is a better idea? What’s the breakpoint that we use in Firefox for Android’s cert error pages, and is it a good idea if we just follow that?


> I didn't think of the iframe case. Bram, what do you think about this?

I kept the stripe decoration to better match the style found on Firefox for Android, but am not opposed to reducing its height, especially when there isn’t as much space around it (like when it appears inside an iframe.


(In reply to :Gijs Kruitbosch from comment #27)
> I would prefer to remove the margins from the other image(s? are there
> more?) and fix them to all be the same size, and to have them take up the
> full viewbox.

Nihanth, it sounds like what we need to do here is go back to session-restore.svg, tab-crashed.svg and welcome-back.svg, then remove the margin.
Flags: needinfo?(bram)
I spoke to Bram about the stripes and the top padding, and the conclusion was:

- For screen height less than 480px, resize the stripes' height to 20px to save 12px of real estate.
- Use a padding value of 38px to prevent overlap with the stripes.

We also spoke about the SVGs. I'm going to undo the margin changes to the SVGs, but not touch any of the other in-content icons either in this bug. Instead, I'll file a new bug to fix those icons and adjust their CSS rules to make everything consistent.

(In reply to :Gijs Kruitbosch from comment #27)
> Different form elements have different default styling on different OSes,
> and default font sizes are different. You should ideally test on other OSes
> as well. I can test on things not available to you if necessary, but it'd be
> good to at least check on other OSes as you are able.

Fair enough. Fwiw the only form element that these pages contain is the error reporting checkbox (other than the buttons, but those are custom styled anyway).

> > Unifying these files is likely not trivial,
> 
> What makes you say that?

Just an impression I've gotten from looking at these files. Maybe I'm wrong, it was pure speculation.

> I think we keep doing this to ourselves and it keeps not being a priority to
> clean this up, so we keep breaking it (cf. the advanced panel stuff that
> broke). We punt every time, and we need to stop punting.

This is fair, I guess. However, I'd like to upload one more iteration of this patch before I forget the details of the discussion around it.
Depends on: 1251462
Priority: P2 → --
Review commit: https://reviewboard.mozilla.org/r/46237/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46237/
Attachment #8741166 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741168 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741169 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741170 - Flags: review?(gijskruitbosch+bugs)
Phew, finally redid this and split the patch into logical steps. Please forgive me if in the process of redoing the changes, I missed some review comments. This was just easier for me than doing an interactive hg commit of the changes already made, especially with the net/cert error page merge having landed. Anyway, I'm going through this bug again and double checking.

Thanks, and I hope the review is easier this time.
Attachment #8723391 - Attachment is obsolete: true
Comment on attachment 8741170 [details]
MozReview Request: Bug 1220481 - Fix whitespace nits. r=Gijs

https://reviewboard.mozilla.org/r/46245/#review42901
Attachment #8741170 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8741166 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8741166 [details]
MozReview Request: Bug 1220481 - Deduplicate common styles between aboutNetError.css and info-pages.css. r=Gijs

https://reviewboard.mozilla.org/r/46237/#review42899

r- for breaking the formatting of the contents of the error pages. Did you test the patch?

::: browser/base/content/aboutNetError.xhtml:225
(Diff revision 1)
> -        var errTitle = document.getElementById("et_" + err);
> -        var errDesc  = document.getElementById("ed_" + err);
> +        var errTitle = document.getElementById("et_" + err).textContent;
> +        var errDesc  = document.getElementById("ed_" + err).textContent;

This should be using innerHTML, here and below. Right now you've broken the formatting of e.g. the "Server not found" error, which uses HTML.

::: browser/base/content/aboutNetError.xhtml:479
(Diff revision 1)
> -    <div id="errorContainer">
> -      <div id="errorTitlesContainer">
> +    <div id="errorContainer" class="container">
> +      <div id="errorTitlesContainer" class="title">

Adding classes to these 2 elements seems like it doesn't actually do anything for the styles, right, as the container is removed anyway?

::: browser/base/content/aboutNetError.xhtml:481
(Diff revision 1)
>      <div id="certErrorPageTitle" style="display: none;">&certerror.pagetitle1;</div>
>  
>      <!-- ERROR ITEM CONTAINER (removed during loading to avoid bug 39098) -->
> -    <div id="errorContainer">
> -      <div id="errorTitlesContainer">
> -        <h1 id="et_generic">&generic.title;</h1>
> +    <div id="errorContainer" class="container">
> +      <div id="errorTitlesContainer" class="title">
> +        <div id="et_generic">&generic.title;</div>

Why change these to `<div>` from `<h1>` ? AFAICT it makes no difference for what you're doing here, and it does break blame.

::: browser/base/content/aboutNetError.xhtml:542
(Diff revision 1)
> -      <div id="errorTitle">
> -        <h1 id="errorTitleText" />
> +      <div id="errorTitle" class="title">
> +        <h1 class="title-text" id="errorTitleText"/>

If we do this, can we get rid of the IDs? If we get rid of the class on the invisible error message container, there's only one element with this class anyway, and the mixture of classes and IDs used to set images on these is confusing.

::: browser/themes/shared/aboutNetError.css
(Diff revision 1)
> -  margin-top: 1.2em;
> -  min-width: 150px

This isn't in a .button-container, so there is no equivalent rule after this patch alone, is there? Should this blob be in a different patch ?

::: browser/themes/shared/aboutNetError.css
(Diff revision 1)
>    display: none;
>  }
>  
>  #buttonContainer {
>    display: none;
> -  flex-flow: row wrap;

This too seems to have no equivalent.

::: browser/themes/shared/aboutNetError.css
(Diff revision 1)
> -
> -#returnButton {
> -  background-color: var(--in-content-primary-button-background);
> -  border: none;
> -  color: var(--in-content-selected-text);
> -  min-width: 250px;

This button was wider, and I don't see an equivalent style for this either...
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

https://reviewboard.mozilla.org/r/46239/#review42927

::: browser/base/content/aboutNetError.xhtml:269
(Diff revision 1)
> -          document.getElementById("buttonContainer").style.display = "flex";
> +          document.getElementById("certErrorButtonContainer").style.display = "flex";
> +          document.body.className = "certerror";
>          }
>  
>          if (err == "weakCryptoUsed") {
> -          document.getElementById("errorTitle").setAttribute("weakCrypto", "true");
> +          document.body.className = "certerror";

This stuff is now annoying to test becaues bug 1253166 removed rc4 override support, so I don't know of a way to get this page to show up at all. Manually loading:

about:neterror?e=weakCryptoUsed&u=https%3A//rc4.badssl.com/&c=UTF-8&f=regular&d=An%20error%20occurred%20during%20a%20connection%20to%20rc4.badssl.com.%0A%0ACannot%20communicate%20securely%20with%20peer%3A%20no%20common%20encryption%20algorithm%28s%29.%0A%0AError%20code%3A%20%3Ca%20id%3D%22errorCode%22%20title%3D%22SSL_ERROR_NO_CYPHER_OVERLAP%22%3ESSL_ERROR_NO_CYPHER_OVERLAP%3C/a%3E%0A

(which is what we're currently loading with the error code changed)

produces a link on the error code, that hasn't been linkified. The real error doesn't try to link the error code because you're using textContent (see feedback on the previous patch). So I can't tell if linking the errors to SUMO or elsewhere is broken here or elsewhere, but we should make sure that we don't ship it broken either way.

::: browser/base/content/aboutNetError.xhtml:582
(Diff revision 1)
>          // button.
>          if (window.top == window) {
>              var button = document.getElementById("errorTryAgain");
> -            var nextSibling = button.nextSibling;
>              var parent = button.parentNode;
>              parent.removeChild(button);

Nit: While we're here,

```
button.remove();
```

::: browser/themes/shared/aboutNetError.css
(Diff revision 1)
> -#errorTitle[sslv3=true],
> -#errorTitle[weakCrypto=true] {
> -  background-image: url("cert-error.svg");
> -}

This case isn't caught by `body.certerror` so now the icon is wrong, see e.g. https://rc4.badssl.com/ . We still need an equivalent to the code that set this to either get it treated as a cert error (which might change the options we give people to proceed, so tread carefully!) or apply the same styling despite not having `.certerror` on the body.

::: browser/themes/shared/aboutNetError.css:44
(Diff revision 1)
>    display: none;
>  }
>  
> -#buttonContainer {
> +.button-container {
> +  display: flex;
> +  flex-flow: row wrap;

So we're adding this back here... I'm not sure why this is in a different patch to the removal, but OK. Feels like it should have just stayed in there so we don't touch blame on the line.

::: browser/themes/shared/aboutNetError.css:132
(Diff revision 1)
>    padding: 1em 0;
>  }
> +
> +@media only screen and (max-width: 959px) {
> +  body {
> +    padding: 5em 3.2em;

So initially, padding is 75px 0;

Here, you're setting it in em. That doesn't work because it's supposed to match the size of the background image on the body, which is sized in px, and those sizes are sometimes not going to line up depending on OS font sizes (e.g. by default on Ubuntu, and either OS X or Windows depending on where you got the current ratio from).

What's more, I don't really follow why we're adding horizontal padding. Should we just have the horizontal padding there from the beginning? What's 3.2em based on? On my OSX box, it seems to be 48px, which matches what info-pages.css declares, but that does it in px rather than em. So I worry that you picked an em value to match the px value on your machine, and that is not going to work everywhere (ie it will look wrong for people with large fonts set) - we should likely just hardcode the 48px.

::: browser/themes/shared/aboutNetError.css:136
(Diff revision 1)
> +    border: none;
> +    padding: 1rem;
> +    background-color: #e0e2e5;
> +    font-weight: 300;

I'm deeply uncomfortable with changing the border/background styling of the buttons here seemingly willy-nilly. The colors/borders on the error page on desktop Firefox should look the same when the window is 950px wide or 1000px wide. Please can we get the spec updated to do that (or ignore that part of the spec).

Additionally, the lack of border and using background-color (with very little contrast) exclusively means this won't look like a button anymore on e.g. Windows High Contrast mode. It'll just be some free-floating bit of text.


Likewise, the padding/font-weight change seems predicated on wanting to be more touch-friendly, for which there are more appropriate detecting mechanisms than simply checking the screen/window width.

Even if we wanted to keep this, note that the selector you're using elsewhere is `.button-container > button`, and now you're using just `button` - why?

::: browser/themes/shared/aboutNetError.css:148
(Diff revision 1)
> +  .title > h1 {
> +    padding-top: 0;
> +  }

Shouldn't this be .title-text ?

::: browser/themes/shared/aboutNetError.css:156
(Diff revision 1)
> +}
> +
> +@media only screen and (max-width: 640px) {
> + 
> +  body {
> +    padding: 5em 1.33em 0;

This has the same problems I outlined earlier.

::: browser/themes/shared/aboutNetError.css:160
(Diff revision 1)
> +  h1 {
> +    font-size: 2.25em;
> +  }

This seems like it should use .title-text ? In fact, it seems like it now gets overridden by the info-pages.css selector for .title-text, which is more specific. So either this rule is not necessary, or it should use the same selector and thereby override the one from info-pages.css .

(FWIW, testing this quickly, it seems this font size is huge and we should just remove this rule.)

::: browser/themes/shared/aboutNetError.css:165
(Diff revision 1)
> +  h1 {
> +    font-size: 2.25em;
> +  }
> + 
> +  button {
> +    /* Force buttons to display: block here to try and enfoce collapsing margins */

Nit: fix typo in this comment.

Is there an example of when this does something? Shouldn't this be restricted to .button-container as well?

::: browser/themes/shared/aboutNetError.css:167
(Diff revision 1)
> +  }
> + 
> +  button {
> +    /* Force buttons to display: block here to try and enfoce collapsing margins */
> +    display: block;
> +    width: 100%;

Whether this makes sense depends on the container, so doing this for all `button`s doesn't seem like a good idea.

::: browser/themes/shared/aboutNetError.css:171
(Diff revision 1)
> +  .advanced-button {
> +    margin-top: 1.2em;
> +    float: none !important;
> +  }

This selector does not match anything.

::: browser/themes/shared/aboutNetError.css:176
(Diff revision 1)
> +  .title > h1 {
> +    padding-bottom: 0;
> +    border-bottom: none;
> +  }

Again, feels like this should use .title-text .

::: browser/themes/shared/aboutNetError.css:185
(Diff revision 1)
> +}
> +
> +/* For small window height, shift the stripes up by 10px.
> + * We could just change the background size, but that changes
> + * the angle of the stripes so just shifting up is easier. */
> +@media (max-height: 480px) {

It seems to me that we could do this sooner, but that's just me.
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741168 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8741168 [details]
MozReview Request: Bug 1220481 - New responsive styles for blockedSite.xhtml. r=Gijs

https://reviewboard.mozilla.org/r/46241/#review42941

I'm going to clear review on the 3rd and 4th patch because I think there are a sufficient number of issues in the previous two patches that will influence these patches, that it would make sense to get those addressed before I review the following patches, as we move the then-hopefully-changed rules/elements around etc.
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

https://reviewboard.mozilla.org/r/46243/#review42903

::: browser/themes/shared/blockedSite.css:23
(Diff revision 1)
>  
>  .title-text {
>    color: white;
>  }
>  
> -.button-container {
> +#buttons button:not(.primary) {

This and all the other `#buttons button` selectors could use:

```
.button-container > button
```

right? That would seem preferable...
Attachment #8741169 - Flags: review?(gijskruitbosch+bugs)
FWIW, it seems quite a lot of the whitespace you fix in the last patch you add in the previous patches - please just don't add it in the first place. :-)
> ::: browser/themes/shared/aboutNetError.css:185
> > +@media (max-height: 480px) {
> 
> It seems to me that we could do this sooner, but that's just me.

To be more precise: at a bigger window height, we could already reduce the size of the stripes at the top.
https://reviewboard.mozilla.org/r/46239/#review42927

> Nit: fix typo in this comment.
> 
> Is there an example of when this does something? Shouldn't this be restricted to .button-container as well?

This makes the buttons occupy a full "row" horizontally. They appear stacked one below the other.
https://reviewboard.mozilla.org/r/46239/#review42927

> I'm deeply uncomfortable with changing the border/background styling of the buttons here seemingly willy-nilly. The colors/borders on the error page on desktop Firefox should look the same when the window is 950px wide or 1000px wide. Please can we get the spec updated to do that (or ignore that part of the spec).
> 
> Additionally, the lack of border and using background-color (with very little contrast) exclusively means this won't look like a button anymore on e.g. Windows High Contrast mode. It'll just be some free-floating bit of text.
> 
> 
> Likewise, the padding/font-weight change seems predicated on wanting to be more touch-friendly, for which there are more appropriate detecting mechanisms than simply checking the screen/window width.
> 
> Even if we wanted to keep this, note that the selector you're using elsewhere is `.button-container > button`, and now you're using just `button` - why?

I've removed this styling but it causes the buttons' height to look way too small compared to their width when viewport width is below 640px (because they occupy a full "row" in that case - see my previous comment). I'll needinfo? Bram about this. Sorry for the double comment, reviewboard derp'd I think.
https://reviewboard.mozilla.org/r/46239/#review42927

> This stuff is now annoying to test becaues bug 1253166 removed rc4 override support, so I don't know of a way to get this page to show up at all. Manually loading:
> 
> about:neterror?e=weakCryptoUsed&u=https%3A//rc4.badssl.com/&c=UTF-8&f=regular&d=An%20error%20occurred%20during%20a%20connection%20to%20rc4.badssl.com.%0A%0ACannot%20communicate%20securely%20with%20peer%3A%20no%20common%20encryption%20algorithm%28s%29.%0A%0AError%20code%3A%20%3Ca%20id%3D%22errorCode%22%20title%3D%22SSL_ERROR_NO_CYPHER_OVERLAP%22%3ESSL_ERROR_NO_CYPHER_OVERLAP%3C/a%3E%0A
> 
> (which is what we're currently loading with the error code changed)
> 
> produces a link on the error code, that hasn't been linkified. The real error doesn't try to link the error code because you're using textContent (see feedback on the previous patch). So I can't tell if linking the errors to SUMO or elsewhere is broken here or elsewhere, but we should make sure that we don't ship it broken either way.

I missed an existing style that makes the link look like normal text, I've added it back. This preserves behavior with none of these patches applied. There's no actual info text added to the certificateErrorDebugInformation block in this case (which is what addDomainError() tries to link error code anchor elements to) so this seems correct?
Comment on attachment 8741166 [details]
MozReview Request: Bug 1220481 - Deduplicate common styles between aboutNetError.css and info-pages.css. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46237/diff/1-2/
Attachment #8741166 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741168 - Flags: review?(gijskruitbosch+bugs)
Attachment #8741169 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46239/diff/1-2/
Comment on attachment 8741168 [details]
MozReview Request: Bug 1220481 - New responsive styles for blockedSite.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46241/diff/1-2/
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46243/diff/1-2/
Attachment #8741170 - Attachment is obsolete: true
Bram, Gijs has some good reasons for keeping buttons styled the same for all viewport widths: https://reviewboard.mozilla.org/r/46239/#comment56101.

Can you please give me a spec that preserves the button styling as Gijs suggests? Or maybe you have some counter arguments? Thanks!
Flags: needinfo?(bram)
(In reply to Nihanth Subramanya [:nhnt11] from comment #50)
> Bram, Gijs has some good reasons for keeping buttons styled the same for all
> viewport widths: https://reviewboard.mozilla.org/r/46239/#comment56101.

The reason why button colour and background aren’t the same for all widths was because the narrow styles was meant to be shown on mobile devices. And there, we’d like to follow the style that Firefox Android/iOS design use.

However, should we want to keep the style consistent, I have no objection. The benefit here is rather obvious, too: smaller and simpler stylesheet.

So what we  can do here is erase line 136–139, and that should do it.

Thoughts?
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #51)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #50)
> > Bram, Gijs has some good reasons for keeping buttons styled the same for all
> > viewport widths: https://reviewboard.mozilla.org/r/46239/#comment56101.
> 
> The reason why button colour and background aren’t the same for all widths
> was because the narrow styles was meant to be shown on mobile devices. And
> there, we’d like to follow the style that Firefox Android/iOS design use.
> 
> However, should we want to keep the style consistent, I have no objection.
> The benefit here is rather obvious, too: smaller and simpler stylesheet.
> 
> So what we  can do here is erase line 136–139, and that should do it.
> 
> Thoughts?

As you can see in my response to the comment I linked, I've already done this in the latest patch but it looks rather ugly when the window is just small enough for the media block to take effect. I've attached a screenshot for context.

If we remove lines 136-139, we likely want a new spec to avoid the ugly huge buttons. It's especially awkward for the "Server not found" case, where there's only one button ("Try again"). I don't have any ideas for making that look nice other than to just lower the viewport width threshold below 640px (but I can't think off the top of my head of a mathematical way to decide what a good new threshold would be).
Flags: needinfo?(bram)
(In reply to Nihanth Subramanya [:nhnt11] from comment #52)
> […] I don't have any ideas for making
> that look nice other than to just lower the viewport width threshold below
> 640px (but I can't think off the top of my head of a mathematical way to
> decide what a good new threshold would be).

480px would be a good threshold, but 640px covers both a phone’s portrait and landscape layouts. We don’t want the small device layout to change when you rotate your phone, for example.

If you’re online or on IRC, I’m available for a quick chat. Perhaps it’ll be quicker to resolve there.
Flags: needinfo?(bram)
Comment on attachment 8741166 [details]
MozReview Request: Bug 1220481 - Deduplicate common styles between aboutNetError.css and info-pages.css. r=Gijs

https://reviewboard.mozilla.org/r/46237/#review43303

This looks good with the below fixed.

::: browser/base/content/aboutNetError.xhtml:233
(Diff revision 2)
> -        var title = document.getElementById("errorTitleText");
> +        var title = document.querySelector(".title-text");
>          if (title)
>          {
> -          title.parentNode.replaceChild(errTitle, title);
> +          title.innerHTML = errTitle;

While we're here, this nullcheck seems pretty useless to me. Let's get rid of it.

::: browser/base/content/aboutNetError.xhtml:245
(Diff revision 2)
>          if (sd) {
>            if (gIsCertError) {
>              sd.innerHTML = document.getElementById("ed_nssBadCert").innerHTML;
>            }
>            else {
> -            sd.textContent = getDescription();
> +            sd.innerHTML = getDescription();

I totally (unintentionally! I just wasn't clear...) set you up for this, for which I apologize - but getDescription() comes out of the URL (unlike the ed_ and et_ stuff, which is from the page itself!), so we shouldn't just dump it into innerHTML, because that creates spoofing / XSS risks. This should remain .textContent, which it was before both this patch and your previous patch as well. If there are issues with that (are there?) then we should very very very carefully fix that in some other bug.
Attachment #8741166 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Nihanth Subramanya [:nhnt11] from comment #52)
> If we remove lines 136-139, we likely want a new spec to avoid the ugly huge
> buttons. It's especially awkward for the "Server not found" case, where
> there's only one button ("Try again"). I don't have any ideas for making
> that look nice other than to just lower the viewport width threshold below
> 640px (but I can't think off the top of my head of a mathematical way to
> decide what a good new threshold would be).

We could center the button for widths between 640 and 480px ?
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

https://reviewboard.mozilla.org/r/46239/#review43307

This is really kind of r+, but I think considering some of the refactoring + questions I'd like to stamp the final version, too, please.

::: browser/base/content/aboutNetError.xhtml:119
(Diff revision 2)
> -        document.getElementById("buttonContainer").style.display = "flex";
> +        document.getElementById("certErrorButtonContainer").style.display = "flex";
>          document.getElementById("advancedButton").style.display = "block";
> -        document.getElementById("errorTryAgain").style.display = "none";
> +        document.getElementById("netErrorButtonContainer").style.display = "none";

Hmm... thinking about this, this is kind of tightly coupled with the styling of the page, which is unfortunate. Can we just add a class to the body, like "certerror-weak" or something, or and then style the button containers from the CSS based on that?

::: browser/base/content/aboutNetError.xhtml:260
(Diff revision 2)
>          {
>            ld.innerHTML = errDesc;
>          }
>  
>          if (err == "sslv3Used") {
> -          document.querySelector(".title").setAttribute("sslv3", "true");
> +          document.getElementById("netErrorButtonContainer").style.display = "none";

We already do this from showAdvancedButton, so this line could just be removed, I think.

::: browser/base/content/aboutNetError.xhtml:264
(Diff revision 2)
> -          document.querySelector(".title").setAttribute("sslv3", "true");
> +          document.getElementById("netErrorButtonContainer").style.display = "none";
> -          document.getElementById("errorTryAgain").style.display = "none";
>            document.getElementById("learnMoreContainer").style.display = "block";
>            var learnMoreLink = document.getElementById("learnMoreLink");
>            learnMoreLink.href = "https://support.mozilla.org/kb/how-resolve-sslv3-error-messages-firefox";
> -          document.getElementById("buttonContainer").style.display = "flex";
> +          document.getElementById("certErrorButtonContainer").style.display = "flex";

Ditto.

::: browser/themes/shared/aboutNetError.css:104
(Diff revision 2)
>  
>  #automaticallyReportInFuture {
>    cursor: pointer;
>  }
>  
> -body:not(.certerror) #errorCode {
> +#errorCode:not([href]) {

nice!

::: browser/themes/shared/aboutNetError.css:157
(Diff revision 2)
> +    padding-top: 0;
> +  }
> +}
> +
> +@media only screen and (max-width: 640px) {
> +

Nit: please remove the blank line here.

::: browser/themes/shared/aboutNetError.css:159
(Diff revision 2)
> +}
> +
> +@media only screen and (max-width: 640px) {
> +
> +  body {
> +    padding: 75px 20px 0;

Here you remove the padding-bottom, but for max-height 480px we add it back. Is that intentional, and just about whether we center content or something? On small windows like those < 480px, we're still going to ditch 76px on the padding. Seems like we could leave the bottom padding off in the latter case to make it more likely content fits on the screen? Oh, hm, though maybe the content gets too close to the bottom if there is 0 padding? Now I'm not sure anymore. :-)

Not opening an issue for this, but it would be good to have clarity about what our aim is with these.
Comment on attachment 8741168 [details]
MozReview Request: Bug 1220481 - New responsive styles for blockedSite.xhtml. r=Gijs

https://reviewboard.mozilla.org/r/46241/#review43309

r=me if and only if at least this and the previous commit get squashed with the last commit. Some of my comments on the aboutNetError.css case apply here, and got fixed for net/certerror, and in the final merge commit, but not in this one (e.g. padding; use of .button-container button, etc.). :-)

::: browser/themes/shared/blockedSite.css:21
(Diff revision 2)
> +  background-color: #b14646;
> +  color: white;
> +}
> +
> +.title-text {
> +  color: white;

Is this necessary, considering the body color is already set to white?
Attachment #8741168 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

https://reviewboard.mozilla.org/r/46243/#review43311

r=me with the whitespace fixed (where did that commit go? Mozreview booboo? Or did you push -r . while on this commit without the last one / without rebasing that one?)

::: browser/themes/shared/error-pages.css:28
(Diff revision 2)
> +
> +@media only screen and (max-width: 959px) {
> +  body {
> +    padding: 75px 48px;
> +  }
> + 

Nit: please remove trailing whitespace here and below.
Attachment #8741169 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8741166 [details]
MozReview Request: Bug 1220481 - Deduplicate common styles between aboutNetError.css and info-pages.css. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46237/diff/2-3/
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46239/diff/2-3/
Comment on attachment 8741168 [details]
MozReview Request: Bug 1220481 - New responsive styles for blockedSite.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46241/diff/2-3/
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46243/diff/2-3/
(In reply to :Gijs Kruitbosch from comment #56)
> Comment on attachment 8741167 [details]
> MozReview Request: Bug 1220481 - New responsive design styles for
> aboutNetError.xhtml. r=Gijs
> 
> https://reviewboard.mozilla.org/r/46239/#review43307
> 
> ::: browser/base/content/aboutNetError.xhtml:119
> (Diff revision 2)
> > -        document.getElementById("buttonContainer").style.display = "flex";
> > +        document.getElementById("certErrorButtonContainer").style.display = "flex";
> >          document.getElementById("advancedButton").style.display = "block";
> > -        document.getElementById("errorTryAgain").style.display = "none";
> > +        document.getElementById("netErrorButtonContainer").style.display = "none";
> 
> Hmm... thinking about this, this is kind of tightly coupled with the styling
> of the page, which is unfortunate. Can we just add a class to the body, like
> "certerror-weak" or something, or and then style the button containers from
> the CSS based on that?

showAdvancedButton() is called from all cases of the certerror class, so I just used that - no need for a new one.

> Here you remove the padding-bottom, but for max-height 480px we add it back.
> Is that intentional, and just about whether we center content or something?
> On small windows like those < 480px, we're still going to ditch 76px on the
> padding. Seems like we could leave the bottom padding off in the latter case
> to make it more likely content fits on the screen? Oh, hm, though maybe the
> content gets too close to the bottom if there is 0 padding? Now I'm not sure
> anymore. :-)

Good point, I forgot that this will override the 0 padding for < 640px. I'm going to upload another revision of the patch with the margin rules isolated for clarity (since we will likely need nested @media queries anyway, unless I can think of something better).
https://reviewboard.mozilla.org/r/46243/#review44353

::: browser/themes/shared/error-pages.css:68
(Diff revision 3)
> + * the angle of the stripes so just shifting up is easier. */
> +@media (max-height: 480px) {
> +  body {
> +    background-position: 10px -10px;
> +    padding-top: 38px;
> +    padding-bottom: 38px;

There needs to be another rule to remove the bottom padding for width < 640px (this overrides that case).

I'll isolate all the padding rules into one block and write a better comment for clarity, probably worth the extra LOC since we'll likely need nested @media queries.

Sorry for the double BMO comment, I wanted to open an issue here too for the record.
Attachment #8741167 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

https://reviewboard.mozilla.org/r/46239/#review44429

r=me
https://reviewboard.mozilla.org/r/46243/#review44353

> There needs to be another rule to remove the bottom padding for width < 640px (this overrides that case).
> 
> I'll isolate all the padding rules into one block and write a better comment for clarity, probably worth the extra LOC since we'll likely need nested @media queries.
> 
> Sorry for the double BMO comment, I wanted to open an issue here too for the record.

After playing around with a build, I like the bottom padding for small window height. I agree with what you said about "maybe the content gets too close to the bottom if there is 0 padding". I'll add a comment to that effect.
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46243/diff/3-4/
Comment on attachment 8741166 [details]
MozReview Request: Bug 1220481 - Deduplicate common styles between aboutNetError.css and info-pages.css. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46237/diff/3-4/
Comment on attachment 8741167 [details]
MozReview Request: Bug 1220481 - New responsive design styles for aboutNetError.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46239/diff/3-4/
Comment on attachment 8741168 [details]
MozReview Request: Bug 1220481 - New responsive styles for blockedSite.xhtml. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46241/diff/3-4/
Comment on attachment 8741169 [details]
MozReview Request: Bug 1220481 - Unify styles common to blockedSite.css and aboutNetError.css in a new error-pages.css file. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46243/diff/4-5/
https://hg.mozilla.org/mozilla-central/rev/9062049993e1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Iteration: --- → 48.3 - Apr 25
Priority: -- → P1
Depends on: 1267514
Blocks: 1241084
You need to log in before you can comment on or make changes to this bug.