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

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Comment 1

[:Gijs (he/him)]

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...

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[:Gijs (he/him)]

(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? :-)

Comment 4

[Jim Mathies [:jimm]]

(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.

Comment 5

[:Gijs (he/him)]

Attachment: 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.

Comment 6

[:Gijs (he/him)]

Stephen, can you provide assets we can use here, probably for both Windows and Linux?

Comment 7

[Tim Nguyen :ntim]

Is this what you are looking for : http://mxr.mozilla.org/mozilla-central/ident?i=NS_THEME_MENUARROW ?

Comment 8

[Tim Nguyen :ntim]

Btw, this hack might work :
.subviewbutton .menu-right {
   color: black !important;
}

Comment 9

[:Gijs (he/him)]

(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.

Comment 10

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: menu-discolureArrow.png

Based on the arrows in shorlanders mockup, but adjusted to meet the default Windows colors.

Comment 11

[:Gijs (he/him)]

Attachment: add custom arrows for Australis bookmarks menu panel on Windows,

Alright, how is this, Mike? Tested on Win XP Luna and Win7 Aero.

Comment 12

[Mike de Boer [:mikedeboer]]

This needs to be done for Linux too, I'm afraid...

Comment 13

[Mike de Boer [:mikedeboer]]

Apart from that, re. code it looks a-ok to me!

Comment 14

[:Gijs (he/him)]

Attachment: 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.

Comment 15

[Mike de Boer [:mikedeboer]]

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.

Comment 16

[Mike de Boer [:mikedeboer]]

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.

Comment 17

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/03e7402f6fd3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03e7402f6fd3

Comment 19

[:Gijs (he/him)]

(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 20

[:Gijs (he/him)]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc1fdefca201

Comment 22

[:Gijs (he/him)]

(In reply to Carsten Book [:Tomcat] from comment #21)
> https://hg.mozilla.org/mozilla-central/rev/cc1fdefca201

This was really bug 981700.

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92e878e7f87e