Closed Bug 996562 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

29 Branch
All
Linux
defect
Not set
minor
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan

People

(Reporter: ioana_damy, Assigned: rn10950, Mentored)

Details

(Whiteboard: [good next bug][lang=css])

Attachments

(2 files, 7 obsolete files)

Attached image 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).
This issue is reproducible on all current versions: Firefox 29, 30 and 31.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=0
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
Mentor: gijskruitbosch+bugs
Points: --- → 2
Whiteboard: p=0 → [good next bug][lang=css]
Ill take it
(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.
Assignee: nobody → rn10950
Attached image fx_996562-Gnome.png (obsolete) —
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.
Attached image fx_996562-Unity.png (obsolete) —
The Unity screenshot.
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.
Attached image link states (obsolete) —
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"
(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.
Attached image customize bug (obsolete) —
I have fixed the link state issue, but when I go into customize mode, that text is unreadable. I have provided a screenshot.
Attachment #8546968 - Attachment is obsolete: true
Attachment #8546969 - Attachment is obsolete: true
Attachment #8547115 - Attachment is obsolete: true
(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? :-)
Flags: needinfo?(dao)
Attached patch bug996562_bookmarks-tabbar.diff (obsolete) — Splinter Review
Here is the patch for the bookmarks on the tabbar
Attachment #8547220 - Flags: review+
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.
Attachment #8547220 - Flags: review+
Attached patch patch #2 - fixes customize bug (obsolete) — Splinter Review
I tried what you suggested and it fixed the customize bug.
Attachment #8547128 - Attachment is obsolete: true
Attachment #8547220 - Attachment is obsolete: true
Attachment #8547224 - Flags: review+
Attached patch patch #2 - fixes customize bug (obsolete) — Splinter Review
This is an updated version of the last patch adding a line break after the new rule
Attachment #8547224 - Attachment is obsolete: true
Attachment #8547227 - Flags: review+
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. :-)
Attachment #8547227 - Flags: review+ → feedback+
I added :not(:active) to the rule and it does nothing about hovering off the menu. Do you have any ideas?
(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!
OK, the :not([open]) worked so this is the patch
Attachment #8547227 - Attachment is obsolete: true
Attachment #8547258 - Flags: review?(gijskruitbosch+bugs)
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? :-)
Attachment #8547258 - Flags: review?(gijskruitbosch+bugs) → review+
Iteration: --- → 37.3 - 12 Jan
Flags: qe-verify-
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/631baa2e7054
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(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
Flags: needinfo?(dao)
You need to log in before you can comment on or make changes to this bug.