1102378 - favicon placeholder boxes waste horizontal space in the bookmarks toolbar

Status: UNCONFIRMED

(Firefox :: Theme, defect)

Description

[Forest]

Attachment: screenshot from letired@gmail

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141013200257

Steps to reproduce:

I placed some items, including some bookmarklets, in the bookmarks toolbar.


Actual results:

The bookmarklets (and bookmarks without favicons) are displayed with ugly, meaningless boxes next to them, wasting so much horizontal space that some of my bookmarks/bookmarklets are pushed off the right edge of the toolbar.


Expected results:

Items that have no favicon, such as bookmarklets, should use only as much horizontal space as required to display their names, just like the items in the menu bar.

Various ideas for how to thandle this were posted to bug 691594, which was closed because the first attempt at a fix was later backed out to fix bug 888745.

A group of students (raffik, sofiane, abdelaziz, Aierken) have already offered to work on this.

Comment 1

[rafa]

(In reply to Forest from comment #0)
> Créé attachement 8526159  [détails]
> capture d'écran de letired @ gmail
> 
> User Agent: Mozilla / 5.0 (X11;; Ubuntu Linux x86_64; rv: 33.0) Gecko /
> 20100101 Firefox / 33.0
>  Construire ID: 20141013200257
> 
> Étapes pour reproduire:
> 
> Je mis quelques articles, dont certains bookmarklets, dans la barre d'outils
> des signets.
> 
> 
> Les résultats réels:
> 
> Les bookmarklets (et les signets sans favicons) sont affichés avec laids,
> des boîtes vides de sens à côté d'eux, perdre autant horizontale espace que
> certains de mes signets / bookmarklets sont poussés vers le bord droit de la
> barre d'outils.
> 
> 
> Résultats attendus:
> 
> Articles qui ont pas de favicon, comme bookmarklets, devraient utiliser
> autant d'espace horizontal que nécessaire pour afficher leurs noms, tout
> comme les éléments de la barre de menu.
> 
> Diverses idées pour THandle cette inscription au bug 691594 , qui a été
> fermé parce que la première tentative à un correctif a été plus tard désisté
> de fixer bogue 888745 .
> 
> Un groupe de étudiants (Raffik, sofiane, Abdelaziz, Aierken) ont déjà offert
> de travailler sur ce point.


hi forest,
we are ok to work on this new bug,can you assigne the bug to our group.
thank you.

Comment 2

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

What approach are you planning on taking in this bug that will solve the issue that you have presented but does not reintroduce the issue that bug 888745 was about?

Comment 3

[vempe2]

Good evening , I have seen a bit architecture mozilla but I do not found the corependant file has this bug, can i helpe me please? 
thank you

Comment 4

[Dão Gottwald [:dao]]

(In reply to vempe2 from comment #3)
> Good evening , I have seen a bit architecture mozilla but I do not found the
> corependant file has this bug, can i helpe me please? 
> thank you

Attachment 749229 [details] [diff] should give you a clue.

Comment 5

[fos]

hi Dão,
 
I am Looking for this bug , I read your comments I can not understand at your comment precedent [diff] there are green rectangles.
however correspend the green rectangle?  why there are 2 code tables?

thank's

Comment 6

[Dão Gottwald [:dao]]

https://bugzilla.mozilla.org/attachment.cgi?id=749229&action=diff is just a "nicer" visualization of https://bugzilla.mozilla.org/attachment.cgi?id=749229. The left side shows the state of the file without the patch applied, the right side shows the state of the file with the patch applied. Lines marked green are lines that have been added. Lines marked red would represent removed lines, but there aren't any in that particular patch.

Comment 7

[rafa]

thanks Dão.

Comment 8

[rafa]

Hello,

we are working on this bug. However each time that we modify a file, the compilation process compiles all the code and not only the file that we modified. Is there a way to compile only the files relevant to the modification. This will speed-up our work.

Thanks.

Comment 9

[Marco Bonardo [:mak]]

(In reply to rafa from comment #8)
> we are working on this bug. However each time that we modify a file, the
> compilation process compiles all the code and not only the file that we
> modified. Is there a way to compile only the files relevant to the
> modification. This will speed-up our work.

in this case you can do just ./mach build browser
(you could even do browser/themes but the time difference is not very interesting)

Comment 10

[sofiane]

Hello,
We have implemanted the patch provided by Dao in comment N°4 and indeed the square disapears.
However, the space remains.
We are working on that issue and will give you some news soon.
Thanks.

Comment 11

[sofiane]

Good afternoon,

I would like to ask whether some one can point me to reference of mozilla CSS definitions.
I have some troubles while understanding the syntax and the meaning of some elements (for example #home-button, #bookmarks-toolbar-placeholder, how the negation of those elements works, etc.)

Thanks,

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Hi sofiane. I see that you haven't received any response. In the future, you should use the "Need more information from [someone specific]" option at the bottom of this page to make sure that someone is informed that you are expecting an answer. Otherwise, it's pretty easy to lose track of questions as we typically receive hundreds of bugzilla updates per day.

Let's ask Dao if he has an idea.

Comment 13

[Dão Gottwald [:dao]]

I'm not quite sure what the question is. What do you mean by CSS definitions? #home-button and #bookmarks-toolbar-placeholder are just elements that exist in browser.xul, but that doesn't really seem relevant to this bug.

Comment 14

[sofiane]

Hello Dao, David,

Thank you for your answers. Here's how we are trying to solve this issue.

First we applied the patch provided in comment 4. This didn't change the behavior of the bookmarks toolbar but it gave us some ideas about how to proceed.

Our idea is to target the items in the bookmark toolbar that are not home button, placeholder, container or that do not have a favicon. For those items (this is all the items which display the empty square) we do not allocate the 16px space needed to display the favicon.

We tried to experiment with the .bookmark-item rule in the browser.css file but with no success. We commented the function that calls the empty square icon. Then we inverted the .bookmark-item rule in the following way: 

> .bookmark-item:(#home-button):not([container]) > .toolbarbutton-icon,

This gave us the wanted behavior: bookmarks with icons show correctly and bookmarks with no icon do not display the empty square. However bookmarks in the bookmark menu (not in the toolbar) also do not display the empty square. This way we also lose control over the size of favicons (if a favicon is 30px of size, this is not applied).

We would like to find a way to target more precisely the bookmarks with no favicon. We modified the .bookmark-item rule in a many different ways but with not success. We tried expressions such: 

> .bookmark-item:not(#home-button):not([defaultFavicon.png]) > .toolbarbutton-icon,
> .bookmark-item:not(#home-button):not([image]):not([container]) > .toolbarbutton-icon,

But we can't target only bookmarks with no favicon. This was the meaning of our question about the correct way to define #home-button, #bookmarks-toolbar-placeholder and the other element of .bookmark-item.
  
Do you think that this is a correct manner to deal with this issue. If not, do you have other ideas.
If yes, do you have some hints about how to target more precisely the bookmarks with no favicon.

Thanks.

Comment 15

[Dão Gottwald [:dao]]

As I understand it, this selector should get you what you want.

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

That's just copied from the patch mentioned in comment 4.

Comment 16

[sofiane]

Attachment: attachment 1102378

Hello,

Here are the patch you think about it?
Thanks

Comment 17

[Dão Gottwald [:dao]]

Looks like this patch is mostly the same as attachment 749229 [details] [diff] [review], which we backed out for causing bug 888745.