Closed Bug 1000700 Opened 10 years ago Closed 6 years ago

menu-button dropmarkers aren't inverted with dark LWT

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Version:
29 Branch
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: MattN, Unassigned, Mentored)

References

Details

(Whiteboard: [devedition-polish][lang=css])

Attachments

(2 files, 2 obsolete files)

releases-mozilla-beta-b074e386a410_5_darkLWT_appMenuClosed.png
24.75 KB, image/png
Details
dark-bookmarks-chevron-dark-lwtheme.png
15.43 KB, image/png
Details 
A dark dropmarker on a dark theme doesn't go well.
Blocks: 513418
Flags: firefox-backlog+
Same issue with the dropmarker inside of the bookmarks toolbar
At least for the bookmarks dropmarker, there is an inverted state available (and can be seen on hover).  But it just isn't yet applied on the [brighttext] toolbar.
So, there are a lot of dropmarkers I've found so far in the theme (some have are inverted options, some don't):

OSX:
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/toolbarbutton-dropmarker-lion.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/toolbarbutton-dropmarker-lion@2x.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/toolbarbutton-dropmarker.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/folderDropArrow.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/folderDropArrow@2x.png

Windows:
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/toolbarbutton-dropdown-arrow-inverted.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/toolbarbutton-dropdown-arrow-XPVista7.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/toolbarbutton-dropdown-arrow.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/places/toolbarDropMarker-aero.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/places/toolbarDropMarker.png

On linux, there is a -moz-appearance: toolbarbutton-dropdown - I'm not seeing any icons, but I'm probably missing them.

I'm not sure what's the best way to proceed with this bug.  i.e. do we want to get an inverted alternative for each of these, or should we try to consolidate all of these into a single class or icon so it's easier to manage.
Whiteboard: [devedition-polish]
(In reply to Brian Grinstead [:bgrins] from comment #4)
> So, there are a lot of dropmarkers I've found so far in the theme (some have
> are inverted options, some don't):
> 
> OSX:
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/
> toolbarbutton-dropmarker-lion.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/
> toolbarbutton-dropmarker-lion@2x.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/
> toolbarbutton-dropmarker.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/
> folderDropArrow.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/
> folderDropArrow@2x.png
> 
> Windows:
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> toolbarbutton-dropdown-arrow-inverted.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> toolbarbutton-dropdown-arrow-XPVista7.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/
> toolbarbutton-dropdown-arrow.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/places/
> toolbarDropMarker-aero.png
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/places/
> toolbarDropMarker.png
> 
> On linux, there is a -moz-appearance: toolbarbutton-dropdown - I'm not
> seeing any icons, but I'm probably missing them.
> 
> I'm not sure what's the best way to proceed with this bug.  i.e. do we want
> to get an inverted alternative for each of these, or should we try to
> consolidate all of these into a single class or icon so it's easier to
> manage.

"Yes".

That is, we should use a single class of sorts, but we will need to keep some of the different images because of platform differences (the xpvista7 and windows 8 toolbarbutton-dropdown-arrows are not the same, for instance). The places specific ones should be removable.

The inverted variant from Windows should be usable on other platforms, too, and looks like it should already be in use here:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#618

I don't know why/when it isn't.
There are rough instructions in comment #5. For this bug it will be necessary to understand why the rule in browser.css linked there isn't good enough, to update it (or adjust other rules / add more) to ensure it works, and to do similar work on OSX and Linux. 

Please ignore:

(In reply to :Gijs Kruitbosch from comment #5)
> The places specific ones should be removable.

this bit.
Mentor: gijskruitbosch+bugs
Points: --- → 5
Whiteboard: [devedition-polish] → [devedition-polish][lang=css]
Assignee: nobody → scwwu
Hello Gijs, I've started working on the fix, and wonder if you could take a look? I'm not sure if I'm on the right track by changing the browser.css and devedition.css files. What I've done:

On devedition.css:
Check if devtoolstheme="dark", and use -moz-image-region to make the dropmarker white.

On browser.css:
The original dropmarker on toolbar (https://bug1000700.bmoattachments.org/attachment.cgi?id=8411559) doesn't have an inverted version, so I changed it to the same one as bookmarks.

Thanks!
Attachment #8745201 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745201 [details]
MozReview Request: Bug 1000700 - Part 1: Use white dropmarkers on dark dev edition theme on OSX; r?Gijs

https://reviewboard.mozilla.org/r/48939/#review45783

Did you intend to add other parts than just this one, as the commit message says "Part 1", but there is only one commit?

::: browser/themes/osx/browser.css:303
(Diff revision 1)
>  
>  #personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
>    -moz-box-orient: horizontal;
>  }
>  
> +.toolbarbutton-1 > .toolbarbutton-menu-dropmarker,

Rather than using the places icon here, I believe that the selectors you removed further down in this file...

::: browser/themes/osx/browser.css
(Diff revision 1)
> -.toolbarbutton-1 > .toolbarbutton-menu-dropmarker,
> -.toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> -  list-style-image: url(chrome://browser/skin/toolbarbutton-dropmarker.png);
> -}

... should be left here, and then have ones added for dark themes:

toolbar[brighttext] .toolbarbutton-1 > .toolbarbutton-menu-dropmarker

and the same for .toolbarbutton-menubutton-dropmarker, that use a darker image. It would probably make sense to have a shared inverted image included from browser/themes/shared/ that we use for this on all platforms.


You can then omit the devedition-specific fixes.
Alternatively, we could move + rename the places dropmarkers, assuming they're the right size etc. But we shouldn't make the main browser window rely on images in the places subfolders.
https://reviewboard.mozilla.org/r/48939/#review45783

Just this one for now because I wasn't sure if I'm on the right track. I proceed to work on the linux version once I got this one right :)

> ... should be left here, and then have ones added for dark themes:
> 
> toolbar[brighttext] .toolbarbutton-1 > .toolbarbutton-menu-dropmarker
> 
> and the same for .toolbarbutton-menubutton-dropmarker, that use a darker image. It would probably make sense to have a shared inverted image included from browser/themes/shared/ that we use for this on all platforms.
> 
> 
> You can then omit the devedition-specific fixes.

I see! So I should be using toolbar[brighttext] instead of messing with the devedition.

It seems that each OS has its own set of image assets and they all look and organized differently. I'll need to look into it to see if it's possible to have a shared inverted image.
Hello Gijs, while working on the linux theme, I found that part of the style rules for toolbar dropmarker come from the 'toolkit' rather than 'browser':
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#133

On the other hand, the OSX theme has the dropmarker styles all in the 'browser' directory (with some style rules from toolkit overridden):
https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#303
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/dropmarker.css

I'm not sure where the toolbar dropmarker css stylesheets and png files should belong to? in browser or toolkit? Having one OS does it one way and another does it the other way seems odd to me.

Thank you!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Scott Wu [:scottwu] from comment #12)
> Hello Gijs, while working on the linux theme, I found that part of the style
> rules for toolbar dropmarker come from the 'toolkit' rather than 'browser':
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> toolbarbutton.css#133
> 
> On the other hand, the OSX theme has the dropmarker styles all in the
> 'browser' directory (with some style rules from toolkit overridden):
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css#303
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> dropmarker.css
> 
> I'm not sure where the toolbar dropmarker css stylesheets and png files
> should belong to? in browser or toolkit? Having one OS does it one way and
> another does it the other way seems odd to me.

Yes, it's odd! Settle in for some attempts at explanations...

Toolkit is supposed to be "generic non-Firefox-specific stuff", so potentially useful/usable by Thunderbird, SeaMonkey, etc. In principle, "toolbarbutton dropdown arrows" can live there.

So why isn't it all there... well, some of this stuff, especially things like the bright text determination on toolbars, only exists in Firefox. So it would be incorrect to put selectors that rely on that in toolkit.

Then there are issues that toolkit generally kind of tried to stick with the styling of the OS it lives on. So toolkit windows styling is supposed to give you something that "fits in" on Windows, and OS X styling and Linux styling there will do the same for their respective OSes.

Now, the tricky bit is that for Firefox specifically, we have to various degrees moved away from that a little bit *especially* on Windows and OS X. Although since about v29, when we landed Australis, we've also slowly mostly (but not entirely) stopped using e.g. GTK icons on Linux, we still use lots of system colors, much more so than we do on Windows and OS X. There are some other smaller differences.

Then there's the "but we're all human" stuff: sometimes stuff doesn't end up in the 'right' place, or it's just Really Hard to get it right on all the platforms, in all the cases, for all the applications.

So this is why styles are split sometimes...



Now... for this particular bug. Hum. I think the images including inverted icons can live in toolkit. I don't know why the OS X ones (nb: not the places ones you linked, but https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/browser/themes/osx/browser.css#1082-1097 ) were not added to toolkit in the bug that added them (bug 559033 - Firefox 4 timeframe, 6 years ago!). Dão, do you know?

We've done some work on this type of thing before with close icons, moving them into toolkit and unifying their appearance, which might be helpful to look at: bug 851001.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Scott Wu [:scottwu] from comment #12)
> > I'm not sure where the toolbar dropmarker css stylesheets and png files
> > should belong to? in browser or toolkit? Having one OS does it one way and
> > another does it the other way seems odd to me.

I think we should pause for a moment here. Refactoring and unifying all the things might not be the best idea at this time. Some differences might be arbitrary, but there are likely good reasons for other differences. toolkit's toolbar and toolbarbutton styling is generally not very consistent across platforms because the look and feel of those platforms and the ways we integrate with them in terms of native theming are different.

If you're not sure what you're doing, I'd recommend sticking with the current "odd" structure.

(In reply to :Gijs Kruitbosch from comment #13)
> Now... for this particular bug. Hum. I think the images including inverted
> icons can live in toolkit.

They could. Generally we put images where they're used. If the inverted icon is used in browser but not in toolkit, it would normally go in browser, but if we put the non-inverted variant in toolkit, one could argue that putting the inverted variant there too makes some sense.

> I don't know why the OS X ones (nb: not the
> places ones you linked, but
> https://dxr.mozilla.org/mozilla-central/rev/
> fc15477ce628599519cb0055f52cc195d640dc94/browser/themes/osx/browser.css#1082-
> 1097 ) were not added to toolkit in the bug that added them (bug 559033 -
> Firefox 4 timeframe, 6 years ago!). Dão, do you know?

I don't know. I would guess it's because the OS X toolkit toolbarbutton styling is weird and crufty and nobody wanted to touch it.
Flags: needinfo?(dao+bmo)
Thank you Gijs and Dão! It's definitely clearer to me now.

If toolkit is for non-firefox-specific stuff then I think I should make the changes only in the browser instead of changing things in toolkit. Although there are similar icons in toolkit, they are not exactly the same, so I wouldn't say the inverted icon belong there.

I think I will put the icon and some common styles in the shared area of browser instead. Will let you know as soon as I have a patch :)
Attachment #8745201 - Attachment is obsolete: true
Hello Gijs, I moved the dropmark icons outside of the places folder and reference them in a shared css file.

Do you know how can I test my patch in different resolutions? The mac I use has retina display yet my linux doesn't, so I'm not certain if they are displayed correctly on different screens.
Comment on attachment 8749082 [details]
MozReview Request: Bug 1000700 - Use white dropmarkers on dark lwtheme on OSX and Linux; r?Gijs

https://reviewboard.mozilla.org/r/50743/#review47485

Some general notes:

1) we should put the image region stuff in shared/ as well.
2) what happens on OS X now? It doesn't look like you removed the existing styling for the dropdown icons there...

This looks like it's heading in the right direction. Was there a particular reason you've not tackled Windows yet?

At some point, it would probably be a good idea for Dão to review this as well.

::: browser/themes/linux/browser.css:163
(Diff revision 1)
> -.bookmark-item > .toolbarbutton-menu-dropmarker {
> -  display: none;
> +.bookmark-item > .toolbarbutton-menu-dropmarker,
> +.toolbarbutton-1 > .toolbarbutton-menu-dropmarker {
> +  -moz-appearance: none !important;
> +  -moz-image-region: rect(0, 7px, 5px, 0);
> +  -moz-margin-start: 3px;
> +}
> +
> +toolbar[brighttext] .bookmark-item > .toolbarbutton-menu-dropmarker,
> +toolbar[brighttext] .toolbarbutton-1 > .toolbarbutton-menu-dropmarker {
> +  -moz-image-region: rect(5px, 7px, 10px, 0);
> +}
> +
> +toolbar[brighttext] .bookmark-item:hover > .toolbarbutton-menu-dropmarker {
> +  -moz-image-region: rect(0px, 7px, 5px, 0);
> +}

This code should probably not be in the "places toolbar" section.

Note also that you're not styling toolbarbutton-menubutton-dropmarker on Linux, but you *are* doing that on OS X and in shared/ . We should probably do it everywhere, so these rules will need updating to include the menubutton dropmarker as well.

::: browser/themes/linux/browser.css:200
(Diff revision 1)
>  /* Dropmarker for folder bookmarks */
>  .bookmark-item[container] > .toolbarbutton-menu-dropmarker {
>    display: -moz-box !important;
>  }

Seems like this can be removed, too.

::: browser/themes/shared/jar.inc.mn:56
(Diff revision 1)
> +  skin/classic/browser/folderDropArrow.png                     (../shared/folderDropArrow.png)
> +  skin/classic/browser/folderDropArrow@2x.png                  (../shared/folderDropArrow@2x.png)

Because they're now not just used for places, we shouldn't call them "folderDropArrow". Let's use "toolbarbutton-dropmarker" instead.

::: browser/themes/shared/toolbarbuttons.inc.css:192
(Diff revision 1)
> +  .bookmark-item > .toolbarbutton-menu-dropmarker,
> +  .toolbarbutton-1 > .toolbarbutton-menu-dropmarker,
> +  .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> +    list-style-image: url(chrome://browser/skin/folderDropArrow@2x.png);
> +  }

If we're adding this here, we should add the region styling in here as well.
Attachment #8749082 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8749082 [details]
MozReview Request: Bug 1000700 - Use white dropmarkers on dark lwtheme on OSX and Linux; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50743/diff/1-2/
Attachment #8749082 - Flags: review?(gijskruitbosch+bugs)
(PSA: because I'm off early today, I likely won't get to the review until Monday. Sorry!)
https://reviewboard.mozilla.org/r/50743/#review47485

Thanks a lot for the review!

- I've moved the image region rules to shared.
- Duplicated rules on OSX has been removed.
- I haven't made any change to Windows because it looks fine to me on Window 7 + dev-edition.

> This code should probably not be in the "places toolbar" section.
> 
> Note also that you're not styling toolbarbutton-menubutton-dropmarker on Linux, but you *are* doing that on OS X and in shared/ . We should probably do it everywhere, so these rules will need updating to include the menubutton dropmarker as well.

I've moved the styles to shared so both OSX and linux use the same rules. Except I kept the :hover state for linux because the button background is highlighted.

> Because they're now not just used for places, we shouldn't call them "folderDropArrow". Let's use "toolbarbutton-dropmarker" instead.

Since "toolbarbutton-dropmarker" already exists, I renamed it to "toolbarbutton-dropmarkers" (because it's a sprite image with 2 dropmarkers). Hope that's not confusing.
Attachment #8749082 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8749082 [details]
MozReview Request: Bug 1000700 - Use white dropmarkers on dark lwtheme on OSX and Linux; r?Gijs

https://reviewboard.mozilla.org/r/50743/#review48101

We should make sure we do update Windows in this bug as well, otherwise the shared rules will mess with the existing Windows ones.

::: browser/themes/linux/browser.css:164
(Diff revision 2)
> -  display: none;
> +.bookmark-item:hover > .toolbarbutton-menu-dropmarker,
> +.bookmark-item[open="true"] > .toolbarbutton-menu-dropmarker,
> +toolbar[brighttext] .bookmark-item:hover > .toolbarbutton-menu-dropmarker {

Here and in the hidpi version, the bottom selector seems unnecessary - the contents of the rule are !important, and if the bottom selector (with toolbar[brighttext]) matches, the top one (without it) will match, too.

::: browser/themes/shared/toolbarbuttons.inc.css:17
(Diff revision 2)
> +  -moz-margin-start: 3px;
> +  -moz-margin-end: -2px;

This negative end margin came from the OS X theme. On Linux I see this rule moving the bookmarks item closer to its right-hand neighbour in a way that it's not supposed to do (the spacing between the buttons then looks off).

Can we leave the -moz-margin-end and -moz-margin-start properties in the OS X stylesheet?

Note that they were also specific to bookmarks items, and didn't apply to `.toolbarbutton-1` before.

::: browser/themes/shared/toolbarbuttons.inc.css:29
(Diff revision 2)
> +.bookmark-item > .toolbarbutton-menu-dropmarker {
> +  margin-top: 1px;
> +  -moz-margin-start: 3px;
> +  -moz-margin-end: -2px;
> +}

Then we can omit this rule (ie keep it in OS X, not in shared/).

::: browser/themes/shared/toolbarbuttons.inc.css:35
(Diff revision 2)
> +.toolbarbutton-1 > .toolbarbutton-menu-dropmarker {
> +  -moz-margin-end: 1px;
> +}

This too should probably stay in the OS X stylesheet.

::: browser/themes/shared/toolbarbuttons.inc.css:229
(Diff revision 2)
> +  .toolbarbutton-1 > .toolbarbutton-menu-dropmarker > .dropmarker-icon,
> +  .bookmark-item > .toolbarbutton-menu-dropmarker > .dropmarker-icon {
> +    width: 7px;
> +  }

This should apply to the menubutton-dropmarker as well.
https://reviewboard.mozilla.org/r/50743/#review48101

Apologies for the delay! I've updated the Windows style and removed duplicated rules.

> Here and in the hidpi version, the bottom selector seems unnecessary - the contents of the rule are !important, and if the bottom selector (with toolbar[brighttext]) matches, the top one (without it) will match, too.

Thanks for catching this. I've removed the unnecessary selector.

> This negative end margin came from the OS X theme. On Linux I see this rule moving the bookmarks item closer to its right-hand neighbour in a way that it's not supposed to do (the spacing between the buttons then looks off).
> 
> Can we leave the -moz-margin-end and -moz-margin-start properties in the OS X stylesheet?
> 
> Note that they were also specific to bookmarks items, and didn't apply to `.toolbarbutton-1` before.

It appears that the -moz-margin-start is still required. Otherwise the dropmarker would be touching the text.

> Then we can omit this rule (ie keep it in OS X, not in shared/).

Got it. Thanks.
Comment on attachment 8749082 [details]
MozReview Request: Bug 1000700 - Use white dropmarkers on dark lwtheme on OSX and Linux; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50743/diff/2-3/
Attachment #8749082 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8749082 [details]
MozReview Request: Bug 1000700 - Use white dropmarkers on dark lwtheme on OSX and Linux; r?Gijs

https://reviewboard.mozilla.org/r/50743/#review53120

I feel really bad about this, because I should have flagged this up before, but unfortunately this isn't going to work as-is. :-\

There are two main issues left with this patch:
1) on Linux, we can't rely on an image in the non-lwtheme case because of the native styling of the bookmark buttons (see also bug 734326 where this will change). Dão hinted at this already in comment #14. So if you apply this patch, and run it against e.g. the arc-dark theme on unity/ubuntu, on hover the dropdown icons are dark, when they should be light. The native appearance we use by default avoids this issue.
2) on Windows, we can't rely on the OSX-based icons (again, in the non-lwtheme case) because they don't fit with the colors of the other buttons, which are different depending on the Windows version.

The simplest solution I see here would be:
a) move the OSX images as you're doing now
b) keep the Linux- and OSX- and Windows-specific styling as-is for now, except:
c) update the OS X paths to match the new location of the images
d) add shared CSS that uses the shared images in the lwtheme case.

You can select for the lwtheme case using :-moz-lwtheme (any lightweight theme) and :-moz-lwtheme-brighttext (a dark lwtheme with bright/light text). I don't think using the toolbar selectors for [brighttext] will work well because they will apply in the Linux case (1) that I mentioned above, where we don't want them.

Sorry for not spotting these issues sooner. :-(
Attachment #8749082 - Flags: review?(gijskruitbosch+bugs)
Don't worry about it Gijs :)

I'll give it a try. Thanks for your feedback!
Sorry for not getting back to finishing this bug, as I've been primarily working on date time pickers. I was hoping to come back to it when I have some down time, but I think it's best to unassign myself from this bug for now, so anyone interested can dive right in.
Assignee: scwwu → nobody
Attachment #8749082 - Attachment is obsolete: true
Matt, is this fixed with Photon ?
Flags: needinfo?(MattN+bmo)
Maybe… I don't know how to get a dropmarker to show anymore on toolbars… maybe the design of Photon is to never show dropmarkers on the toolbars? Maybe someone who worked on Photon visuals would know better?
Flags: needinfo?(MattN+bmo)
Dão, is this still relevant ?
Flags: needinfo?(dao+bmo)
Don't think so.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(dao+bmo)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: