Closed
Bug 1001871
Opened 11 years ago
Closed 11 years ago
Sidebar close button is offset to the left
Categories
(Firefox :: Theme, defect)
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)
109.77 KB,
image/png
|
Details | |
547 bytes,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
14.62 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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
Updated•11 years ago
|
Keywords: regression
Updated•11 years ago
|
Flags: firefox-backlog?
Updated•11 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Updated•11 years ago
|
tracking-firefox31:
--- → ?
Updated•11 years ago
|
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
(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?
Assignee | ||
Updated•11 years ago
|
Component: Tabbed Browser → Theme
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
Gijs, could you have a look to this issue? 31 is going to move to beta next Monday. Thanks
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 7•11 years ago
|
||
(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
Assignee | ||
Comment 8•11 years ago
|
||
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
Reporter | ||
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8436355 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8436357 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 14•11 years ago
|
||
Marco, please add this to the current iteration.
Flags: needinfo?(mmucci)
Whiteboard: p=3
Assignee | ||
Updated•11 years ago
|
Whiteboard: p=3 → p=3 [qa+]
Updated•11 years ago
|
Attachment #8436357 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 15•11 years ago
|
||
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.
Assignee | ||
Comment 16•11 years ago
|
||
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?
Assignee | ||
Comment 17•11 years ago
|
||
(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•11 years ago
|
||
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: p=3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
Comment 19•11 years ago
|
||
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+
Updated•11 years ago
|
Keywords: addon-compat
Comment 20•11 years ago
|
||
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+
Comment 21•11 years ago
|
||
Comment on attachment 8436357 [details] [diff] [review]
branch patch
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/2cbd6e22ca96
Attachment #8436357 -
Flags: checkin+
Updated•11 years ago
|
status-firefox30:
--- → unaffected
status-firefox33:
--- → affected
Updated•11 years ago
|
QA Contact: florin.mezei
Updated•11 years ago
|
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 s=33.1 [qa+]
Updated•11 years ago
|
QA Contact: florin.mezei → cornel.ionce
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #8436355 -
Attachment is obsolete: true
Attachment #8439154 -
Flags: review?(gijskruitbosch+bugs)
Comment 24•11 years ago
|
||
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)
Assignee | ||
Comment 25•11 years ago
|
||
(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)
Comment 26•11 years ago
|
||
(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)
Comment 27•11 years ago
|
||
(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)
Assignee | ||
Comment 28•11 years ago
|
||
(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 29•11 years ago
|
||
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+
Assignee | ||
Comment 30•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment 32•11 years ago
|
||
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!]
You need to log in
before you can comment on or make changes to this bug.
Description
•