1018845 - Provide the ability to draw a white fullscreen button for dark themes

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

The native OS X full screen button is very hard to see on darker themes (most notably the new dark theme for private mode). Changing the icon itself is hard, but we could add something to the background that makes it more visible.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: fullscreen-buttons.png

(In reply to Philipp Sackl [:phlsa] from comment #0)
> The native OS X full screen button is very hard to see on darker themes
> (most notably the new dark theme for private mode). Changing the icon itself
> is hard, but we could add something to the background that makes it more
> visible.

How hard / possible would it be to change the icon itself?  Here is a screenshot for to compare the Spotify app with Firefox using a dark lightweight theme.  While there isn't a ton of contrast in the Spotify app, it's easier to see especially while hovering.

Comment 2

[Brian Grinstead [:bgrins]]

needinfo? for Comment 1 with regards the possibility of swapping the icon in a dark theme

Comment 3

[Markus Stange [:mstange]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> How hard / possible would it be to change the icon itself?

It's simple enough if the window is hardware-accelerated (which is the case for ~99.9% of our users according to http://people.mozilla.org/~bjacob/gfx_features_stats/#mac ), but hard if not.
If it's ok to limit this feature to hardware-accelerated systems I can whip up a patch fairly quickly. Would a simple invertfullscreenbutton="true" flag on the window be enough? Or would you like the ability to supply your own images?

Comment 4

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

(In reply to Markus Stange [:mstange] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > How hard / possible would it be to change the icon itself?
> 
> It's simple enough if the window is hardware-accelerated (which is the case
> for ~99.9% of our users according to
> http://people.mozilla.org/~bjacob/gfx_features_stats/#mac ), but hard if not.
> If it's ok to limit this feature to hardware-accelerated systems I can whip
> up a patch fairly quickly. Would a simple invertfullscreenbutton="true" flag
> on the window be enough? Or would you like the ability to supply your own
> images?

That limit is perfectly OK.
What does invertfullscreenbutton="true" do visually? Does it fully invert the icon or will it result in a very dimmed graphic like in the Spotify screenshot?
We probably want something with relatively high contrast since some themes produce a lot of visual noise in that corner.

Comment 5

[Markus Stange [:mstange]]

I was thinking of just literally inverting the existing fullscreen button image, i.e. getting an icon that's half-transparent white with a slightly more transparent black shadow. If that doesn't give enough contrast, we could draw it twice, on top of itself, I suppose...
Another solution would be to make the native fullscreen button invisible and set a background image on our existing #titlebar-fullscreen-button box. But then you wouldn't get hover or mousedown effects.

Comment 6

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

(In reply to Markus Stange [:mstange] from comment #5)
> I was thinking of just literally inverting the existing fullscreen button
> image, i.e. getting an icon that's half-transparent white with a slightly
> more transparent black shadow. If that doesn't give enough contrast, we
> could draw it twice, on top of itself, I suppose...
> Another solution would be to make the native fullscreen button invisible and
> set a background image on our existing #titlebar-fullscreen-button box. But
> then you wouldn't get hover or mousedown effects.

Hm, I *think* that would look odd because of the bevel effects on the button.
But Stephen is a lot more qualified to answer these kinds of questions :)

Comment 7

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

(In reply to Markus Stange [:mstange] from comment #5)
> I was thinking of just literally inverting the existing fullscreen button
> image, i.e. getting an icon that's half-transparent white with a slightly
> more transparent black shadow. If that doesn't give enough contrast, we
> could draw it twice, on top of itself, I suppose...
> Another solution would be to make the native fullscreen button invisible and
> set a background image on our existing #titlebar-fullscreen-button box. But
> then you wouldn't get hover or mousedown effects.

I think inverting it will roughly look like this: http://cl.ly/image/2r32302d0L34

Which isn't great, but better than what we have.

I think I would prefer an option to provide a higher contrast icon even if it means losing mouse states.

Comment 8

[Brian Grinstead [:bgrins]]

Sounds like the tradeoff of using a custom icon is that we could have a higher contrast icon but lose hover and active effects.  I'd be happy with either solution compared with how it looks now

Comment 9

[Brian Grinstead [:bgrins]]

Markus, is using a custom icon for this going to only require the CSS change you mention in Comment 5 or does hiding the native button require further changes?

Comment 10

[Markus Stange [:mstange]]

Hiding the native button requires further changes.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Markus Stange [:mstange] from comment #10)
> Hiding the native button requires further changes.

I'm assuming that the invertfullscreenbutton is an easier solution based on the previous comments.  Stephen, I'd love to get this in for Bug 1053434.  How would you feel about using the invertfullscreenbutton="true" if it was easier to do?

Comment 12

[Markus Stange [:mstange]]

Well, both of the approaches would require changes on the widget level, but hiding the native button is much easier than inverting it.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Markus Stange [:mstange] from comment #12)
> Well, both of the approaches would require changes on the widget level, but
> hiding the native button is much easier than inverting it.

OK, sounds like hiding native button and replacing it with a bg image for #titlebar-fullscreen-button is the way to go then, since that was what Stephen preferred anyway.

Modifying my needinfo in this case to be a request for the fullscreen icon :)

Comment 14

[Brian Grinstead [:bgrins]]

ni? for Comment 13

Comment 15

[Paul Rouget [:paul]]

In osx Yosemite (to be released very soon), there's no fullscreen button anymore.

Comment 16

[Markus Stange [:mstange]]

Attachment: proof of concept

How does this look? This patch turns the fullscreen button white by drawing it twice (to get more opacity) and then making all colors white, keeping the existing alpha. So the shadow also stays white.

Comment 17

[Markus Stange [:mstange]]

Attachment: screenshot

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8507017 [details] [diff] [review]
proof of concept

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

::: content/xul/content/src/nsXULElement.cpp
@@ +1158,5 @@
>                      // if the lwtheme changed, make sure to reset the document lwtheme cache
>                      nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(document);
>                      if (xuldoc) {
>                          xuldoc->ResetDocumentLWTheme();
> +                        SetBrightTitlebarForeground(document->GetDocumentLWTheme() == nsIDocument::Doc_Theme_Bright);

It would be great if there was some way to set the bright titlebar foreground  regardless of the applied lw theme for Bug 1053434.  We are trying to use the brighttext attribute to indicate if we should invert the other toolbar icons, so if there was some way to use that selector to toggle the fullscreen button as well that would be very helpful.  Or maybe some way to toggle this from ToolbarIconColor.inferFromText in browser.js.

Comment 19

[Markus Stange [:mstange]]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> We are
> trying to use the brighttext attribute to indicate if we should invert the
> other toolbar icons, so if there was some way to use that selector to toggle
> the fullscreen button as well that would be very helpful.

Great, then this patch should already be working as intended. document->GetDocumentLWTheme() == nsIDocument::Doc_Theme_Bright is true if and only if :-moz-lwtheme-brighttext matches.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Markus Stange [:mstange] from comment #19)
> (In reply to Brian Grinstead [:bgrins] from comment #18)
> > We are
> > trying to use the brighttext attribute to indicate if we should invert the
> > other toolbar icons, so if there was some way to use that selector to toggle
> > the fullscreen button as well that would be very helpful.
> 
> Great, then this patch should already be working as intended.
> document->GetDocumentLWTheme() == nsIDocument::Doc_Theme_Bright is true if
> and only if :-moz-lwtheme-brighttext matches.

I should clarify - in this case we will be need bright text but a lw theme will not be applied.  So it would be helpful if there was a way to cause this inversion to happen either from js in ToolbarIconColor.inferFromText or from a CSS selector that isn't :-moz-lwtheme-brighttext (instead something like [brighttext]).

Comment 21

[Markus Stange [:mstange]]

Oh, now I get it. Looks like we need an attribute after all. How about <window brighttitlebarforeground="true">?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Markus Stange [:mstange] from comment #21)
> Oh, now I get it. Looks like we need an attribute after all. How about
> <window brighttitlebarforeground="true">?

Yes, that would be perfect.  If it was drawn as inverted in either the case where the brighttitlebarforeground attribute is set or document->GetDocumentLWTheme() == nsIDocument::Doc_Theme_Bright, then we wouldn't even need to add extra checks to ToolbarIconColor.inferFromText to handle the lw theme case.  Otherwise there could be a block in there that set the attribute based on the window matching the ":-moz-lwtheme-brighttext" selector, and a second place where brighttitlebarforeground is set to true when the non-lw dark theme is applied.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> (In reply to Markus Stange [:mstange] from comment #21)
> > Oh, now I get it. Looks like we need an attribute after all. How about
> > <window brighttitlebarforeground="true">?
> 
> Yes, that would be perfect.  If it was drawn as inverted in either the case
> where the brighttitlebarforeground attribute is set or
> document->GetDocumentLWTheme() == nsIDocument::Doc_Theme_Bright, then we
> wouldn't even need to add extra checks to ToolbarIconColor.inferFromText to
> handle the lw theme case.  Otherwise there could be a block in there that
> set the attribute based on the window matching the
> ":-moz-lwtheme-brighttext" selector, and a second place where
> brighttitlebarforeground is set to true when the non-lw dark theme is
> applied.

Or we could add a check for the computed color of the #titlebar-content element in ToolbarIconColor.inferFromText and set the brighttitlebarforeground attribute based on that.  This would handle all different types of themes.

Comment 24

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

(In reply to Markus Stange [:mstange] from comment #16)
> Created attachment 8507017 [details] [diff] [review]
> proof of concept
> 
> How does this look? This patch turns the fullscreen button white by drawing
> it twice (to get more opacity) and then making all colors white, keeping the
> existing alpha. So the shadow also stays white.

Looks good! The inverting has maybe a little too much of a 3D effect, but definitely an improvement over the nearly invisible one we currently have :) Thanks!

Comment 25

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

Comment on attachment 8507017 [details] [diff] [review]
proof of concept

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=1018845#c24

Comment 26

[Joe Walker [:jwalker]]

This is unlikely to land in DevEdition right now.

Comment 27

[Brian Grinstead [:bgrins]]

Markus, is there any chance that you'll be able to finish this before the devedition release on the 10th?  I'm not sure how much work Comments 21 / 23 is.

Comment 28

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Markus, is there any chance that you'll be able to finish this before the
> devedition release on the 10th?  I'm not sure how much work Comments 21 / 23
> is.

IMO this is pretty important, I doubt 10.10 adoption will be very high yet. OS X users are pretty important to me.

Comment 29

[Markus Stange [:mstange]]

Attachment: patch

Comment 30

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8136a0983b7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Brian Grinstead [:bgrins]]

Thanks Markus, how do you feel about uplifting this to aurora?

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 8516355 [details] [diff] [review]
patch

This is not a risky patch, so I feel pretty good about it.

Approval Request Comment
[Feature/regressing bug #]: dark themes / bug 1053434
[User impact if declined]: fullscreen button hard to see
[Describe test coverage new/current, TBPL]: none
[Risks and why]: fairly low
[String/UUID change made/needed]: no IDL change (but IID update in nsIWidget.h)

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8516355 [details] [diff] [review]
patch

IID is ok for Aurora, so approving for uplift.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29c0cc65eafc