Closed
Bug 1075435
Opened 10 years ago
Closed 10 years ago
Borders of lightweight themes don't adapt in customization mode
Categories
(Firefox :: Theme, defect)
Tracking
()
People
(Reporter: phlsa, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(3 files)
288.38 KB,
image/png
|
Details | |
279.72 KB,
image/png
|
Details | |
1.43 KB,
patch
|
Gijs
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The borders of lightweight themes added in bug 1038990 move inwards when in customization mode. They should stay near the edge of the window at all times.
Flags: firefox-backlog+
Reporter | ||
Comment 1•10 years ago
|
||
[Tracking Requested - why for this release]: The patch that caused the regression has been uplifted, so this fix should be tracking the same version.
status-firefox34:
--- → affected
status-firefox35:
--- → affected
tracking-firefox34:
--- → ?
tracking-firefox35:
--- → ?
Assignee | ||
Comment 2•10 years ago
|
||
Note that the border added in bug 1038990 lines up with the background gradient and with the top border on the navigation toolbar as well as with the left and right borders around the navigation toolbar and content area. It's not clear to me how else this is supposed to look. The padding we add around the window in customization mode keeps being a permanent maintenance cost. It's annoying. :(
Reporter | ||
Comment 3•10 years ago
|
||
Here's what it ideally should look like.
Updated•10 years ago
|
Flags: qe-verify?
Comment 4•10 years ago
|
||
Shouldn't this be part of an iteration? Right now it doesn't have that field set...
Flags: qe-verify? → qe-verify+
Comment 5•10 years ago
|
||
Was not added to the IT 35.3 priority list by Gavin/Chad/Madhava.
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to Marco Mucci [:MarcoM] from comment #5) > Was not added to the IT 35.3 priority list by Gavin/Chad/Madhava. I noticed and filed the bug very shortly before the planning meeting started, so they had no chance to add it. Since it regresses customization mode, it should be in though. The (very undesirable) alternative would be backing out bug 1038990.
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 7•10 years ago
|
||
I don't think it's necessary to back out bug 1038990 (which is supposed to improve the look when the user is actually using the browser) because of this glitch (which only affects customization mode)... especially since backing it out won't give you attachment 8498129 [details] either. If you just want the regression fixed, I can easily remove the border added by bug 1038990 in customization mode.
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #7) > If you > just want the regression fixed, I can easily remove the border added by bug > 1038990 in customization mode. Yeah, that would be the second-best option. On the other hand, we hope that lots of users will be setting their themes in customization mode, so this would be their first experience with LWTs. So it would still be (greatly) preferable to keep the borders near the edges of the windows in customization mode, like in the mockup. Can you tell off-hand how much work (i.e. points) that would be?
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #8) > Can you tell off-hand how much work (i.e. points) that would be? I don't know off-hand, maybe Gijs does since he implemented showing lightweight themes in customization mode and probably was more involved with implementing the padding around the window than I was.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 10•10 years ago
|
||
Hm, well, there are two options... One is you could try to switch the toolbox to use padding instead of margin again, but we used to use padding on #tab-view-deck and switched to margins in our quest for perf, so there be CART dragons. The other is that you might be able to change the code added here: https://hg.mozilla.org/mozilla-central/rev/261ac0a51324#l2.160 to layer in the borders as separate background images (ensuring, of course, that their positions are correct). Neither of these seem like much fun, but the second seems workable? Maybe?
Flags: needinfo?(gijskruitbosch+bugs)
Updated•10 years ago
|
Comment 12•10 years ago
|
||
Added to 35.3 priority list. (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #11) > We can add this to the 35.3 priority list.
Updated•10 years ago
|
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Points: --- → 2
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8506928 -
Flags: review?(gijskruitbosch+bugs)
Comment 14•10 years ago
|
||
Comment on attachment 8506928 [details] [diff] [review] patch Review of attachment 8506928 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/windows/browser-aero.css @@ +168,5 @@ > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:not(:-moz-lwtheme)::after, > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme), > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:-moz-lwtheme, > + #main-window[customizing] #navigator-toolbox::after, > + #main-window[customizing] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar) { No sizemode normal here? That seems off. Note that I can't easily test this until Sunday, because I'm away from my Windows machine... I can see about finding a victi...err, I mean colleague, whose machine I can borrow for five minutes (*cough* Matt *cough*) though.
Attachment #8506928 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #14) > Comment on attachment 8506928 [details] [diff] [review] > patch > > Review of attachment 8506928 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/windows/browser-aero.css > @@ +168,5 @@ > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:not(:-moz-lwtheme)::after, > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme), > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:-moz-lwtheme, > > + #main-window[customizing] #navigator-toolbox::after, > > + #main-window[customizing] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar) { > > No sizemode normal here? That seems off. The toolbars won't touch the window border in customize mode regardless of the sizemode, thanks to the massive padding added there, so...
Comment 16•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #15) > (In reply to :Gijs Kruitbosch from comment #14) > > Comment on attachment 8506928 [details] [diff] [review] > > patch > > > > Review of attachment 8506928 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/themes/windows/browser-aero.css > > @@ +168,5 @@ > > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:not(:-moz-lwtheme)::after, > > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme), > > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:-moz-lwtheme, > > > + #main-window[customizing] #navigator-toolbox::after, > > > + #main-window[customizing] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar) { > > > > No sizemode normal here? That seems off. > > The toolbars won't touch the window border in customize mode regardless of > the sizemode, thanks to the massive padding added there, so... I'm confused. AIUI this bug wants borders on the outside of the massive padding. I assumed that's what the patch is trying to accomplish. But there's no window edge painted in maximized or fullscreen mode, and this patch would add one just in customize mode, right? (again, this is "dry" reading the patch because I can't test right now, so I might just be misunderstanding!)
Assignee | ||
Comment 17•10 years ago
|
||
This patch implements "the second-best option" (comment 7, comment 8). This border is within the padding.
Comment 18•10 years ago
|
||
Comment on attachment 8506928 [details] [diff] [review] patch Review of attachment 8506928 [details] [diff] [review]: ----------------------------------------------------------------- OK. This makes more sense to me now. However, I still have a question... > #main-window[customizing] #navigator-toolbox::after, Testing on Windows 7, I don't see why we have this selector. It seems like ::after is always a 1px high border between the navbar and the tabstrip. Why does it need a side border? Ditto for the top one below. I'm probably missing something else now; r=me either with or without those bits, but preferably with an explanation why we need it if it stays in. :-) ::: browser/themes/windows/browser-aero.css @@ +166,5 @@ > > /* Vertical toolbar border */ > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:not(:-moz-lwtheme)::after, > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme), > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:-moz-lwtheme, Nit: generally we put the :not clauses after the [attribute=foo] clauses, I think?
Attachment #8506928 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #18) > > #main-window[customizing] #navigator-toolbox::after, > > Testing on Windows 7, I don't see why we have this selector. It seems like > ::after is always a 1px high border between the navbar and the tabstrip. Why > does it need a side border? Ditto for the top one below. > > I'm probably missing something else now; r=me either with or without those > bits, but preferably with an explanation why we need it if it stays in. :-) It may be redundant, but I'm not entirely sure. Need to test. Since that 1px side border is already there for the non-customizing case and I'm just being consistent here, I'd rather land this as is and file a new bug if needed. > > /* Vertical toolbar border */ > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:not(:-moz-lwtheme)::after, > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(:-moz-lwtheme), > > + #main-window:not([customizing])[sizemode=normal] #navigator-toolbox:-moz-lwtheme, > > Nit: generally we put the :not clauses after the [attribute=foo] clauses, I > think? Most of the time, not always. In this case I flipped it because having :not([customizing]) and [customizing] line up makes the five selectors are easier to scan and understand.
Comment 20•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #19) > (In reply to :Gijs Kruitbosch from comment #18) > > > #main-window[customizing] #navigator-toolbox::after, > > > > Testing on Windows 7, I don't see why we have this selector. It seems like > > ::after is always a 1px high border between the navbar and the tabstrip. Why > > does it need a side border? Ditto for the top one below. > > > > I'm probably missing something else now; r=me either with or without those > > bits, but preferably with an explanation why we need it if it stays in. :-) > > It may be redundant, but I'm not entirely sure. Need to test. Since that 1px > side border is already there for the non-customizing case and I'm just being > consistent here, I'd rather land this as is and file a new bug if needed. Sounds good, esp. if we need to uplift this for 34.
Assignee | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b0bcc53c5d17
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #18) > Comment on attachment 8506928 [details] [diff] [review] > patch > > Review of attachment 8506928 [details] [diff] [review]: > ----------------------------------------------------------------- > > OK. This makes more sense to me now. However, I still have a question... > > > #main-window[customizing] #navigator-toolbox::after, > > Testing on Windows 7, I don't see why we have this selector. It seems like > ::after is always a 1px high border between the navbar and the tabstrip. Why > does it need a side border? Ditto for the top one below. This is needed because #navigator-toolbox::after's fully opaque background color is different from the side border's semitransparent color.
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0bcc53c5d17
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8506928 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]: bug 1038990 [User impact if declined]: visual glitch in customization mode [Describe test coverage new/current, TBPL]: n/a [Risks and why]: low; small CSS-only fix [String/UUID change made/needed]: none
Attachment #8506928 -
Flags: approval-mozilla-beta?
Attachment #8506928 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
QA Contact: cornel.ionce
Comment 25•10 years ago
|
||
Verfied fixed on latest Nighly, build ID: 20141021030208.
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
status-firefox33:
--- → unaffected
status-firefox36:
--- → fixed
Comment 26•10 years ago
|
||
Comment on attachment 8506928 [details] [diff] [review] patch Beta+ Aurora+
Attachment #8506928 -
Flags: approval-mozilla-beta?
Attachment #8506928 -
Flags: approval-mozilla-beta+
Attachment #8506928 -
Flags: approval-mozilla-aurora?
Attachment #8506928 -
Flags: approval-mozilla-aurora+
Comment 27•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b5a0e9f618a6 https://hg.mozilla.org/releases/mozilla-beta/rev/e57353855abf
Updated•10 years ago
|
Comment 28•10 years ago
|
||
Also confirming this fix on: - Firefox 35.0a2, build ID: 20141116004002; - Firefox 34 beta 9, build ID: 20141114133026.
You need to log in
before you can comment on or make changes to this bug.
Description
•