996562 - The text on the Bookmarks toolbar is barely visible when the bar is moved to the tab bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Ioana (away)]

Attachment: Issue displayed.png

STR:
1. Set the Bookmarks toolbar as visible.
2. Customize Firefox by moving the Bookmarks Toolbar items to the tab bar.

Actual results:
The text from the bookmarks items is barely visible since it's dark gray on a black background.

Expected Results:
The bookmarks text has the same properties as the tab titles.

Notes:
1. This issue is only reproducible on Linux.
2. This is not a regression (it reproduces back to Firefox 4.0.1).

Comment 1

[Ioana (away)]

This issue is reproducible on all current versions: Firefox 29, 30 and 31.

Comment 2

[:Gijs (he/him)]

This will need to update .bookmark-item elements that are direct kids of the bookmarks toolbar and aren't hovered/active to color:inherit instead of whatever it is now. The styling would probably have to live somewhere under this header:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#121

Comment 3

[rn10950]

Ill take it

Comment 4

[:Gijs (he/him)]

(In reply to rn10950 from comment #3)
> Ill take it

Great! I'm on vacation right now, but I'll be back on Monday. Let me know if you have any questions, and I'll get to them first thing on Monday if not before.

Comment 5

[rn10950]

Attachment: fx_996562-Gnome.png

I tried to recreate this bug in Unity and Gnome and was unsuccessful. Attached is a  screenshot in GNOME I will also upload one from Unity.

Comment 6

[rn10950]

Attachment: fx_996562-Unity.png

The Unity screenshot.

Comment 7

[rn10950]

Nevermind, I'm an idiot. The bug only appeared fixed because all the default themes in 14.04 are light and the default color of the text looked natural.

Comment 8

[rn10950]

Attachment: link states

I have patched the bug, but in doing so I have created another one. Now, when you hover/click on the bookmark the text becomes unreadable as it blends into the hover "glow"

Comment 9

[:Gijs (he/him)]

(In reply to rn10950 from comment #8)
> Created attachment 8547115 [details]
> link states
> 
> I have patched the bug, but in doing so I have created another one. Now,
> when you hover/click on the bookmark the text becomes unreadable as it
> blends into the hover "glow"

You might be able to use :not(:hover):not(:active) (not sure if you need the :active bit; check!) to work around this?

We really need bug 734326 to fix this properly, but this will do in the meantime.

Comment 10

[rn10950]

Attachment: customize bug

I have fixed the link state issue, but when I go into customize mode, that text is unreadable. I have provided a screenshot.

Comment 11

[:Gijs (he/him)]

(In reply to rn10950 from comment #10)
> Created attachment 8547128 [details]
> customize bug
> 
> I have fixed the link state issue, but when I go into customize mode, that
> text is unreadable. I have provided a screenshot.

Considering the non-selected tabs behave the same way, I doubt this is the fault of your patch. It looks like we should ensure the tabstoolbar background color doesn't change in customize code, but stays dark (like the menu bar). Dão, does that sound right to you?


rn10950, can you attach your patch? :-)

Comment 12

[rn10950]

Attachment: bug996562_bookmarks-tabbar.diff

Here is the patch for the bookmarks on the tabbar

Comment 13

[:Gijs (he/him)]

Comment on attachment 8547220 [details] [diff] [review]
bug996562_bookmarks-tabbar.diff

Ah. But in this case, maybe try adding just a separate rule for the toolbarbutton.bookmark-item:not(.subviewbutton):not(:hover) to set color: inherit ? I expect that we'll want to keep the margin/padding the same even when items are hovered. :-)

Because you added this to a rule for the #bookmarks-toolbar-placeholder, it's also possible that doing that will be enough to fix the customize mode issue.

Comment 14

[rn10950]

Attachment: patch #2 - fixes customize bug

I tried what you suggested and it fixed the customize bug.

Comment 15

[rn10950]

Attachment: patch #2 - fixes customize bug

This is an updated version of the last patch adding a line break after the new rule

Comment 16

[:Gijs (he/him)]

Comment on attachment 8547227 [details] [diff] [review]
patch #2 - fixes customize bug

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

Almost there...

::: browser/themes/linux/browser.css
@@ +124,5 @@
>    margin: 0;
>    padding: 2px 3px;
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton):not(:hover) {

I just tried the patch. It works well, but I did find that we do actually need the :not(:active) after the :not(:hover) as well. Try clicking on a bookmarks folder, and then hovering out of the submenu that opens, and you'll see that the color goes back to being light-colored...

For the next patch, note that instead of setting review+, the idea is that you set it to "?" and put someone's email address (or a string that uniquely matches a bugzilla account name + email, such as ":gijs" (including the ":"!), which will find me) in the field next to the flag. This indicates that you're asking for that person to review the patch. They can then set review+ or review-, depending on whether the patch is good or not yet. See also: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed .

(I'm setting feedback+ to indicate that while this is almost there, it is not quite there yet)

Finally, you don't need to list the files you're changing in the patch description. :-)

Comment 17

[rn10950]

I added :not(:active) to the rule and it does nothing about hovering off the menu. Do you have any ideas?

Comment 18

[:Gijs (he/him)]

(In reply to rn10950 from comment #17)
> I added :not(:active) to the rule and it does nothing about hovering off the
> menu. Do you have any ideas?

Oh, sorry. You're right, that's not enough. You also need :not([open]). Good catch!

Comment 19

[rn10950]

Attachment: bug 996562 - completed patch

OK, the :not([open]) worked so this is the patch

Comment 20

[:Gijs (he/him)]

Comment on attachment 8547258 [details] [diff] [review]
bug 996562 - completed patch

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

Thanks! This looks great. I've landed this as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/631baa2e7054

Note that I adjusted the patch summary a little and added my r=.

This will be merged to mozilla-central sometime within the next 24 hours, and then eventually released as part of Firefox. Thanks for your help!

Do you also have access to a Windows 8 machine? In which case, would you be interested in helping to fix e.g. bug 999298 ? Or maybe you know JS and would like a challenge with something like bug 989307? :-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/631baa2e7054

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to rn10950 from comment #10)
> > Created attachment 8547128 [details]
> > customize bug
> > 
> > I have fixed the link state issue, but when I go into customize mode, that
> > text is unreadable. I have provided a screenshot.
> 
> Considering the non-selected tabs behave the same way, I doubt this is the
> fault of your patch. It looks like we should ensure the tabstoolbar
> background color doesn't change in customize code, but stays dark (like the
> menu bar). Dão, does that sound right to you?

yes, I would say so