691594 - Remove empty placeholder next to bookmark text in toolbar when there is no image provided (or no favicon desired)

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: screenshot of default favicons on bookmarks toolbar

See attached screenshot.  Basically there is an empty placeholder box in the bookmarks toolbar for many of my links and ideally I'd either see nothing if there's not a favicon for the bookmarked page, and even if there is it would be great to choose not to have favicons in the toolbar as a space-saving measure.

Forking this issue off of bug 685059 as requested.

Comment 1

[Marco Bonardo [:mak]]

I think the change to show icons in the toolbar on OSX was explicit and a design choice.
So, what's exactly the request, add an option to hide all icons on toolbar? as a side not that should be really easy with a userchrome and there are addons doing that (for example https://addons.mozilla.org/firefox/addon/hide-favicons/).
Or you think we should just hide empty icons? This may be hard to do since we just fire an async request for the icon and we don't know if what we get back is the default icon or another one.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Marco Bonardo [:mak] from comment #1)
> I think the change to show icons in the toolbar on OSX was explicit and a
> design choice.

Can that be confirmed somehow? Is there a bug where this was all hashed out? (I'd be curious to learn the history of this decision)

> So, what's exactly the request, add an option to hide all icons on toolbar?
> as a side not that should be really easy with a userchrome and there are
> addons doing that (for example
> https://addons.mozilla.org/firefox/addon/hide-favicons/).
> Or you think we should just hide empty icons? This may be hard to do since
> we just fire an async request for the icon and we don't know if what we get
> back is the default icon or another one.

Funnily, the addon you mention isn't available for Mac so I guess if this is an intended design then yes, I would like this bug to become about having a way to hide the icons at the very least, though detecting that they aren't there in the first place would be preferable imo.

Comment 3

[Siddhartha Dugar [:sdrocking]]

Removing the icon entirely may look broken. We can have a generic star icon instead.

Comment 4

[PimpUigi]

A gold star icon would make sense for pages that are actually in your bookmarks, while the grey dotted line box is alright for pages that have no favicon and blank tabs.

I accidentally reverted the favicon to the blank piece of paper when I made the bug fix for Bug 700972

Comment 5

[Dick Boogaers]

(In reply to sdrocking from comment #3)
> Removing the icon entirely may look broken. We can have a generic star icon
> instead.

I think that would be lovely. The empty square does look like some bug. In fact I spent two days looking for the culprit causing it.

Comment 6

[letired]

Attachment: screenshot of bookmark toolbar using both text and favicons for bookmarks with new default favicon appearing

Comment 7

[letired]

Unclear from reading the this bug report, but this is also an issue with Firefox 8 on Windows XP.

The problem with the proposed solution of adding a different default favicon is that it continues to take up too much room in the toolbar. I already regularly have to pick and choose between things I want to have on my bookmark toolbar that I don't have enough room for. (see image)

The problem with the proposed solution of having an option to remove favicons entirely is that it breaks my ability to have a mixed combination of bookmarks in the toolbar, some of which are represented via text and some of which are represented via only their favicon. Being able to represent a bookmark by it's favicon only is an incredibly useful trick for me. (see image)

Also adding my voice to those who thing the empty square makes things look broken.

Comment 8

[Forest]

It's worth noting that the Bookmarks Toolbar is also the usual home of bookmarklets, none of which has its own icon.  Adding an icon next to each one not only wastes space but also gives the impression that it's a web site, which it isn't.  I happen to use that toolbar almost exclusively for bookmarklets, so for me, the addition of those placeholder icons was rather frustrating.  (A couple years back, my bookmarklets appeared in the toolbar with no icon.)

And yes: when a web site does have a distinct favicon, it is useful to be able to place it in the toolbar with no text, as a space saving measure.  Horizontal space is precious in a toolbar.

Comment 9

[Forest]

See also: bug 747620 comment 4.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Assigning this bug to Aish since they requested to fix bug 747620.

Here are the cases that we need to handle:

1) Bookmark has icon and label => show icon and label
2) Bookmark has icon, no label => show icon
3) Bookmark has label, no icon => show label
4) Bookmark has no icon or label => show generic icon (dotted line here would be preferred since the globe/Firefox icon is too closely related with privileged pages)

You'll want to update the CSS rules for .bookmark-item in /browser/themes/*/browser.css.

Comment 12

[u467816]

Attachment: Patch for windows

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 741341 [details] [diff] [review]
Patch for windows

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

As we discussed, this will also need to be fixed for linux and osx.

I noticed that there was 5 pixels on the left side of the button that shouldn't be there when the icon isn't there. The 5px is coming from http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#26.

We should tweak that line so it is:
> .toolbarbutton-icon[image]:not([image=""])[label]:not([label=""]),
> .toolbarbutton-icon[type="menu"] {
>   -moz-margin-end: 5px;
> }

which would mean that we should only put those 5px of margin in if there is both an image and a label.

This is off to a great start though!

Comment 14

[u467816]

Attachment: Modified patch for windows 7

Modified "browser.css" to include the favicon dimension(16*16), when the image is provided by the website. Modified "toolbarbutton.css".

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 742372 [details] [diff] [review]
Modified patch for windows 7

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

We should get review from a toolkit peer for the toolkit changes. This will get another person to double-check the changes as well as let them know that the default toolbarbutton styling is changing.

This looks to be in the right path. Please make the same changes for osx and linux.

::: browser/themes/windows/browser.css
@@ +648,5 @@
>  }
>  
>  /* ::::: bookmark items ::::: */
>  
> +.bookmark-item[label=""]  {

This should also apply to .bookmark-item:not([label]), so change this to:
> .bookmark-item[label=""],
> .bookmark-item:not([label]) {
>   ...
> }

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 742372 [details] [diff] [review]
Modified patch for windows 7

>--- a/toolkit/themes/windows/global/toolbarbutton.css
>+++ b/toolkit/themes/windows/global/toolbarbutton.css

>-.toolbarbutton-icon[label]:not([label=""]),
>+.toolbarbutton-icon[label]:not([label=""])[image]:not([image=""]),
> .toolbarbutton-icon[type="menu"] {
>   -moz-margin-end: 5px;
> }

This isn't going to work for toolbarbuttons in general, as the icon source can also be set via list-style-image rather than the 'image' attribute.

So please restrict your patch to browser.css. It seems like you could just set display:none for the icon in the relevant cases, such that the above margin wouldn't matter.

Comment 17

[u467816]

Attachment: Modified patch for windows 7

Added modifications in browser.css, leaving out toolbarbutton.css as it is.

Comment 18

[u467816]

Attachment: Patch for Linux and OSX

Do I have to change the code in line 2149 in osx/browser.css ?

@media (min-resolution: 2dppx) {
  .bookmark-item {
    list-style-image: url("chrome://mozapps/skin/places/[e-mail]");
  }

  image.bookmark-item {
    width: 16px;
  }
}

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 742785 [details] [diff] [review]
Modified patch for windows 7

>-.bookmark-item > .toolbarbutton-icon {
>+.bookmark-item > .toolbarbutton-icon[image]:not([image=""]) {
>   width: 16px;
>   height: 16px;
> }

>-.bookmark-item  {
>+.bookmark-item[label=""],
>+.bookmark-item:not([label])  {
>   list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
> }

These changes shouldn't be needed.

>+.bookmark-item[image]:[image=""][label]:not([label=""])
>+{
>+    display:none;
>+}

This selector is completely broken. I believe this is what you want:

.bookmark-item:not([image]):not([label=""]) > .toolbarbutton-icon {

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 742787 [details] [diff] [review]
Patch for Linux and OSX

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

Please follow the feedback that Dao provided in the previous review.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Hi Aish, are you able to still continue working on this bug? Let me know if there is anything I can do to help you along here.

Comment 22

[u467816]

(In reply to Dão Gottwald [:dao] from comment #19)
> Comment on attachment 742785 [details] [diff] [review]
> Modified patch for windows 7
> 
> >-.bookmark-item > .toolbarbutton-icon {
> >+.bookmark-item > .toolbarbutton-icon[image]:not([image=""]) {
> >   width: 16px;
> >   height: 16px;
> > }
> 
> >-.bookmark-item  {
> >+.bookmark-item[label=""],
> >+.bookmark-item:not([label])  {
> >   list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
> > }
> 
> These changes shouldn't be needed.
> 
> >+.bookmark-item[image]:[image=""][label]:not([label=""])
> >+{
> >+    display:none;
> >+}
> 
> This selector is completely broken. I believe this is what you want:
> 
> .bookmark-item:not([image]):not([label=""]) > .toolbarbutton-icon {

If I just add the above line with display:none (leaving out all other changes), how will the condition that "a site has image attrib but its empty" be taken into account?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Aish from comment #22)
> If I just add the above line with display:none (leaving out all other
> changes), how will the condition that "a site has image attrib but its
> empty" be taken into account?

No. What kind of bookmark items have an empty image attribute?

Comment 24

[u467816]

Ok, I am just new to this. I don't know much. Thanks

Comment 25

[u467816]

Attachment: Modified patch for windows,linux,osx

I could not verify the patch for Linux and OSX. I did it for windows 7

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 748029 [details] [diff] [review]
Modified patch for windows,linux,osx

It just occurred to me that we need to handle some items that have an icon but don't have an image attribute:

- bookmark folders (detectable with the [container] attribute)
- buttons that pretend to be a bookmark-item when placed on the personal toolbar, like the Home button. I think the best way to make sure only real bookmarks are affected might be adding "#PlacesToolbarItems >" to the selector.

Comment 27

[u467816]

Attachment: Patch for windows

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 748905 [details] [diff] [review]
Patch for windows

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

Looks good! Just need to fix up a couple white-space issues in this, and also include the linux and osx changes here too. You can put all the changes in one patch.

::: browser/themes/windows/browser.css
@@ +612,5 @@
>    height: 16px;
>  }
>  
> +#PlacesToolbarItems > .bookmark-item:not([image]):not([label=""]):not([container]) > .toolbarbutton-icon {    
> +  display:none;

Please remove the trailing white-space at the end of line 615 and include a space after 'display:'.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Please also follow the comments that Dao provided in https://bugzilla.mozilla.org/show_bug.cgi?id=691594#c26

Comment 30

[u467816]

Attachment: Patch for Windows, Linux and OSX

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/729e7f7f623e

Comment 32

[Siddhartha Dugar [:sdrocking]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/729e7f7f623e

It's surprising that bug 685059 didn't land before Australis but this one did.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Siddhartha Dugar [:sdrocking] from comment #32)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/729e7f7f623e
> 
> It's surprising that bug 685059 didn't land before Australis but this one
> did.

The two bugs are only similar in the fact that they make us rely on the default favicon less. Since the bookmarks toolbar is less prevalent and not getting a major overhaul like the tabs are, it's easier to ship this one sooner.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/729e7f7f623e

Comment 35

[Caspy7]

After updating to Aurora 24 I noticed that my bookmarklet placeholder icons were gone (obviously, this bug).

A few issues have arisen for me as a result.
I have one bookmarklet simply named "DR".  Its width has now been reduced by half, such that it now takes much more focus/effort/time to precisely click it.  It's now slightly less the width than if it were merely an icon with no text.
As an experiment I renamed it to "i".  The total width of the bookmark on mouseover was 9 px (before the icon removal it was 30 px).  I'm not saying that removing the icon placeholder is the wrong thing to do, but that this has created a UX issue in some instances because of the extra effort to click very small widths.

I'd also observe that the icons and placeholders make a natural delineation between bookmarks.  With the placeholders gone, bookmarklets sandwiched between bookmarks or other bookmarklets are now visually harder targets.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark S. from comment #35)

See bug 888745. We can also set a minimum width on bookmark items.

Comment 37

[bhavana bajaj [:bajaj]]

Aish/Dao, given https://bugzilla.mozilla.org/show_bug.cgi?id=888745#c4 can you please help with the backout on Aurora here ?

Comment 38

[Dão Gottwald [:dao]]

Attachment: backout

backed out because of bug 888745

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 775807 [details] [diff] [review]
backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f815340b1d7

Comment 40

[Ed Morley [:emorley]]

(In reply to Dão Gottwald [:dao] from comment #39)
> Comment on attachment 775807 [details] [diff] [review]
> backout
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3f815340b1d7

Backout merged:
https://hg.mozilla.org/mozilla-central/rev/75465ea1cfb6

Comment 41

[Ed Morley [:emorley]]

Sorry, https://hg.mozilla.org/mozilla-central/rev/3f815340b1d7 even

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 775807 [details] [diff] [review]
backout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug
User impact if declined: bug 888745
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 43

[bhavana bajaj [:bajaj]]

Comment on attachment 775807 [details] [diff] [review]
backout

Approving the low risk backout to avoid the user impact in 888745

Comment 44

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5936c6a4b298

Comment 45

[raffik]

hello;
we are four students in computing of the university Paris 8 Vincennes Saint Denis (France),and we want to work on this bug for our school project.

raffik,sofiane,abdelaziz,Aierken.

Cordially

Comment 46

[Dão Gottwald [:dao]]

This was fixed but backed out for bug 888745. As it stands, this bug isn't actionable.

Comment 47

[Forest]

(In reply to Dão Gottwald [:dao] from comment #46)
> This was fixed but backed out for bug 888745. As it stands, this bug isn't
> actionable.

In what version of Firefox was it fixed? I'm using version 33 and the bug is still present.

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Forest from comment #47)
> (In reply to Dão Gottwald [:dao] from comment #46)
> > This was fixed but backed out for bug 888745. As it stands, this bug isn't
> > actionable.
> 
> In what version of Firefox was it fixed? I'm using version 33 and the bug is
> still present.

It never got released with the changes that were proposed in this bug. With the changes, if you had multiple bookmarks missing icons next to each other, it could be too hard to tell where one bookmark button started and the other ended without mousing over each of them.

Comment 49

[Forest]

I see. To summarize: removing the empty box icons created a new problem, so someone decided the appropriate fix for the new problem was to restore original problem, and now we're stuck with the empty box icons again.

Appeasing one group of users by causing problems for another group seems rather... short-sighted. I don't understand why someone would think that a space-wasting box is a good separator for items in a toolbar. It seems to me that a narrow separator would be more appropriate, as it would waste less space and look a heck of a lot better than these meaningless boxes. Failing that, a user-configurable option to skip the boxes would solve both bugs.

Rather than closing this bug and pretending the problem doesn't exist, how about just changing the title from "remove" to "optionally remove" or "replace"?

Comment 50

[Forest]

Also, for the record, it seems to me that separating bookmark toolbar items the same way that menu toolbar items are separated would be objectively better than what be have today.

Comment 51

[Forest]

Here's a summary of the ideas I posted to bug 888745 that would solve both problems (rather than etching this one in stone):

- Use the same policy for separating bookmark toolbar items that is used for menu toolbar items. This would make the UI consistent.
- If a visible separator must be forced on everyone, at least make it narrow, like the separator lines used everywhere else in the menus and toolbars.
- Improving on that idea, make the use of a visible separator configurable, so those of us using bookmarkelts don't have them pushed off the edge of the browser window.
- Define a minimum width for toolbar items.

If raffik, sofiane, abdelaziz, and Aierken want to work on this, it seems silly not to let them.

Comment 52

[Dão Gottwald [:dao]]

(In reply to Forest from comment #51)
> Here's a summary of the ideas I posted to bug 888745 that would solve both
> problems (rather than etching this one in stone):
> 
> - Use the same policy for separating bookmark toolbar items that is used for
> menu toolbar items. This would make the UI consistent.

Not sure what this even means.

> - If a visible separator must be forced on everyone, at least make it
> narrow, like the separator lines used everywhere else in the menus and
> toolbars.

Please feel free to file a new bug on this. Bonus points for a patch or a mockup that the UX team could review.

> - Improving on that idea, make the use of a visible separator configurable,
> so those of us using bookmarkelts don't have them pushed off the edge of the
> browser window.

This sounds like overkill, not something that average users should have to deal with. (If you're a more adventurous user, you can already hide the placeholder favicons today via userChrome.css.)

> - Define a minimum width for toolbar items.

This wouldn't solve the problem of short bookmark titles unnecessarily occupying too much space, nor would it avoid bug 888745 for long titles.

Comment 53

[Forest]

(In reply to Dão Gottwald [:dao] from comment #52)
> > - Use the same policy for separating bookmark toolbar items that is used for
> > menu toolbar items. This would make the UI consistent.
> 
> Not sure what this even means.

It means that rather than keeping these meaningless and needlessly wide placeholder boxes, bookmark toolbar items could be separated the same way that menu bar items are separated. I believe that would address bug 888745, address this bug, and fix an inconsistency that we have today.

> > - If a visible separator must be forced on everyone, at least make it
> > narrow, like the separator lines used everywhere else in the menus and
> > toolbars.
> 
> Please feel free to file a new bug on this.

A new bug would hide all the discussion of the problem that already exists here, disconnect everyone who has already subscribed to this bug, and probably make it easier for it to be blown off in the same way that this one was yesterday. I'm not a fan of throwing away significantly relevant information.

> Bonus points for a patch or a mockup that the UX team could review.

I can't tell whether that is a sincere suggestion or just a deflection technique, but in case you've forgotten, we already have four students who want to work on this.

> > - Improving on that idea, make the use of a visible separator configurable,
> > so those of us using bookmarkelts don't have them pushed off the edge of the
> > browser window.
> 
> This sounds like overkill, not something that average users should have to
> deal with. (If you're a more adventurous user, you can already hide the
> placeholder favicons today via userChrome.css.)

Where is this documented, please? I'd like to try it.

> > - Define a minimum width for toolbar items.
> 
> This wouldn't solve the problem of short bookmark titles unnecessarily
> occupying too much space, nor would it avoid bug 888745 for long titles.

It would solve the width problem here if it didn't add as much width as the placeholder boxes do, and it would avoid the "mouse-eye dexterity test" problem mentioned in bug 888745. Anyway, I didn't say this was the best idea of the bunch; I just mentioned it because even this partial fix would be better than solving one problem by forcing another to persist.

My point is that addressing bug 888745 with a WONTFIX here is throwing the baby out with the bathwater. If you don't want to deal with this one right now, just leave it open, for Pete's sake. That costs nothing. Better yet, let the nice folks who stepped up with an offer to work on it do so.

Comment 54

[Marco Bonardo [:mak]]

This is a ticketing system (bug tracker more specifically), leaving things open doesn't help anyone. The specific suggestion that was tried here didn't work and thus is a WONTFIX, it should not be tried again.
The remaining discussion would have been better handled in a mailing list, like firefox-dev. By reopening this you'd end up hiding information, cause no one wants to read 53 comments, just to figure out a couple concepts that could be summed up in a few rows in a new bug (especially when you follow tens of bugs per day).

A new proposal should be filed into a new bug, that can point to this (there is a "see also" field). Doing that would benefit everyone and allow to restart work on a new solution that UX can evaluate and someone can implement.

Comment 55

[Forest]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #54)
> The specific suggestion that was tried here didn't
> work and thus is a WONTFIX, it should not be tried again.

> A new proposal should be filed into a new bug, that can point to this (there
> is a "see also" field).

Oh, I see: You're apparently accustomed to using a bug report to track a particular solution, while I'm accustomed to using it to track a particular problem. Well, that would expain why the WONTFIX status change was made, and why I found it so perplexing. Okay, if that's how the Mozilla team uses their bug tracker, I can adjust accordingly.

Comment 56

[rafa]

(In reply to Forest from comment #51)
> Here's a summary of the ideas I posted to bug 888745 that would solve both
> problems (rather than etching this one in stone):
> 
> - Use the same policy for separating bookmark toolbar items that is used for
> menu toolbar items. This would make the UI consistent.
> - If a visible separator must be forced on everyone, at least make it
> narrow, like the separator lines used everywhere else in the menus and
> toolbars.
> - Improving on that idea, make the use of a visible separator configurable,
> so those of us using bookmarkelts don't have them pushed off the edge of the
> browser window.
> - Define a minimum width for toolbar items.
> 
> If raffik, sofiane, abdelaziz, and Aierken want to work on this, it seems
> silly not to let them.



hi,
we do not know if we can work on the bug 691594, when we read comments of the other member, there are those who say that the bug is useless,and we mustn't work on it. 

so,we can work on the bug 691594 or no?
thanks

Comment 57

[Forest]

As suggested above, I filed a new bug report: bug 1102378.

> so,we can work on the bug 691594 or no?

raffik, I suggest you re-post your offer to the new bug report. It seems that is where new work on this problem will be tracked.

Comment 58

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Forest from comment #55)
> Oh, I see: You're apparently accustomed to using a bug report to track a
> particular solution, while I'm accustomed to using it to track a particular
> problem.

Yeah, the bug report summary here indicates the solution to the problem. The real problem here is that the square/dashed icon is pretty meaningless. But we haven't come to a solution that doesn't have more negative trade-offs than positive.