Closed Bug 885139 Opened 11 years ago Closed 10 years ago

LWT header image applied to the selected tab isn't updated upon re-cropping

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- verified

People

(Reporter: gkw, Assigned: MattN)

References

Details

(Keywords: reproducible, Whiteboard: [Australis:P3])

Attachments

(4 files, 1 obsolete file)

Attached image screenshot
Tested on a 2013-06-18 UX build, MattN reproduced it.

1. Install Blue Thunderbird theme at https://addons.mozilla.org/en-US/firefox/addon/blue-thunderbird/
2. Make sure Firefox is open on a Macbook Pro without any external monitor attached (in my case, a 24" Dell monitor).
3. Attach the monitor, the Firefox window should automatically move to the external monitor, but the resolution of that window should not yet be full screen (because the resolution on the Macbook is less than the 24" monitor?).
4. Press the green zoom button on the top left to maximise the window in the 24" monitor.
5. Select the left-most or 2nd left-most tab, you will see the remnant of the theme on the tab.
(refer to screenshot)
Actually I just realized that it's related to cropping of lightweight themes.
Component: Graphics → Theme
Product: Core → Firefox
LWT images are not tiled on #main-window so we shouldn't attempt to either otherwise there is a mismatch with the background in case the image isn't large enough.

A part 2 is needed to update the browser-lightweightTheme LWT images to deal with cropping from LightweightThemeImageOptimizer.jsm.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Attachment #765106 - Flags: review?(dao)
The core issue is that LightweightThemeListener in browser-addons.js doesn't get notified about re-cropping since lightweight-theme-styling-update observers aren't notified. 

Tim/Dão, which of these options do you prefer?
1) fire lightweight-theme-styling-update after |this._update(this._lastData);| in the resize event handler. I haven't checked what else that will break.
2) Have LightweightThemeListener add a MutationObserver for "style" attribute changes and update the stylesheet accordingly.
3) (a cleaner option 2) Move the header image URL to a new attribute on the window and use attr() from @style to refer to it. The mutation observer would only watch changes on this new attribute and not all @style changes.
4) Add a new observer notification to notify about LWT cropping at runtime.
Flags: needinfo?(ttaubert)
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: background-attachment:fixed is positioned incorrectly after attaching external monitor → LWT header image applied to the selected tab isn't updated upon re-cropping
Attached image Part 1 screenshot
This is an improvement but as you can see it won't be perfect until part 2. The border will be shown at the bottom of the selected tab if the LWT is not wide enough.
Here a link to LightweightThemeListener on the UX branch: https://hg.mozilla.org/projects/ux/file/69523ed020c3/browser/base/content/browser-addons.js#l419
Blocks: 813786
No longer blocks: australis-tabs
Attachment #765106 - Flags: review?(dao) → review+
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
Hmm. I'm not sure I completely understand the problem here but (4) sounds like a good way to go. If you want to be notified about re-cropping at runtime this seems like a good solution instead of watching for symptoms.
Flags: needinfo?(ttaubert)
Matt, what needs to be done here still to finish it up?
Flags: needinfo?(mnoorenberghe+bmo)
Implement one of the options from comment 3.
Flags: needinfo?(MattN+bmo)
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P3]
I'm going to implement option 4 from comment 3 today.
I somehow forgot about Windows and Linux in the previously r+'d patch and I also only did the middle and not the ends.

Since Dão won't be around until Monday, anyone else want to review?
Attachment #765106 - Attachment is obsolete: true
Attachment #8391812 - Flags: review?(mconley)
Attachment #8391812 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8391812 [details] [diff] [review]
Part 1 v.2 - Don't tile LightweightTheme images on the selected tab - all platforms

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

Assuming the windows bit covers aero too and you tested this, it makes sense to me. Don't forget to update the reviewer bit of the patch description.
Attachment #8391812 - Flags: review?(mconley)
Attachment #8391812 - Flags: review?(gijskruitbosch+bugs)
Attachment #8391812 - Flags: review+
Comment on attachment 8391812 [details] [diff] [review]
Part 1 v.2 - Don't tile LightweightTheme images on the selected tab - all platforms

Pushed Part 1 since it's still an improvement.
https://hg.mozilla.org/integration/fx-team/rev/4e685cd530ac
Attachment #8391812 - Flags: checkin+
Keywords: leave-open
Comment on attachment 8391812 [details] [diff] [review]
Part 1 v.2 - Don't tile LightweightTheme images on the selected tab - all platforms

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis LWT support
User impact if declined: If the users window is on screens of different resolutions (e.g. multiple monitors or changing the resolution of a single monitor), a selected tab near the left of the window may have misaligned theme remnants showing.
Testing completed (on m-c, etc.): Locally on XP, Win8 and 10.9
Risk to taking this patch (and alternatives if risky): Low risk CSS change only affecting users with lightweight themes applied
String or IDL/UUID changes made by this patch: None
Attachment #8391812 - Flags: approval-mozilla-aurora?
I still need to test this at the office with a dual monitor setup but it works locally when changing the resolution of my laptop's screen.
Attachment #8392083 - Flags: review?(dao)
Flags: needinfo?(MattN+bmo)
Attachment #8391812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8392083 [details] [diff] [review]
Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs

This works on my dual monitor setup at the office.
Attachment #8392083 - Flags: review?(ttaubert)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8392083 [details] [diff] [review]
Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs

Tim is away so Jared, can you take this?
Attachment #8392083 - Flags: review?(ttaubert)
Attachment #8392083 - Flags: review?(jaws)
Attachment #8392083 - Flags: review?(dao)
Comment on attachment 8392083 [details] [diff] [review]
Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs

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

::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +48,1 @@
>    _active: false,

Whoa, this is amazingly confusing. _active tells you if it is enabled? and enabled tells you if it is visible? Does it make sense to rename these variables s/_enabled/_displayedInWindow/ and s/_active/_enabled/ ?
Attachment #8392083 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> Comment on attachment 8392083 [details] [diff] [review]
> Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs
> 
> Review of attachment 8392083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/LightweightThemeConsumer.jsm
> @@ +48,1 @@
> >    _active: false,
> 
> Whoa, this is amazingly confusing. _active tells you if it is enabled? and
> enabled tells you if it is visible? Does it make sense to rename these
> variables s/_enabled/_displayedInWindow/ and s/_active/_enabled/ ?

IIRC there are add-ons that depend on the internals of our LWT implementation now. We ran into this at an earlier point of our Australis work, but I don't recall the bug offhand... :-(
> ::: toolkit/modules/LightweightThemeConsumer.jsm
> @@ +48,1 @@
> >    _active: false,
> 
> Whoa, this is amazingly confusing. _active tells you if it is enabled? and <snip/>

FYI, I filed bug 985950 about Splinter not including enough context.
(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> > Comment on attachment 8392083 [details] [diff] [review]
> > Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs
> > 
> > Review of attachment 8392083 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/modules/LightweightThemeConsumer.jsm
> > @@ +48,1 @@
> > >    _active: false,
> > 
> > Whoa, this is amazingly confusing. _active tells you if it is enabled? and
> > enabled tells you if it is visible? Does it make sense to rename these
> > variables s/_enabled/_displayedInWindow/ and s/_active/_enabled/ ?
> 
> IIRC there are add-ons that depend on the internals of our LWT
> implementation now. We ran into this at an earlier point of our Australis
> work, but I don't recall the bug offhand... :-(

Ok, that is less than good, but it does answer the question of "Does it make sense?" :) Thanks for the quick reply.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> ::: toolkit/modules/LightweightThemeConsumer.jsm
> @@ +48,1 @@
> >    _active: false,
> 
> Whoa, this is amazingly confusing. _active tells you if it is enabled? and
> enabled tells you if it is visible? Does it make sense to rename these
> variables s/_enabled/_displayedInWindow/ and s/_active/_enabled/ ?

Yeah, I added the comments because I was also confused. I didn't want to change the code so that we can uplift to beta with lower risk.
https://hg.mozilla.org/integration/fx-team/rev/87e9f48141ea
Keywords: leave-open
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Comment on attachment 8392083 [details] [diff] [review]
Part 2 - v.1.1 Notify lightweight-theme-optimized when optimizing occurs

Part 1 landed so this is for part 2.

[Approval Request Comment for part 2]
Bug caused by (feature/regressing bug #): bug 813786
User impact if declined: The behind the selected tab may appear when a theme is applied (see attachment 765122 [details]).
Testing completed (on m-c, etc.): Locally and m-c soon.
Risk to taking this patch (and alternatives if risky): Low risk mostly adding a observer notification and not changing the flow of existing code for the most part.
String or IDL/UUID changes made by this patch: None
Attachment #8392083 - Flags: approval-mozilla-beta?
Attachment #8392083 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/87e9f48141ea
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Attachment #8392083 - Flags: approval-mozilla-beta?
Attachment #8392083 - Flags: approval-mozilla-beta+
Attachment #8392083 - Flags: approval-mozilla-aurora?
Attachment #8392083 - Flags: approval-mozilla-aurora+
Keywords: verifyme
We don`t have this setup at the office, Gary can you please confirm that the issue is fixed on:
Firefox 29 beta 4, latest Nightly and Aurora?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b4-candidates/build1/mac/en-US/
http://nightly.mozilla.org/
http://www.mozilla.org/en-US/firefox/aurora/
Flags: needinfo?(gary)
Yes, this seems to work great now. Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Verified on 2014-03-31 nightly.
Gary, could you also confirm that this works fine on a Firefox 30 build (e.g. http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/30.0b1-candidates/)?
You need to log in before you can comment on or make changes to this bug.