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

RESOLVED FIXED in Firefox 33

Status

()

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rctgamer3, Assigned: Gijs)

Tracking

({regression})

32 Branch
Firefox 35
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify -

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ fixed, firefox34+ fixed, firefox35 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

5 years ago
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
Reporter

Updated

5 years ago
Blocks: 982347
Severity: normal → major
[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?
Assignee

Comment 2

5 years ago
(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?
Flags: needinfo?(bmcbride)
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.
Flags: needinfo?(bmcbride)
Assignee

Comment 4

5 years ago
Attachment #8482666 - Flags: review?(bmcbride)
Assignee

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee

Comment 5

5 years ago
Marco, can you add this to the iteration spreadsheet, please?
Iteration: --- → 35.1
Points: --- → 2
Flags: qe-verify-
Flags: needinfo?(mmucci)
Flags: in-testsuite-
Flags: firefox-backlog+
Target Milestone: Firefox 32 → ---
Added to IT 35.1
Flags: needinfo?(mmucci)
Why is this stylesheet in content/ in the first place?
Assignee

Comment 9

5 years ago
(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?
Flags: needinfo?(bmcbride)
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?
Keywords: regression
Assignee

Comment 11

5 years ago
(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.
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.
Tracking but we won't block 33 for this.
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.
Flags: needinfo?(bmcbride)
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.
Attachment #8482666 - Flags: review?(bmcbride) → review+
(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.
(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.
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.
Assignee

Comment 20

5 years ago
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).
Attachment #8484140 - Flags: review?(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.
Attachment #8484140 - Flags: review?(dao) → review+
Assignee

Comment 23

5 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/4d52d359af70
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 24

5 years ago
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
Attachment #8482666 - Flags: approval-mozilla-beta?
Attachment #8482666 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4d52d359af70
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Comment on attachment 8482666 [details] [diff] [review]
add blank theme file for net error pages,

Approved the simple change for Aurora and Beta.
Attachment #8482666 - Flags: approval-mozilla-beta?
Attachment #8482666 - Flags: approval-mozilla-beta+
Attachment #8482666 - Flags: approval-mozilla-aurora?
Attachment #8482666 - Flags: approval-mozilla-aurora+

Comment 28

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.