Closed Bug 1018845 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: phlsa, Assigned: mstange)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Flags: firefox-backlog+
Whiteboard: p=5
Attached image 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.
needinfo? for Comment 1 with regards the possibility of swapping the icon in a dark theme
Flags: needinfo?(philipp)
(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?
Flags: needinfo?(mstange)
(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.
Flags: needinfo?(philipp)
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.
(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 :)
Flags: needinfo?(shorlander)
(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.
Flags: needinfo?(shorlander)
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
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?
Flags: needinfo?(mstange)
Hiding the native button requires further changes.
Flags: needinfo?(mstange)
(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?
Flags: needinfo?(shorlander)
Well, both of the approaches would require changes on the widget level, but hiding the native button is much easier than inverting it.
(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 :)
Flags: needinfo?(shorlander)
ni? for Comment 13
Flags: needinfo?(shorlander)
In osx Yosemite (to be released very soon), there's no fullscreen button anymore.
Attached patch proof of concept (obsolete) — Splinter Review
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.
Attachment #8507017 - Flags: feedback?(shorlander)
Attached image screenshot
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.
(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.
(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]).
Oh, now I get it. Looks like we need an attribute after all. How about <window brighttitlebarforeground="true">?
(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.
(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.
(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!
Flags: needinfo?(shorlander)
Assignee: nobody → bgrinstead
Assignee: bgrinstead → mstange
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
Attachment #8507017 - Flags: feedback?(shorlander) → feedback+
This is unlikely to land in DevEdition right now.
No longer blocks: 1053434
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.
Flags: needinfo?(mstange)
(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.
Attached patch patchSplinter Review
Attachment #8507017 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8516355 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/e8136a0983b7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Thanks Markus, how do you feel about uplifting this to aurora?
Flags: needinfo?(mstange)
Iteration: --- → 36.2
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5
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)
Flags: needinfo?(mstange)
Attachment #8516355 - Flags: approval-mozilla-aurora?
Comment on attachment 8516355 [details] [diff] [review]
patch

IID is ok for Aurora, so approving for uplift.
Attachment #8516355 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1000714
Component: Theme → Widget: Cocoa
Product: Firefox → Core
Summary: [UX] Add a background glow/outline/graphic to the fullscreen button on dark themes → Provide the ability to draw a white fullscreen button for dark themes
Target Milestone: Firefox 36 → mozilla36
Version: 31 Branch → Trunk
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.