1035536 - New error pages are unable to be styled by 3rd party complete themes

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[rctgamer3]

Since the new look of the landed in Firefox 32, complete themes are unable to style the new error pages (like about:neterror), because the files aren't in a themable directory (only styles from content are loaded).

about:neterror:

chrome://browser/content/aboutneterror/netError.css
(chrome://global/skin/netError.css exists but isn't loaded anymore)

about:certerror:

chrome://browser/skin/aboutCertError.css
chrome://browser/content/certerror/aboutCertError.css

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

[Tracking Requested - why for this release]:
Looks like we lost the chance on 32, but is pretty important for theme devs. Is this fixable or were the files moved intentionally?

Comment 2

[:Gijs (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #1)
> [Tracking Requested - why for this release]:
> Looks like we lost the chance on 32, but is pretty important for theme devs.
> Is this fixable or were the files moved intentionally?

Blair?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

It was intentional, but not with the aim of breaking 3rd party themes :\ That said, we have been intentionally de-prioritizing support for 3rd party themes, as there's just so few people using them (and the experience of using them is sub-par).

I guess a simple solution would be to have the page load a blank theme file.

Comment 4

[:Gijs (he/him)]

Attachment: add blank theme file for net error pages,

Comment 5

[:Gijs (he/him)]

Marco, can you add this to the iteration spreadsheet, please?

Comment 6

[Marco Mucci [:MarcoM]]

Added to IT 35.1

Comment 7

[Aleksandr Seleznev]

[Note] New about:privatebrowsing page has very similar issue. Can somebody fix it too?

1. https://bugzilla.mozilla.org/show_bug.cgi?id=1009370
2. http://hg.mozilla.org/mozilla-central/file/27e503753e00/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#l22

Comment 8

[Dão Gottwald [:dao]]

Why is this stylesheet in content/ in the first place?

Comment 9

[:Gijs (he/him)]

(In reply to Alexander Seleznev from comment #7)
> [Note] New about:privatebrowsing page has very similar issue. Can somebody
> fix it too?
> 
> 1. https://bugzilla.mozilla.org/show_bug.cgi?id=1009370
> 2.
> http://hg.mozilla.org/mozilla-central/file/27e503753e00/browser/components/
> privatebrowsing/content/aboutPrivateBrowsing.xhtml#l22

I'm not sure about the answer to comment #8 either here or for bug 1009370. Blair?

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment 1 makes it sound like this is important to some number of theme devs. Comment 3 states that we are deprioritizing the capability that we regressed here anyway. How does the proposed fix of loading a blank theme file help theme devs? Is this important enough to warrant uplift to beta 33?

Comment 11

[:Gijs (he/him)]

(In reply to Lawrence Mandel [:lmandel] from comment #10)
> Comment 1 makes it sound like this is important to some number of theme
> devs. Comment 3 states that we are deprioritizing the capability that we
> regressed here anyway. How does the proposed fix of loading a blank theme
> file help theme devs?

Because themes can only affect files under skin packages (browser/themes) and the current pages don't load any such files, which means it isn't possible for themes to affect the styling of the pages. Loading an empty file in the default theme means that theme authors can put a functional styling file at the same URL in their theme.

>  Is this important enough to warrant uplift to beta 33?

I think the fix currently here is important and low risk enough to warrant uplift, yes. If the current fix is not OK (there's a needinfo to understand the situation better) it's possible that a different fix will have different risks and therefore we may need to revisit. For now I would say that if the patch got review, I would ask for beta uplift.

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

Even if only for code quality reasons, we shouldn't be moving theme code into content, unless there's a strong reason for doing it. It's true that complete themes haven't gotten much love for years, but that doesn't mean they are deprecated or that we can't improve their UX in the future. For now I'd be happy if we just avoid breaking them.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tracking but we won't block 33 for this.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The reasoning behind having all the CSS in content was that these pages are intended to be free from any platform conventions and differences. So, indeed, there wasn't a *strong* reason for having all the CSS in content, as we can technically achieve this by including a shared CSS file from each of the themes.

In the context of these pages, I think there are arguments for doing it either way. In the interests of getting things actually landed rather than bikeshedding on it, we went with the easiest.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8482666 [details] [diff] [review]
add blank theme file for net error pages,

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

I think this is the lowest risk for Beta.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jorge Villalobos [:jorgev] from comment #12)
> Even if only for code quality reasons, we shouldn't be moving theme code
> into content

I don't see how this relates to code quality.

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #16)
> (In reply to Jorge Villalobos [:jorgev] from comment #12)
> > Even if only for code quality reasons, we shouldn't be moving theme code
> > into content
> 
> I don't see how this relates to code quality.

I think keeping style code in the skin rather than content is a code quality issue, but I can see how there could be differences of opinion there.

Comment 18

[Dão Gottwald [:dao]]

The empty theme file seems like the right choice for beta and aurora, but for mozilla-central let's please move the styling stuff to themes.

Comment 19

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/61d42bd6aa43

Comment 20

[:Gijs (he/him)]

Attachment: move CSS to themes directory,

I moved the xhtml file to the root of browser/base/content, because it seemed that having a directory for 1 file was a bit pointless. The svg and css I moved to browser/themes/shared. Note that locally, I needed a clobber build to get this to work right, otherwise about:neterror didn't load correctly. If this is reproducible when trying the patch (maybe I did something wrong?), we should consider (a) touching CLOBBER when landing this, and/or (b) figuring out why that is the case and fixing it (but that might not be trivial).

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8484140 [details] [diff] [review]
move CSS to themes directory,

>   skin/classic/browser/aboutCertError.css
>   skin/classic/browser/aboutCertError_sectionCollapsed.png
>   skin/classic/browser/aboutCertError_sectionCollapsed-rtl.png
>   skin/classic/browser/aboutCertError_sectionExpanded.png
>   skin/classic/browser/aboutNetError.css                        (../shared/aboutNetError.css)
>+  skin/classic/browser/netErrorInfo.svg                         (../shared/netErrorInfo.svg)

I'd prefer aboutNetErrorInfo.svg or, looking at aboutCertError_sectionCollapsed.png et al, aboutNetError_info.svg.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61d42bd6aa43

Comment 23

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/4d52d359af70

Comment 24

[:Gijs (he/him)]

Comment on attachment 8482666 [details] [diff] [review]
add blank theme file for net error pages,

Approval Request Comment
[Feature/regressing bug #]: new error page styling
[User impact if declined]: custom themes can't style the new error page at all
[Describe test coverage new/current, TBPL]: none
[Risks and why]: low, only adds a new empty file and links to it from the main page
[String/UUID change made/needed]: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d52d359af70

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8482666 [details] [diff] [review]
add blank theme file for net error pages,

Approved the simple change for Aurora and Beta.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25da1c334a23
https://hg.mozilla.org/releases/mozilla-beta/rev/f9e4f36ba116

Comment 28

[mail]

I tested this on my system Mozilla/5.0 (Windows NT 6.2; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

It appears fixed with themes such as "Lava Fox V2." but I did see an outlier in "Purple Fox" but I assume this is a mistake on the developers part.