983761 - [e-mail] should be used with lightweight themes

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of blurry middle top line

browser-lightweightTheme.css needs to use the 2x image on OS X or it looks blurry in the middle of the tab with a LWT.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 Add support for CSSGroupingRules in LightweightThemeListener

I needed to add support for CSSGroupingRules otherwise we would throw an error in updateStyleSheet and wouldn't handle the media query rules.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Use [e-mail]

Comment 3

[:Gijs (he/him)]

Comment on attachment 8391457 [details] [diff] [review]
v.1 Add support for CSSGroupingRules in LightweightThemeListener

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

So, this is fascinating. As a change, it seems OK to me, so r+

I'll just note that _modifiedStyles is now a dense instead of a sparse array, and that the code assumes that the stylesheet doesn't change except by what it itself is doing (but so did the previous code, I guess). As nobody else seems to rely on this internal array, that should be fine, but we've seen other people being broken by changes we made to LWT internals, so proceed with caution.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8391458 [details] [diff] [review]
v.1 Use [e-mail]

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

Assuming you've tested this and aspect ratios are fine and so on, rs=me

Comment 5

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #3)
> I'll just note that _modifiedStyles is now a dense instead of a sparse
> array, and that the code assumes that the stylesheet doesn't change except
> by what it itself is doing (but so did the previous code, I guess). As
> nobody else seems to rely on this internal array, that should be fine, but
> we've seen other people being broken by changes we made to LWT internals, so
> proceed with caution.

Yeah, this may break if an extension modifies the stylesheet programmatically rather than overlaying the URI but that was true with the previous code too.

   https://hg.mozilla.org/integration/fx-team/rev/610649d473cc
   https://hg.mozilla.org/integration/fx-team/rev/c56350f64da2

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8391457 [details] [diff] [review]
v.1 Add support for CSSGroupingRules in LightweightThemeListener

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Theme support for Australis tabs
User impact if declined: Users with HiDPI screens will have a blurry middle of the tab
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): LWT-specific so it should be lower risk.
String or IDL/UUID changes made by this patch: None

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/610649d473cc
https://hg.mozilla.org/mozilla-central/rev/c56350f64da2

Comment 8

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e183bc8bbf8
https://hg.mozilla.org/releases/mozilla-aurora/rev/e7d56e2e9568