Closed Bug 983761 Opened 7 years ago Closed 7 years ago

tab-active-middle@2x.png should be used with lightweight themes

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

()

Details

(Whiteboard: [Australis:P3-])

Attachments

(3 files)

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.
I needed to add support for CSSGroupingRules otherwise we would throw an error in updateStyleSheet and wouldn't handle the media query rules.
Attachment #8391457 - Flags: review?(mconley)
Attachment #8391458 - Flags: review?(shorlander)
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.
Attachment #8391457 - Flags: review?(mconley) → review+
Comment on attachment 8391458 [details] [diff] [review]
v.1 Use tab-active-middle@2x.png

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

Assuming you've tested this and aspect ratios are fine and so on, rs=me
Attachment #8391458 - Flags: review?(shorlander) → review+
(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 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
Attachment #8391457 - Flags: approval-mozilla-aurora?
Attachment #8391458 - Flags: approval-mozilla-aurora?
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/610649d473cc
https://hg.mozilla.org/mozilla-central/rev/c56350f64da2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 30
Attachment #8391457 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8391458 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.