Closed
Bug 1018845
Opened 8 years ago
Closed 8 years ago
Provide the ability to draw a white fullscreen button for dark themes
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
People
(Reporter: phlsa, Assigned: mstange)
References
Details
Attachments
(3 files, 1 obsolete file)
119.61 KB,
image/png
|
Details | |
250.89 KB,
image/png
|
Details | |
12.39 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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+
Updated•8 years ago
|
Whiteboard: p=5
Comment 1•8 years ago
|
||
(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•8 years ago
|
||
needinfo? for Comment 1 with regards the possibility of swapping the icon in a dark theme
Flags: needinfo?(philipp)
Updated•8 years ago
|
Flags: needinfo?(mstange)
Assignee | ||
Comment 3•8 years ago
|
||
(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)
Reporter | ||
Comment 4•8 years ago
|
||
(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)
Assignee | ||
Comment 5•8 years ago
|
||
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.
Reporter | ||
Comment 6•8 years ago
|
||
(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)
Comment 7•8 years ago
|
||
(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)
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
Hiding the native button requires further changes.
Flags: needinfo?(mstange)
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
(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)
Comment 15•8 years ago
|
||
In osx Yosemite (to be released very soon), there's no fullscreen button anymore.
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
(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•8 years ago
|
||
(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]).
Assignee | ||
Comment 21•8 years ago
|
||
Oh, now I get it. Looks like we need an attribute after all. How about <window brighttitlebarforeground="true">?
Comment 22•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
(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)
Updated•8 years ago
|
Assignee: nobody → bgrinstead
Updated•8 years ago
|
Assignee: bgrinstead → mstange
Comment 25•8 years ago
|
||
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+
Comment 27•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
(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.
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8507017 -
Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8516355 -
Flags: review?(roc)
Attachment #8516355 -
Flags: review?(roc) → review+
Assignee | ||
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8136a0983b7
Comment 31•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e8136a0983b7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 32•8 years ago
|
||
Thanks Markus, how do you feel about uplifting this to aurora?
Flags: needinfo?(mstange)
Updated•8 years ago
|
Iteration: --- → 36.2
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5
Assignee | ||
Comment 33•8 years ago
|
||
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 34•8 years ago
|
||
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+
Comment 35•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/29c0cc65eafc
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
Updated•8 years ago
|
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
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•