1001871 - Sidebar close button is offset to the left

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

Bug 865826 caused a couple of (I consider them serious) visual regressions.

a/browser/themes/osx/browser.css:
> sidebarheader > .tabs-closebutton > .toolbarbutton-text {
>   display: none;
> }
Removing this bit causes the label element of the sidebar close button in to be visible (even if empty), moving the close button (12px!) to the left. This rule should stay in the stylesheet.

a/browser/themes/windows/browser.css:
> .tabs-closebutton > .toolbarbutton-icon {
>   -moz-margin-end: 0px !important;
>   -moz-padding-end: 2px !important;
>   -moz-padding-start: 2px !important;
> }
Removing this bit causes the sidebar close button to become narrower in windows, and thus harder to hit. This rule should also stay, and a similar rule could also be added for the linux stylesheet, although in that case it's not technically a regression.

Dao, CC'ing you directly because you were assigned bug 865826.

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

And since we're at it, I don't think this is needed anymore:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4123

Comment 2

[Matthew N. [:MattN]]

The sidebar close button is also now double the expected size on HiDPI OS X but I'm not sure if bug 865826  caused that too.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Luís Miguel [:Quicksaver] from comment #0)
> Bug 865826 caused a couple of (I consider them serious) visual regressions.
> 
> a/browser/themes/osx/browser.css:
> > sidebarheader > .tabs-closebutton > .toolbarbutton-text {
> >   display: none;
> > }
> Removing this bit causes the label element of the sidebar close button in to
> be visible (even if empty), moving the close button (12px!) to the left.
> This rule should stay in the stylesheet.

This shouldn't be needed, as it's not needed on Linux or Windows. Can somebody figure out what's messing this up on OS X?

Comment 4

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (away May 9 - 12) from comment #2)
> The sidebar close button is also now double the expected size on HiDPI OS X
> but I'm not sure if bug 865826  caused that too.

Ehsan filed this as bug 1008610. It certainly isn't the case on 29 or 30, so I expect this is related.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Matthew N. [:MattN] (away May 9 - 12) from comment #2)
> > The sidebar close button is also now double the expected size on HiDPI OS X
> > but I'm not sure if bug 865826  caused that too.
> 
> Ehsan filed this as bug 1008610. It certainly isn't the case on 29 or 30, so
> I expect this is related.

Egh, copy/paste fail. Bug 1008594.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Gijs, could you have a look to this issue? 31 is going to move to beta next Monday. Thanks

Comment 7

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Luís Miguel [:Quicksaver] from comment #1)
> And since we're at it, I don't think this is needed anymore:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#4123

(I had completely forgotten about this bug, and also about what I meant to point at with that link. I had to go through the file again to make sure...)

This meant to point to the mTabstripCloseButton property: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4154

Comment 8

[Dão Gottwald [:dao]]

Let's please have one issue per bug.

Specifically, this is the issue I think we should be tracking this bug for:

(In reply to Luís Miguel [:Quicksaver] from comment #0)
> a/browser/themes/osx/browser.css:
> > sidebarheader > .tabs-closebutton > .toolbarbutton-text {
> >   display: none;
> > }
> Removing this bit causes the label element of the sidebar close button in to
> be visible (even if empty), moving the close button (12px!) to the left.
> This rule should stay in the stylesheet.

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> Gijs, could you have a look to this issue? 31 is going to move to beta next
> Monday. Thanks

Trying to answer my question from comment 3 would be particularly helpful.

Comment 9

[Luís Miguel [:quicksaver] (not reading bugmail)]

Attachment: Comparison of the sidebar close button before and after bug 865826

(In reply to Dão Gottwald [:dao] from comment #3)
> > a/browser/themes/osx/browser.css:
> > > sidebarheader > .tabs-closebutton > .toolbarbutton-text {
> > >   display: none;
> > > }
> This shouldn't be needed, as it's not needed on Linux or Windows. Can
> somebody figure out what's messing this up on OS X?

In Windows and Linux this is happening:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#229
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#224
>  description,
>  label {
>    cursor: default;
>    margin-top: 1px;
>    margin-bottom: 2px;
>    -moz-margin-start: 6px;
>    -moz-margin-end: 5px;
>  }
is overruled by
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#29
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#31
>  .toolbarbutton-text {
>    margin: 0 !important;
>    text-align: center;
>  }

In OS X the opposite happens:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#23
>  .toolbarbutton-text {
>    margin: 0px;
>    padding: 0px;
>    text-align: center;
>    vertical-align: middle;
>  }
is overruled by
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/global.css#199
>  label {
>    margin: 2px 6px;
>  }

So, in OS X the sidebar close button label has a visible margin now, even though the label itself is empty, which shifts the close button to the left.

> Specifically, this is the issue I think we should be tracking this bug for:
> 
> (In reply to Luís Miguel [:Quicksaver] from comment #0)

I disagree with this. As you can see from the screenshot, bug 865826 also clearly affected the sidebar close button in windows. Unless you meant that you'd rather separate this bug by OS, and if so I'll be glad to open a new bug for windows then.

Also, what I meant with comment 1 and comment 7 was more along a clean-up after bug 865826, just like these visual regressions are "clean-up" as well, sort of. But I'd also be glad to open a new bug just for that in case you still think it doesn't fit here.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Luís Miguel [:Quicksaver] from comment #9)
> In Windows and Linux this is happening:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> global.css#229
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> global.css#224
> >  description,
> >  label {
> >    cursor: default;
> >    margin-top: 1px;
> >    margin-bottom: 2px;
> >    -moz-margin-start: 6px;
> >    -moz-margin-end: 5px;
> >  }
> is overruled by
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> toolbarbutton.css#29
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> toolbarbutton.css#31
> >  .toolbarbutton-text {
> >    margin: 0 !important;
> >    text-align: center;
> >  }
> 
> In OS X the opposite happens:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> toolbarbutton.css#23
> >  .toolbarbutton-text {
> >    margin: 0px;
> >    padding: 0px;
> >    text-align: center;
> >    vertical-align: middle;
> >  }
> is overruled by
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> global.css#199
> >  label {
> >    margin: 2px 6px;
> >  }

Ok, so it seems like we need to use !important on OS X like we're doing on Windows. Thanks for investigating!

> > Specifically, this is the issue I think we should be tracking this bug for:
> 
> I disagree with this. As you can see from the screenshot, bug 865826 also
> clearly affected the sidebar close button in windows. Unless you meant that
> you'd rather separate this bug by OS, and if so I'll be glad to open a new
> bug for windows then.

Yes, it belongs in a separate bug, not because a different OS is affected but because it's a separate issue.

> Also, what I meant with comment 1 and comment 7 was more along a clean-up
> after bug 865826, just like these visual regressions are "clean-up" as well,
> sort of. But I'd also be glad to open a new bug just for that in case you
> still think it doesn't fit here.

Indeed, it doesn't fit here at all.

Comment 11

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Dão Gottwald [:dao] from comment #10)
> Thanks for investigating!

No problem. I should have done that long ago, I'm sorry about that, this completely vanished from my mind.

Also, filed bug 1022211 and bug 1022214.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

Comment 13

[Dão Gottwald [:dao]]

Attachment: branch patch

Comment 14

[Dão Gottwald [:dao]]

Marco, please add this to the current iteration.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8436355 [details] [diff] [review]
patch

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

I won't be able to review this tonight as it's a lot more involved; I'll try to look at it tomorrow, but it might not be done until Monday - sorry.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8436357 [details] [diff] [review]
branch patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 865826
User impact if declined: visual glitch
Testing completed (on m-c, etc.): nope (this is a different, less risky patch than what I want to land on mozilla-central)
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 17

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 8436355 [details] [diff] [review]
> patch
> 
> Review of attachment 8436355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I won't be able to review this tonight as it's a lot more involved; I'll try
> to look at it tomorrow, but it might not be done until Monday - sorry.

Feel free to land the patch if you r+ it before the uplift, as I probably won't be around.

Comment 18

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 19

[:Gijs (he/him)]

Comment on attachment 8436355 [details] [diff] [review]
patch

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

I'm digging all the extra rule removal, but there are some issues with the patch as-is, so f+ for now.

::: browser/themes/osx/browser.css
@@ -235,5 @@
>  
>  .bookmark-item > .toolbarbutton-text,
>  #personal-bookmarks[cui-areatype="toolbar"] > #bookmarks-toolbar-placeholder > .toolbarbutton-text {
>    display: -moz-box !important; /* Force the display of the label for bookmarks */
> -  margin: 0 !important;

This overrode the panelUIOverlay.css styling for subviewbuttons with .bookmark-item, iow, bookmarks in the bookmarks menu button dropdown. Which means they now get a margin that wasn't there before, increasing the size of the menu items. That shouldn't happen.

http://imgur.com/U3YcvYF,XCn79A6

::: browser/themes/osx/places/organizer.css
@@ -87,5 @@
>    margin: 1px 4px;
>  }
>  
> -#placesToolbar > toolbarbutton > .toolbarbutton-text {
> -  display: none;

This doesn't work, sadly, because the buttons in the #placesToolbar have labels:

http://imgur.com/tPMX13F

::: toolkit/themes/osx/global/toolbarbutton.css
@@ +20,5 @@
>    text-shadow: none;
>  }
>  
>  .toolbarbutton-text {
> +  margin: 0 !important; /* !important for overriding global.css */

So unfortunately there are things that depend on that override happening. The findbar text buttons, for instance (highlight all / match case) are really cramped after this patch, having gone from margin: 2px 6px; to margin: 0...

DOM Inspector's 'inspect' and 'find' buttons suddenly also shrink (although that's arguably a feature rather than a bug). But it's clear that there's add-on impact, so we should at least document this.

I've not seen anything else besides what's documented in this review, but it's possible I missed something. Probably worth having an extra QA check after executing this change.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8436357 [details] [diff] [review]
branch patch

Checked with Gijs on IRC. We approve this patch for 31 and they work on a better fix for 30.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8436357 [details] [diff] [review]
branch patch

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2cbd6e22ca96

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e233208a0f2

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 24

[:Gijs (he/him)]

Comment on attachment 8439154 [details] [diff] [review]
patch v2

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

Almost there, but there's still the issue with the bookmark menu.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +79,5 @@
>  }
>  
>  .subviewbutton > .toolbarbutton-text,
>  .subviewbutton > .menu-iconic-text {
> +  margin: 2px 0 !important;

Because these are now !important, they still override the .bookmark-item margin: 0 styles, and so the menuitems are still too big.

If we need to keep these (which I suspect, with a still more specific rule for the menu, but I haven't checked for certain) then we should add a comment here, too, explaining why we need to use !important.

Comment 25

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #24)
> ::: browser/themes/osx/customizableui/panelUIOverlay.css
> @@ +79,5 @@
> >  }
> >  
> >  .subviewbutton > .toolbarbutton-text,
> >  .subviewbutton > .menu-iconic-text {
> > +  margin: 2px 0 !important;
> 
> Because these are now !important, they still override the .bookmark-item
> margin: 0 styles, and so the menuitems are still too big.

I don't understand what you think the expected result is here. This code in panelUIOverlay.css explicitly sets a margin for .bookmark-item:

.subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) > .toolbarbutton-text {
  margin: 2px 6px;
}

I don't see why margin should be 0 for these. .bookmark-item > .toolbarbutton-text in browser.css likely only wants to target buttons on the toolbar, since that's the only type of elements this selector used to match until Australis broke this assumption.

Comment 26

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to :Gijs Kruitbosch from comment #24)
> > ::: browser/themes/osx/customizableui/panelUIOverlay.css
> > @@ +79,5 @@
> > >  }
> > >  
> > >  .subviewbutton > .toolbarbutton-text,
> > >  .subviewbutton > .menu-iconic-text {
> > > +  margin: 2px 0 !important;
> > 
> > Because these are now !important, they still override the .bookmark-item
> > margin: 0 styles, and so the menuitems are still too big.
> 
> I don't understand what you think the expected result is here. This code in
> panelUIOverlay.css explicitly sets a margin for .bookmark-item:
> 
> .subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) >
> .toolbarbutton-text {
>   margin: 2px 6px;
> }

Sorry, I was misled and plead innocence. However, this is more confusing than I previously thought. The removal of the margin: 0 !important for bookmark-item was probably correct (although it's hard for me to check right now).

The rule you cite doesn't affect the items I'm complaining about, because the items in the bookmarks menu button are menu items, so they don't have a .toolbarbutton-text.

They do have a menu-iconic-text, which gets set to margin: 0 with !important here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/menu.css#26 .

The .menu-iconic-text rule that I originally cited here didn't impact them at all before because it was without !important, but you added !important so now it does. I /think/ that the menu-iconic-text half of the selector in this rule can safely be removed, but I'm not 100% sure.

Does that clarify?

Also, are you writing these patches blind? This would probably be simpler if you had a mac to test on...

Comment 27

[:Gijs (he/him)]

(also note that the toolbarbutton-text case still applies, even for the bookmarks menu button's subview, when you move the bookmarks menu to the panel... because it's the bookmarks menu button and it's several levels of crazy)

Comment 28

[Dão Gottwald [:dao]]

Attachment: patch v3

(In reply to :Gijs Kruitbosch from comment #26)
> Does that clarify?

Yep.

> Also, are you writing these patches blind? This would probably be simpler if
> you had a mac to test on...

As far as OS X is concerned, yes.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8439823 [details] [diff] [review]
patch v3

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

As best I can tell, this works perfectly. Thanks!

Comment 30

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/e2b3ecf5916b

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2b3ecf5916b

Comment 32

[Andrei Vaida [:avaida]]

This issue is verified fixed on:
 * Nightly 33 (2014-06-16),
 * Aurora 32 (2014-06-16),
 * Firefox 31 Beta 1 (Build ID: 20140610163407),
using the following platforms:
 * Mac OS X 10.9.2 [1],
 * Mac OS X 10.8.5 [2],
 * Mac OS X 10.7.5 [3].

[1] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:33.0) Gecko/20100101 Firefox/33.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:33.0) Gecko/20100101 Firefox/33.0