Closed Bug 1001871 Opened 6 years ago Closed 5 years ago

Sidebar close button is offset to the left

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox30 --- unaffected
firefox31 + verified
firefox32 --- verified
firefox33 --- verified

People

(Reporter: quicksaver, Assigned: dao)

References

Details

(Keywords: addon-compat, regression, Whiteboard: p=3 s=33.1 [qa!])

Attachments

(3 files, 2 obsolete files)

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.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
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.
(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?
Component: Tabbed Browser → Theme
(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.
(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.
Gijs, could you have a look to this issue? 31 is going to move to beta next Monday. Thanks
Flags: needinfo?(gijskruitbosch+bugs)
(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
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.
OS: All → Mac OS X
Summary: Appearance regressions in the sidebar caused by bug 865826 → Sidebar close button is offset to the left
(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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8436355 - Flags: review?(gijskruitbosch+bugs)
Attached patch branch patchSplinter Review
Attachment #8436357 - Flags: review?(gijskruitbosch+bugs)
Marco, please add this to the current iteration.
Flags: needinfo?(mmucci)
Whiteboard: p=3
Whiteboard: p=3 → p=3 [qa+]
Attachment #8436357 - Flags: review?(gijskruitbosch+bugs) → review+
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 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
Attachment #8436357 - Flags: approval-mozilla-aurora?
(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.
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: p=3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
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.
Attachment #8436355 - Flags: review?(gijskruitbosch+bugs) → feedback+
Keywords: addon-compat
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.
Attachment #8436357 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: florin.mezei
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 s=33.1 [qa+]
QA Contact: florin.mezei → cornel.ionce
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8436355 - Attachment is obsolete: true
Attachment #8439154 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8439154 - Flags: review?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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...
Flags: needinfo?(gijskruitbosch+bugs)
(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)
Attached patch patch v3Splinter Review
(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.
Attachment #8439154 - Attachment is obsolete: true
Attachment #8439823 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8439823 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e2b3ecf5916b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=33.1 [qa+] → p=3 s=33.1 [qa!]
Duplicate of this bug: 412180
You need to log in before you can comment on or make changes to this bug.