Closed Bug 978309 Opened 6 years ago Closed 6 years ago

Don't show white arrows on bookmarks panel folders when active on Linux & Windows

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:P3])

Attachments

(3 files, 1 obsolete file)

No description provided.
Whiteboard: [Australis:P-] → [Australis:P3]
Looks like this is completely controlled through native code. :-\

Jim, is that right? Is this glyph provided and colored through nsNativeTheme.cpp? That's what it looks like to me...
Flags: needinfo?(jmathies)
I'm not sure. mbrubeck was working on these elements for highdpi and was having a hard time finding the code that rendered them. I haven't had a chance to look at this but can.
(In reply to Jim Mathies [:jimm] from comment #2)
> I'm not sure. mbrubeck was working on these elements for highdpi and was
> having a hard time finding the code that rendered them. I haven't had a
> chance to look at this but can.

I mean, I guess the end of the story will be that we'll need to override it (with an image) specifically for the bookmarks menu, so I can work from there. Unless you have time to do exactly that? :-)
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Jim Mathies [:jimm] from comment #2)
> > I'm not sure. mbrubeck was working on these elements for highdpi and was
> > having a hard time finding the code that rendered them. I haven't had a
> > chance to look at this but can.
> 
> I mean, I guess the end of the story will be that we'll need to override it
> (with an image) specifically for the bookmarks menu, so I can work from
> there. Unless you have time to do exactly that? :-)

I don't know, can you post a screen shot of what the issue is or str to reproduce? I have no context.
Flags: needinfo?(jmathies)
Attached image bookmark menu.png
(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to Jim Mathies [:jimm] from comment #2)
> > > I'm not sure. mbrubeck was working on these elements for highdpi and was
> > > having a hard time finding the code that rendered them. I haven't had a
> > > chance to look at this but can.
> > 
> > I mean, I guess the end of the story will be that we'll need to override it
> > (with an image) specifically for the bookmarks menu, so I can work from
> > there. Unless you have time to do exactly that? :-)
> 
> I don't know, can you post a screen shot of what the issue is or str to
> reproduce? I have no context.

Sure. Here's a screenshot. Basically, the issue is that we use non-native styling and so the white arrow for submenus is invisible. It should be using the black one throughout. I suspect we'll just need to provide an image and use that for all versions of Windows. Might be able to reuse an image that's in the tree already.
Stephen, can you provide assets we can use here, probably for both Windows and Linux?
Flags: needinfo?(shorlander)
Btw, this hack might work :
.subviewbutton .menu-right {
   color: black !important;
}
(In reply to Tim Nguyen [:ntim] from comment #7)
> Is this what you are looking for :
> http://mxr.mozilla.org/mozilla-central/ident?i=NS_THEME_MENUARROW ?

No, I know about that code. It uses native Windows code to render the arrow, and there's no real way to influence that without altering our widget code, which would be weird. We simply need .png images to replace these arrows (which are done through so -moz-appearance: menuarrow).

(In reply to Tim Nguyen [:ntim] from comment #8)
> Btw, this hack might work :
> .subviewbutton .menu-right {
>    color: black !important;
> }

No, that does not work. Nor will anything else along those lines: the code for the color of the arrow doesn't look at CSS at all.
Attached image menu-discolureArrow.png
Based on the arrows in shorlanders mockup, but adjusted to meet the default Windows colors.
Flags: needinfo?(shorlander)
Alright, how is this, Mike? Tested on Win XP Luna and Win7 Aero.
Attachment #8388911 - Flags: review?(mdeboer)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
This needs to be done for Linux too, I'm afraid...
OS: Windows XP → All
Hardware: x86 → All
Apart from that, re. code it looks a-ok to me!
Summary: [WinXP] Don't show white arrows on bookmarks panel folders when active → Don't show white arrows on bookmarks panel folders when active on Linux & Windows
Now with linux support. Needed a height/width setting because menu.css sets those, and otherwise the icon looks warped.
Attachment #8389185 - Flags: review?(mdeboer)
Attachment #8388911 - Attachment is obsolete: true
Comment on attachment 8389185 [details] [diff] [review]
add custom arrows for Australis bookmarks menu panel on Windows and Linux,

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

Since the two images are identical, can you place it in ../shared/places/bookmarks-menu-arrow.png and update the jar.mn entries?

I also was able to have off 33% using ImageOptim.app here...

The CSS looks good. Shame of the code duplication, but that can not be avoided.
Attachment #8389185 - Flags: review?(mdeboer) → review-
Comment on attachment 8389185 [details] [diff] [review]
add custom arrows for Australis bookmarks menu panel on Windows and Linux,

Discussed this on IRC. r=me with a minified version of the arrows png.
Attachment #8389185 - Flags: review- → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/03e7402f6fd3
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/03e7402f6fd3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
(In reply to :Gijs Kruitbosch from comment #14)
> Created attachment 8389185 [details] [diff] [review]
> add custom arrows for Australis bookmarks menu panel on Windows and Linux,
> 
> Now with linux support. Needed a height/width setting because menu.css sets
> those, and otherwise the icon looks warped.

So this hunk disappeared. Sticking this in bug 969592 because I noticed it there.
Comment on attachment 8389185 [details] [diff] [review]
add custom arrows for Australis bookmarks menu panel on Windows and Linux,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' bookmarks panel
User impact if declined: arrows are hard to see on Windows and Linux
Testing completed (on m-c, etc.): on m-c, manual
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8389185 - Flags: approval-mozilla-aurora?
Attachment #8389185 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Carsten Book [:Tomcat] from comment #21)
> https://hg.mozilla.org/mozilla-central/rev/cc1fdefca201

This was really bug 981700.
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.