Closed Bug 1182102 Opened 5 years ago Closed 5 years ago

Stop setting the bookmark-item class on the home button or the bookmarks button when moving them to the bookmarks toolbar

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
star-icons.png is overloaded with different stars for different contexts. There's no value in using it for #bookmarks-menu-button.bookmark-item. It's smaller (16*16) than the star we otherwise use for that button (18*18), but that doesn't actually make a meaningful difference.

I'll do the same for Windows in bug 1174122.
Attachment #8631608 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8631608 [details] [diff] [review]
patch

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

This makes the star disappear when the button is on the bookmarks toolbar, so r-.

I also don't really understand what this patch is trying to accomplish. It seems like at best we'll get a blurry/badly-scaled icon on the bookmarks star button, for no gain except the clarity of the file name and/or slight code simplification. That doesn't seem like a sensible trade-off. Why are we trying to do this?
Attachment #8631608 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs Kruitbosch from comment #1)
> Comment on attachment 8631608 [details] [diff] [review]
> patch
> 
> Review of attachment 8631608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes the star disappear when the button is on the bookmarks toolbar,
> so r-.

On OS X? Pretty sure this worked for me as expected on Linux and Windows. It should get the icon from the first rule in toolbarbuttons.inc.css.

> I also don't really understand what this patch is trying to accomplish.

It's a simplification pretty much in every aspect. What we're doing right now is just pointless and leads to be the icon using the wrong color because star-icons.png doesn't come in as many versions as Toolbar.png.

> It
> seems like at best we'll get a blurry/badly-scaled icon on the bookmarks
> star button,

Um, no. We don't target this icon when constraining the size of bookmark item icons. The icon from Toolbar.png should be used at the original size, just like the icon used for the bookmarks menu dropdown.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Comment on attachment 8631608 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8631608 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This makes the star disappear when the button is on the bookmarks toolbar,
> > so r-.
> 
> On OS X?

Yes. Clobber build, too.

> > It
> > seems like at best we'll get a blurry/badly-scaled icon on the bookmarks
> > star button,
> 
> Um, no. We don't target this icon when constraining the size of bookmark
> item icons.

I'm not sure what you mean by this.

> The icon from Toolbar.png should be used at the original size,
> just like the icon used for the bookmarks menu dropdown.

This doesn't make sense to me. All the icons in the bookmarks toolbar are 16x16px. Using the icon at 18x18px changes the height of the item and/or bookmarks toolbar, last I checked.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > > This makes the star disappear when the button is on the bookmarks toolbar,
> > > so r-.
> > 
> > On OS X?
> 
> Yes. Clobber build, too.

Could you please open DOM Inspector and check what's overriding toolbarbuttons.inc.css or setting a wrong image region on OS X?

> > > It
> > > seems like at best we'll get a blurry/badly-scaled icon on the bookmarks
> > > star button,
> > 
> > Um, no. We don't target this icon when constraining the size of bookmark
> > item icons.
> 
> I'm not sure what you mean by this.

We size .bookmark-item icons to 16*16px (hence your concern, I imagine), but that rule doesn't apply for this button. So this icon shouldn't get scaled.

> > The icon from Toolbar.png should be used at the original size,
> > just like the icon used for the bookmarks menu dropdown.
> 
> This doesn't make sense to me. All the icons in the bookmarks toolbar are
> 16x16px.

No, see above. Unless I'm missing another legacy rule doing this on OS X.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to Dão Gottwald [:dao] from comment #2)
> > > > This makes the star disappear when the button is on the bookmarks toolbar,
> > > > so r-.
> > > 
> > > On OS X?
> > 
> > Yes. Clobber build, too.
> 
> Could you please open DOM Inspector and check what's overriding
> toolbarbuttons.inc.css or setting a wrong image region on OS X?

It gets the defaultFavicon@2x.png image (http://hg.mozilla.org/mozilla-central/annotate/2c91d57441fd/browser/themes/osx/browser.css#l2546 and further), with the region for Toolbar@2x.png, which means the result is emptiness.

> > > > It
> > > > seems like at best we'll get a blurry/badly-scaled icon on the bookmarks
> > > > star button,
> > > 
> > > Um, no. We don't target this icon when constraining the size of bookmark
> > > item icons.
> > 
> > I'm not sure what you mean by this.
> 
> We size .bookmark-item icons to 16*16px (hence your concern, I imagine), but
> that rule doesn't apply for this button. So this icon shouldn't get scaled.

Your patch removes the rule that sizes the icon. That said, on OS X, the button being on the toolbar still increases the toolbar size, even without your patch, because of this rule:

http://hg.mozilla.org/mozilla-central/annotate/2c91d57441fd/browser/themes/osx/browser.css#l593

which is a bug. In fact, thinking about it, IIRC the button is supposed to not have .toolbarbutton-1 when on the bookmarks toolbar, which should be taken care of by some of the places JS. I don't know why that doesn't seem to be working for me.
Flags: needinfo?(gijskruitbosch+bugs)
We should probably just stop setting the bookmark-item class when moving that button (or the home button) on the personal toolbar. It's pretty arbitrary, we don't do this for other buttons. And we have enough evidence that it's quite a mess. The situation on OS X is even worse than I expected.
Note that I originally introduced this behavior for the home button because we wanted to move it to the personal toolbar /by default/. We ended up not doing that, so the class switching is mostly leftover.
Component: Theme → Toolbars and Customization
Summary: Stop using star-icons.png for #bookmarks-menu-button.bookmark-item and rename it appropriately → Stop setting the bookmark-item class on the home button or the bookmarks button when moving them to the bookmarks toolbar
Attached patch patch (obsolete) — Splinter Review
Attachment #8631608 - Attachment is obsolete: true
Attachment #8647392 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8647392 [details] [diff] [review]
patch

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

You will need to remove the class persistence from browser.xul for these buttons, and add a migration to remove any existing persisted classes. Right now the class still persists on profiles that had the button on the bookmarks toolbar already, which makes them look broken because of all the now-changed rules.
Attachment #8647392 - Flags: review?(gijskruitbosch+bugs)
When we're done here, we should wontfix bug 1014231.
Can we file a followup to fix the fact that these buttons are now enlarging the bookmarks toolbar when placed on it, and are making the bookmarks hover styles look wonky?
(In reply to :Gijs Kruitbosch from comment #11)
> Can we file a followup to fix the fact that these buttons are now enlarging
> the bookmarks toolbar when placed on it, and are making the bookmarks hover
> styles look wonky?

Sure, but it's not clear to me what you'd want to do about these buttons enlarging the toolbar. This should already apply to the vast majority of buttons that we haven't special-cased. Generally it seems like a feature, not a bug, that toolbars grow for larger content.
(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > Can we file a followup to fix the fact that these buttons are now enlarging
> > the bookmarks toolbar when placed on it, and are making the bookmarks hover
> > styles look wonky?
> 
> Sure, but it's not clear to me what you'd want to do about these buttons
> enlarging the toolbar. This should already apply to the vast majority of
> buttons that we haven't special-cased. Generally it seems like a feature,
> not a bug, that toolbars grow for larger content.

I think the logic is/was that the bookmarks + home button are much more likely to be moved to the bookmarks toolbar because they are related to bookmarks (click bookmark vs. click home button, using bookmarks button to bookmark pages, using the bookmarks menu button to access non-toolbar bookmarks). The same can't be said for (most?) other buttons.

I don't mind dropping that, though I don't know how Marco feels about it.

Even if we drop it, though, IMO we should try to ensure that the bookmarks buttons themselves continue looking nice - which at that point we might as well do with bug 734326, which is long overdue for fixing.
(as for what to do - use smaller icons is what we've done previously...)
(In reply to :Gijs Kruitbosch from comment #13)
> I think the logic is/was that the bookmarks + home button are much more
> likely to be moved to the bookmarks toolbar because they are related to
> bookmarks (click bookmark vs. click home button, using bookmarks button to
> bookmark pages, using the bookmarks menu button to access non-toolbar
> bookmarks). The same can't be said for (most?) other buttons.

The home button's story is that we moved it onto the bookmarks toolbar by default, but then we reverted that. As for the the bookmarks button, I think in of its incarnations we automatically moved it to the bookmarks toolbar when enabling that, but this behavior is gone too.

> Even if we drop it, though, IMO we should try to ensure that the bookmarks
> buttons themselves continue looking nice

Yes, sure, especially if the hover state is broken like you said. Obviously this shouldn't happen.
Attached patch patch v2 (obsolete) — Splinter Review
Still building (had to clobber), but seems straightforward enough that I'm already requesting review.
Attachment #8647392 - Attachment is obsolete: true
Attachment #8647548 - Flags: review?(gijskruitbosch+bugs)
Attached patch patch v2 (obsolete) — Splinter Review
updated to fx-team tip
Attachment #8647548 - Attachment is obsolete: true
Attachment #8647548 - Flags: review?(gijskruitbosch+bugs)
Attachment #8647573 - Flags: review?(gijskruitbosch+bugs)
I don't have strong opinions, provided things continue working and don't look too fancy. So don't worry about me blocking what you think is worth to do :)
Dao is right that at certain points we wanted to move both home and bookmarks button to the toolbar and this was part of that.
Moving the bookmarks button could still make sense for some users who want to keep the contexts separated (and some other browsers exactly do this), so if we should really special case something, it would only be this button. But is it worth it?
Comment on attachment 8647548 [details] [diff] [review]
patch v2

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

LGTM on OS X.

However, having just tested on Windows 8.1, I see the dropmarker (panel image) of the bookmarks button being stretched. The star and the home button seem fine.

::: browser/components/nsBrowserGlue.js
@@ +2141,5 @@
>      }
>  
> +    if (currentUIVersion < 31) {
> +      if (xulStore.hasValue(BROWSER_DOCURL, "bookmarks-menu-button", "class")) {
> +        xulStore.removeValue(BROWSER_DOCURL, "bookmarks-menu-button", "class");

Nit: 

The implementation of this is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/xulstore/XULStore.js#260 and afaict removeValue is just a no-op if the id/attribute doesn't exist, so you don't need the if (...hasValue(...)) checks.
Attachment #8647548 - Attachment is obsolete: false
Attachment #8647548 - Attachment is obsolete: true
Comment on attachment 8647573 [details] [diff] [review]
patch v2

(I bzimported literally 10 minutes ago, so I reviewed the correct patch on windows, I think)
Attachment #8647573 - Flags: review?(gijskruitbosch+bugs)
Attached patch patch v3Splinter Review
Attachment #8647573 - Attachment is obsolete: true
Attachment #8648356 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8648356 [details] [diff] [review]
patch v3

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

Let's do it.

Could you please file a followup for the bookmarks toolbarbutton hover state looking broken on Windows 8? This predates this patch but it looks sloppy, and ideally I'd like to fix. :-)
Attachment #8648356 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/7500607479f1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.