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)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 31
People
(Reporter: gkw, Assigned: MattN)
References
Details
(Keywords: reproducible, Whiteboard: [Australis:P3])
Attachments
(4 files, 1 obsolete file)
40.58 KB,
image/png
|
Details | |
88.49 KB,
image/png
|
Details | |
4.22 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
MattN
:
checkin+
|
Details | Diff | Splinter Review |
4.28 KB,
patch
|
jaws
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•11 years ago
|
||
Actually I just realized that it's related to cropping of lightweight themes.
Component: Graphics → Theme
Product: Core → Firefox
Assignee | ||
Comment 2•11 years ago
|
||
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 | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Here a link to LightweightThemeListener on the UX branch: https://hg.mozilla.org/projects/ux/file/69523ed020c3/browser/base/content/browser-addons.js#l419
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #765106 -
Flags: review?(dao) → review+
Updated•11 years ago
|
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
Matt, what needs to be done here still to finish it up?
Flags: needinfo?(mnoorenberghe+bmo)
Assignee | ||
Comment 8•11 years ago
|
||
Implement one of the options from comment 3.
Flags: needinfo?(MattN+bmo)
Updated•10 years ago
|
Blocks: australis-tabs-win
Updated•10 years ago
|
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P3]
Assignee | ||
Comment 11•10 years ago
|
||
I'm going to implement option 4 from comment 3 today.
Assignee | ||
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 15•10 years ago
|
||
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?
Assignee | ||
Comment 17•10 years ago
|
||
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)
Updated•10 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•10 years ago
|
Attachment #8391812 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 18•10 years ago
|
||
Part 1: https://hg.mozilla.org/releases/mozilla-aurora/rev/9570e3ec5c4b
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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 21•10 years ago
|
||
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+
Comment 22•10 years ago
|
||
(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... :-(
Comment 23•10 years ago
|
||
> ::: 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.
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/87e9f48141ea
Keywords: leave-open
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Assignee | ||
Comment 27•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
status-firefox31:
--- → fixed
Updated•10 years ago
|
Attachment #8392083 -
Flags: approval-mozilla-beta?
Attachment #8392083 -
Flags: approval-mozilla-beta+
Attachment #8392083 -
Flags: approval-mozilla-aurora?
Attachment #8392083 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 29•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ae1a0a5bd266
Assignee | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/81e1b9a7cfab
Comment 31•10 years ago
|
||
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)
Reporter | ||
Comment 32•10 years ago
|
||
Yes, this seems to work great now. Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Comment 34•10 years ago
|
||
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.
Description
•