Closed Bug 1075435 Opened 5 years ago Closed 5 years ago

Borders of lightweight themes don't adapt in customization mode

Categories

(Firefox :: Theme, defect)

x86
Windows 8
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified
firefox36 --- verified

People

(Reporter: phlsa, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(3 files)

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+
[Tracking Requested - why for this release]: The patch that caused the regression has been uplifted, so this fix should be tracking the same version.
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. :(
Attached image mockup-borders.png
Here's what it ideally should look like.
Flags: qe-verify?
Shouldn't this be part of an iteration? Right now it doesn't have that field set...
Flags: qe-verify? → qe-verify+
Was not added to the IT 35.3 priority list by Gavin/Chad/Madhava.
(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)
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.
(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?
(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)
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)
We can add this to the 35.3 priority list.
Flags: needinfo?(gavin.sharp)
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Points: --- → 2
Attached patch patchSplinter Review
Attachment #8506928 - Flags: review?(gijskruitbosch+bugs)
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)
(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...
(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!)
This patch implements "the second-best option" (comment 7, comment 8). This border is within the padding.
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+
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/b0bcc53c5d17
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Blocks: 1038990
No longer depends on: 1038990
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?
QA Contact: cornel.ionce
Verfied fixed on latest Nighly, build ID: 20141021030208.
Status: RESOLVED → VERIFIED
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+
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.