Closed Bug 1408121 Opened 7 years ago Closed 7 years ago

Dark theme should apply dark colors to Firefox menus

Categories

(Firefox :: Theme, enhancement, P5)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
relnote-firefox --- 61+
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jaws, Assigned: masinico, Mentored)

References

(Depends on 4 open bugs, Regressed 3 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1408121: Dark theme should apply dark colors to Firefox menus.
59 bytes, text/x-review-board-request
ntim
: review+
jaws
: review+
Details
Enabling the dark theme changes the colors of the toolbars to be dark, but the menus still have a bright background. We have the same issue with the sidebar background color. Using a dark background will help users who may have chosen the Dark theme due to light sensitivity (using the browser in the dark).
Whiteboard: [photon-visual][triage]
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0) > Enabling the dark theme changes the colors of the toolbars to be dark, but > the menus still have a bright background. We have the same issue with the > sidebar background color. > > Using a dark background will help users who may have chosen the Dark theme > due to light sensitivity (using the browser in the dark). It doesn't really help with that, as Web content doesn't pick up our Dark theme. See also bug 1402312 comment 2. The side bar is bug 1385518, but that hasn't been a priority for the same reason.
Priority: -- → P5
See Also: → 1385518
Summary: Dark theme should apply dark colors to Firefox menus and sidebar → Dark theme should apply dark colors to Firefox menus
Whiteboard: [photon-visual][triage]
(In reply to Dão Gottwald [::dao] from comment #1) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0) > > Enabling the dark theme changes the colors of the toolbars to be dark, but > > the menus still have a bright background. We have the same issue with the > > sidebar background color. > > > > Using a dark background will help users who may have chosen the Dark theme > > due to light sensitivity (using the browser in the dark). > > It doesn't really help with that, as Web content doesn't pick up our Dark > theme. See also bug 1402312 comment 2. That is entirely up to the choice of the user. Users may only visit sites with dark backgrounds or may have installed an add-on that gives specific (or all) web pages a dark background color. The background colors of our menus are otherwise not customizable by an add-on and are under our control.
Users visiting only dark sites or installing an addon to make all Web content dark sounds like an extreme minority case that can't really justify a significant engineering effort here and the implied maintenance cost. To users suffering from clinical light sensitivity, I'd recommend using a dark high contrast theme on Windows; they take care of all these things automatically.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
(In reply to Dão Gottwald [::dao] from comment #3) > Users visiting only dark sites or installing an addon to make all Web > content dark sounds like an extreme minority case that can't really justify > a significant engineering effort here and the implied maintenance cost. To > users suffering from clinical light sensitivity, I'd recommend using a dark > high contrast theme on Windows; they take care of all these things > automatically. I hope you will see that this patch is actually pretty simple and it also allowed me the opportunity to remove the PNG versions of our arrows on OSX.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review194250 ::: browser/themes/shared/compacttheme.inc.css:101 (Diff revision 4) > +.panel-arrow { > + fill: var(--chrome-secondary-background-color); Can we introduce an --arrowpanel-background variable in the toolkit files and just set that in compacttheme.inc.css ? ::: browser/themes/shared/compacttheme.inc.css:105 (Diff revision 4) > +photonpanelmultiview .subviewbutton[checked="true"] { > + -moz-context-properties: fill; > + fill: currentColor; > +} I would prefer if this was directly set here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1366 instead of inside compacttheme.inc.css
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review194370 ::: browser/base/content/browser.css:380 (Diff revision 6) > .webextension-browser-action[cui-areatype="menu-panel"], > toolbarpaletteitem[place="palette"] > .webextension-browser-action { > list-style-image: var(--webextension-menupanel-image, inherit); > } > > + .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-brighttext, If you're planning to use :-moz-lwtheme-brighttext, then setting stuff inside compacttheme.inc.css is probably not a good idea. :-moz-lwtheme-brighttext also applies for dark themes from AMO, not just compact themes. ::: browser/themes/shared/customizableui/panelUI.inc.css:1196 (Diff revision 6) > +.panel-banner-item:not([disabled]):hover { > + background-color: hsla(96,65%,75%,.9); > +} > + > +.panel-banner-item:not([disabled]):hover:active { > + background-color: hsl(96,65%,75%); > +} These 2 rules are just overriding the 2 other ones above. Did you mean to include -moz-lwtheme-brighttext in the selector ? ::: toolkit/themes/linux/global/popup.css:46 (Diff revision 6) > } > > .panel-arrow[side="top"], > .panel-arrow[side="bottom"] { > list-style-image: var(--panel-arrow-image-vertical, > - url("chrome://global/skin/arrow/panelarrow-vertical-themed.svg")); > + url("chrome://global/skin/arrow/panelarrow-vertical.svg")); Did you forget to set -moz-context-properties, fill, stroke for linux ? ::: toolkit/themes/osx/global/popup.css:54 (Diff revision 6) > +.panel-arrow { > + -moz-context-properties: fill, stroke; > + fill: #fcfcfc; > + stroke: rgba(9,9,9,.16); > +} Sane here. ::: toolkit/themes/windows/global/popup.css:72 (Diff revision 6) > + fill: -moz-field; > + stroke: ThreeDShadow; This should probably be: fill: var(--arrowpanel-background); stroke: var(--arrowpanel-border-color); then compacttheme.inc.css will no longer need to set the fill for the panel arrow.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review194372 (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > (In reply to Dão Gottwald [::dao] from comment #3) > > Users visiting only dark sites or installing an addon to make all Web > > content dark sounds like an extreme minority case that can't really justify > > a significant engineering effort here and the implied maintenance cost. To > > users suffering from clinical light sensitivity, I'd recommend using a dark > > high contrast theme on Windows; they take care of all these things > > automatically. > > I hope you will see that this patch is actually pretty simple and it also > allowed me the opportunity to remove the PNG versions of our arrows on OSX. Your patch only changes arrow panels, not all menus, so the result is rather inconsistent. It also seems to have issues with hovered items and generally with platform colors such as threedshadow or graytext used in arrow panels. Aside from that, as I said in bug 1402312 comment 2, making transient UI dark is even counterproductive for users visiting average web sites. The dark theme is a cool gimmick, but let's not pretend it can be an effective accessibility feature.
Attachment #8918018 - Flags: review?(dao+bmo) → review-
Ideally beside panels the awesomebar and the search bar could benefit from this treatment. Not all menus are fit for a dark theme. Especially the context menus since they are system related and prone to change (on Windows I think they will be redesigned as part of the fluent design language. They will probably also have a dark theme to match the system wide option).
(In reply to Dão Gottwald [::dao] from comment #13) > Your patch only changes arrow panels, not all menus, so the result is rather > inconsistent. It also seems to have issues with hovered items and generally > with platform colors such as threedshadow or graytext used in arrow panels. Which other menus should I make this change to be more consistent? I could make the change to the context menus and/or menubar menus (File, Edit, View, History, etc on Windows), but since those right now are set to match native menu style I haven't touched them. Can you point out which issues you are seeing with threedshadow and graytext? I can update my patch to fix those issues. A generic response of "issues with hovered items and generally platform colors" isn't really helpful. > Aside from that, as I said in bug 1402312 comment 2, making transient UI > dark is even counterproductive for users visiting average web sites. The > dark theme is a cool gimmick, but let's not pretend it can be an effective > accessibility feature. Okay, I can drop the accessibility reasoning, but as comment 2 says, shorlander would like our theme to touch all of our surface areas match. I can understand your reaction towards the in-content UI, but this is not in-content UI.
Flags: needinfo?(dao+bmo)
This is a good bug, why not. I heard some complaints from users about inconsistencies with a dark theme that it is not completely dark, which is true. I think what should be considered to cover here is mainly the Hamburger menu and all its submenus, and Library menu and all its submenus. Perhaps also Overflow Menu box in Customize, Downloads button popup, Identity box popup, Page actions popup, Bookmark this page popup and Show your bookmarks popup. Everything else has probably native styles or is very minor.
Attachment #8918018 - Flags: review?(ntim.bugs) → review?(khudson)
Attachment #8918018 - Flags: review?(ntim.bugs)
There seems to be unrelated control center changes in the patch.
Flags: needinfo?(jaws)
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review195652 ::: browser/extensions/activity-stream/data/content/activity-stream.css:1143 (Diff revision 7) > .collapsible-section .section-top-bar .section-info-option .info-option::after, .collapsible-section .section-top-bar .section-info-option .info-option::before { > content: ""; > offset-inline-end: 0; > position: absolute; } > .collapsible-section .section-top-bar .section-info-option .info-option::before { > - background-image: url(chrome://global/skin/arrow/panelarrow-vertical-themed.svg), url(chrome://global/skin/arrow/panelarrow-vertical@2x.png); > + background-image: url(chrome://global/skin/arrow/panelarrow-vertical.svg), url(chrome://global/skin/arrow/panelarrow-vertical@2x.png); The `panelarrow-vertical@2x.png` shouldn't exist after this patch, right? So probably just use a single `background-image: url` and remove both entries from `browser_parsable_css`?
Curious, toolkit/themes/windows/global/arrow/panelarrow-vertical.svg has 20px width while toolkit/themes/osx/global/arrow/panelarrow-vertical.svg has 18px width. Do we know if it's expensive to use one file and resize the svg to the desired width? I suppose it's not visually exactly the same… Also anyone know where's the history of having the different widths for non-mac and mac?
@ntim, the control center changes are necessary. If you open up control center you will see that we need to invert the icons there. @mardak, the arrows were reduced in size on OSX by bug 968595. The benefit of sharing the SVG during the build process and before doesn't seem worth the possible skewing and potential other oddness that would happen by changing the aspect ratio of the asset during runtime.
Flags: needinfo?(jaws)
Flags: needinfo?(dao+bmo)
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review195890 Aside from these code comments, I'm not quite happy with the UX, notably how the panel borders/separators and hover states are barely visible. I found these values to work well for both light/dark themes: --panel-separator-color: hsla(210,4%,50%,.2); --arrowpanel-dimmed: hsla(210,4%,50%,.1); --arrowpanel-dimmed-further: hsla(210,4%,50%,.18); --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25); --arrowpanel-border-color: hsla(210,4%,50%,.1); ::: browser/themes/shared/controlcenter/conn-not-secure.svg:6 (Diff revision 9) > <?xml version="1.0" encoding="utf-8"?> > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> > #include ../icon-colors.inc.svg You can remove the #include (and the preprocessing indicator in jar.mn) now that you no longer use the `fieldtext` class. ::: browser/themes/shared/controlcenter/panel.inc.css:230 (Diff revision 9) > + -moz-context-properties: fill, fill-opacity; > + fill: fieldtext; > + fill-opacity: .6; You can set fill: currentColor; directly here, and remove the declaration from compacttheme.css -moz-fieldtext is the text color of the panels by default (since -moz-field is the background), see: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#28 https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#38 Aside from that, I think fieldtext is an invalid color (not sure how that worked in the SVG), the correct color is -moz-fieldtext: https://dxr.mozilla.org/mozilla-central/search?q=fieldtext&redirect=false ::: browser/themes/shared/controlcenter/panel.inc.css:342 (Diff revision 9) > + fill: fieldtext; > + fill-opacity: .6; Same here ::: browser/themes/shared/controlcenter/panel.inc.css:372 (Diff revision 9) > + -moz-context-properties: fill, fill-opacity; > + fill: fieldtext; > + fill-opacity: .6; Same here. ::: browser/themes/shared/controlcenter/permissions.svg:7 (Diff revision 9) > #include ../icon-colors.inc.svg > Same here ::: browser/themes/shared/controlcenter/tracking-protection.svg:7 (Diff revision 9) > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" > width="24" height="24" viewBox="0 0 24 24"> > #include ../icon-colors.inc.svg Same here ::: toolkit/themes/osx/global/arrow/panelarrow-horizontal.svg:6 (Diff revision 9) > + <path d="M 10,0 L 0,9 10,18 z" fill="rgba(9,9,9,.16)"/> > + <path d="M 10,1 L 1,9 10,17 z" fill="#fcfcfc"/> Shouldn't this use context-fill/stroke ? ::: toolkit/themes/osx/global/popup.css:56 (Diff revision 9) > margin: 1px; > } > > +.panel-arrow { > + -moz-context-properties: fill, stroke; > + fill: var(--arrowpanel-background); arrowpanel-background is a linear-gradient on OSX, so this won't work. https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/global.css#20 You could add a fallback value: fill: var(--arrowpanel-background, #fff) ::: toolkit/themes/shared/non-mac.jar.inc.mn (Diff revision 9) > - skin/classic/global/arrow/panelarrow-horizontal-themed.svg (../../windows/global/arrow/panelarrow-horizontal-themed.svg) > - skin/classic/global/arrow/panelarrow-vertical-themed.svg (../../windows/global/arrow/panelarrow-vertical-themed.svg) With your patch, panelarrow-vertical/horizontal.svg is no longer packaged on Linux. You might want to move the panelarrow-vertical/horizontal.svg entries from the windows jar.mn to the non-mac.jar.inc.mn file to fix that. ::: toolkit/themes/windows/global/arrow/panelarrow-vertical.svg:6 (Diff revision 9) > - <path d="M 0,10 L 10,0 20,10 z" fill="hsla(210,4%,10%,.2)"/> > - <path d="M 1,10 L 10,1 19,10 z" fill="-moz-field"/> > + <path d="M 0,10 L 10,0 20,10 z" fill="context-stroke"/> > + <path d="M 1,10 L 10,1 19,10 z" fill="context-fill"/> > </svg> The equivalent change should be done for panelarrow-horizontal.svg on Windows. ::: toolkit/themes/windows/global/popup.css:96 (Diff revision 9) > margin-top: -5px; > } > > .panel-arrow[side="left"], > .panel-arrow[side="right"] { > - list-style-image: url("chrome://global/skin/arrow/panelarrow-horizontal-themed.svg"); > + list-style-image: url("chrome://global/skin/arrow/panelarrow-horizontal.svg"); I'm not seeing the windows version of this file in the patch.
Attachment #8918018 - Flags: review?(ntim.bugs)
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review195980 ::: browser/themes/shared/compacttheme.inc.css:43 (Diff revision 10) > + /* Arrow panels */ > + --panel-separator-color: hsla(210,4%,50%,.2); > + --arrowpanel-dimmed: hsla(210,4%,50%,.1); > + --arrowpanel-dimmed-further: hsla(210,4%,50%,.18); > + --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25); > + --arrowpanel-border-color: hsla(210,4%,50%,.1); I don't like the idea of putting more special overrides inside compacttheme.inc.css, because: - these overrides get lost when we change/refactor the original rules (bug 1387043), and cause regressions/dead code when we do. - it makes it hard to understand why an override is there and if it's still needed - it makes it harder to convert the theme to a normal WE theme I think we should only add new rules if we don't have alternative solutions. In this case, I believe those colors should work across all themes (default/light/dark), without changing the default theme's look noticeably. Bug 1388589 also took the same approach of taking colors that work for both themes. ::: browser/themes/shared/compacttheme.inc.css:95 (Diff revision 10) > +#appMenu-zoomReset-button:-moz-lwtheme-brighttext { > + background-color: var(--chrome-secondary-background-color); > + background-image: linear-gradient(rgba(255,255,255,.2), rgba(255,255,255,.2)); > + color: var(--chrome-color); > +} > + > +#appMenu-zoomReset-button:hover:-moz-lwtheme-brighttext { > + background-image: linear-gradient(rgba(255,255,255,.25), rgba(255,255,255,.25)); > +} > + > +#appMenu-zoomReset-button:hover:active:-moz-lwtheme-brighttext { > + background-image: linear-gradient(rgba(255,255,255,.15), rgba(255,255,255,.15)); > +} I'm confident we can find more generic colors that we can just put by default. Using a gray with some opacity should work for both themes. ::: browser/themes/shared/compacttheme.inc.css:109 (Diff revision 10) > +.subviewbutton[shortcut]::after { > + color: var(--chrome-color); > + opacity: .5; > +} It would be nice to add a comment on what this does, in case we want to do housekeeping on this file later. ::: browser/themes/shared/compacttheme.inc.css:144 (Diff revision 10) > +.panel-banner-item:-moz-lwtheme-brighttext { > + color: black; > + background-color: hsla(96,65%,75%,.8); > +} > + > +.panel-banner-item:not([disabled]):hover:-moz-lwtheme-brighttext { > + background-color: hsla(96,65%,75%,.9); > +} > + > +.panel-banner-item:not([disabled]):hover:active:-moz-lwtheme-brighttext { > + background-color: hsl(96,65%,75%); > +} > + Using more transparent colors and white text makes more sense to me, but if we can find colors that fit for both themes, that would be even better. Also, please add a comment explaining why this is needed. ::: browser/themes/shared/compacttheme.inc.css:157 (Diff revision 10) > +@media not all and (min-resolution: 1.1dppx) { > + .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-brighttext { > + list-style-image: var(--webextension-menupanel-image-light, inherit); > + } > +} > + > +@media (min-resolution: 1.1dppx) { > + .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-brighttext { > + list-style-image: var(--webextension-menupanel-image-2x-light, inherit); > + } > +} I wonder if there's a more elegant solution to this? This override can easily get forgotten when refactoring/changing https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#357 ::: browser/themes/shared/controlcenter/panel.inc.css:341 (Diff revision 10) > + -moz-context-properties: fill, fill-opacity; > + fill: currentColor; > + fill-opacity: .6; Can you put this in the shared rule here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/controlcenter/panel.inc.css#111 to avoid repeating this? ::: toolkit/themes/osx/global/popup.css:56 (Diff revision 10) > margin: 1px; > } > > +.panel-arrow { > + -moz-context-properties: fill, stroke; > + fill: var(--arrowpanel-background, hsla(0,0%,99%,1)); So it turns out the fallback mechanism doesn't work here. What you could do is: - define the fill to be hsla(0,0%,99%,1) here - add a special rule for .panel-arrow in osx/compacttheme.css ::: toolkit/themes/windows/global/popup.css:113 (Diff revision 10) > .panel-arrowcontent { > --arrowpanel-border-color: hsla(210,4%,10%,.2); > box-shadow: 0 0 4px hsla(210,4%,10%,.2); > } > > - .panel-arrow[side="top"], > - .panel-arrow[side="bottom"] { > + .panel-arrow { > + --arrowpanel-border-color: hsla(210,4%,10%,.2); nit: It would be cleaner not to repeat the border-color twice: :root { --arrowpanel-border-color: hsla(210,4%,10%,.2); } .panel-arrowcontent { box-shadow: ... }
Attachment #8918018 - Flags: review?(ntim.bugs)
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review196010 ::: toolkit/themes/osx/global/popup.css:56 (Diff revision 10) > margin: 1px; > } > > +.panel-arrow { > + -moz-context-properties: fill, stroke; > + fill: var(--arrowpanel-background, hsla(0,0%,99%,1)); Looks like the default theme on OSX specifies a linear-gradient for this value, which I'm pretty sure is an invalid value for fill here; (it looks broken on Mac OSX for me)
The activity stream stuff looks good to me :)
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review195980 > I wonder if there's a more elegant solution to this? This override can easily get forgotten when refactoring/changing https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#357 Without full support for Themes here, I just put a comment in the file pointing to the other place that edits this.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review196242 Looks mostly fine to me. Here are a few more comments. ::: browser/themes/linux/browser.css:32 (Diff revision 11) > > - --panel-separator-color: ThreeDShadow; > - --arrowpanel-dimmed: hsla(0,0%,80%,.3); > - --arrowpanel-dimmed-further: hsla(0,0%,80%,.45); > - --arrowpanel-dimmed-even-further: hsla(0,0%,80%,.8); > + --panel-separator-color: hsla(210,4%,50%,.2); > + --arrowpanel-dimmed: hsla(210,4%,50%,.1); > + --arrowpanel-dimmed-further: hsla(210,4%,50%,.18); > + --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25); > + --arrowpanel-border-color: hsla(210,4%,50%,.1); Is there a reason not to replace the --arrowpanel-border-color value in toolkit directly ? Notably in those files: /toolkit/themes/(osx|windows|linux)/global/global.css ::: browser/themes/osx/browser.css:33 (Diff revision 11) > > - --panel-separator-color: hsla(210,4%,10%,.14); > - --arrowpanel-dimmed: hsla(210,4%,10%,.07); > - --arrowpanel-dimmed-further: hsla(210,4%,10%,.12); > - --arrowpanel-dimmed-even-further: hsla(210,4%,10%,.17); > + --panel-separator-color: hsla(210,4%,50%,.2); > + --arrowpanel-dimmed: hsla(210,4%,50%,.1); > + --arrowpanel-dimmed-further: hsla(210,4%,50%,.18); > + --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25); > + --arrowpanel-border-color: hsla(210,4%,50%,.1); Same question here. ::: browser/themes/osx/compacttheme.css:13 (Diff revision 11) > + /* The second argument to var() is not supported > + yet so the we cannot rely on the fallback to It is supported :) I just didn't understand how it worked exactly. It only works when the variable is not set, but if the variable has a incorrect value, it uses initial and does not use the fallback value. Chrome behaves the same way, so I'm guessing this is the spec-defined behaviour ? ::: browser/themes/shared/compacttheme.inc.css:141 (Diff revision 11) > +@media not all and (min-resolution: 1.1dppx) { > + .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-brighttext { > + list-style-image: var(--webextension-menupanel-image-light, inherit); > + } > +} > + > +@media (min-resolution: 1.1dppx) { > + .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-brighttext { > + list-style-image: var(--webextension-menupanel-image-2x-light, inherit); > + } > +} nit: add a comment for this rule ::: browser/themes/shared/customizableui/customizeMode.inc.css:468 (Diff revision 11) > - url("chrome://global/skin/arrow/panelarrow-vertical-themed.svg")); > %endif > + list-style-image: var(--panel-arrow-image-vertical, > + url("chrome://global/skin/arrow/panelarrow-vertical.svg")); > + -moz-context-properties: fill, stroke; > + fill: var(--arrowpanel-background); This needs a similar fix for macOS unfortunately. ::: browser/themes/shared/customizableui/panelUI.inc.css:1414 (Diff revision 11) > +#appMenu-zoomReset-button { > + background-color: -moz-field; > + background-image: linear-gradient(rgba(204,204,204,.15), rgba(204,204,204,.15)); > + color: -moz-fieldtext; > +} > + > +#appMenu-zoomReset-button:hover { > + background-image: linear-gradient(rgba(204,204,204,.3), rgba(204,204,204,.3)); > +} > + > +#appMenu-zoomReset-button:hover:active { > + background-image: linear-gradient(rgba(204,204,204,.6), rgba(204,204,204,.6)); > +} Can we just fix the border and background directly here instead of these extra rules: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1425 ? ::: browser/themes/windows/browser.css:34 (Diff revision 11) > > - --panel-separator-color: ThreeDLightShadow; > - --arrowpanel-dimmed: hsla(0,0%,80%,.3); > - --arrowpanel-dimmed-further: hsla(0,0%,80%,.45); > - --arrowpanel-dimmed-even-further: hsla(0,0%,80%,.8); > + --panel-separator-color: hsla(210,4%,50%,.2); > + --arrowpanel-dimmed: hsla(210,4%,50%,.1); > + --arrowpanel-dimmed-further: hsla(210,4%,50%,.18); > + --arrowpanel-dimmed-even-further: hsla(210,4%,50%,.25); > + --arrowpanel-border-color: hsla(210,4%,50%,.1); same question here.
Attachment #8918018 - Flags: review?(ntim.bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > (In reply to Dão Gottwald [::dao] from comment #13) > > Your patch only changes arrow panels, not all menus, so the result is rather > > inconsistent. It also seems to have issues with hovered items and generally > > with platform colors such as threedshadow or graytext used in arrow panels. > > Which other menus should I make this change to be more consistent? I could > make the change to the context menus and/or menubar menus (File, Edit, View, > History, etc on Windows), but since those right now are set to match native > menu style I haven't touched them. I don't understand how this difference matters from the user's POV. > Can you point out which issues you are seeing with threedshadow and > graytext? I can update my patch to fix those issues. A generic response of > "issues with hovered items and generally platform colors" isn't really > helpful. It's quite simple: platform colors are moving targets, so if you use a platform color on a hardcoded background, there will likely be situations where they aren't visible enough.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review196314 ::: browser/themes/linux/browser.css:28 (Diff revision 11) > > --toolbarbutton-border-radius: 4px; > --toolbarbutton-vertical-text-padding: calc(var(--toolbarbutton-inner-padding) - 1px); > --toolbarbutton-icon-fill-opacity: .85; > > - --panel-separator-color: ThreeDShadow; > + --panel-separator-color: hsla(210,4%,50%,.2); This regresses high contrast support. ::: browser/themes/shared/icon-colors.inc.svg:13 (Diff revision 11) > .white { > fill: white; > fill-opacity: .7; > } > > </style> It looks like this patch makes this file unused. ::: browser/themes/windows/browser.css:30 (Diff revision 11) > > --toolbarbutton-vertical-text-padding: calc(var(--toolbarbutton-inner-padding) - 1px); > --toolbarbutton-border-radius: 2px; > --toolbarbutton-icon-fill-opacity: 1; > > - --panel-separator-color: ThreeDLightShadow; > + --panel-separator-color: hsla(210,4%,50%,.2); Ditto for regressing high contrast support.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review199676 ::: browser/themes/linux/browser.css:28 (Diff revision 12) > > --toolbarbutton-border-radius: 4px; > --toolbarbutton-vertical-text-padding: calc(var(--toolbarbutton-inner-padding) - 1px); > --toolbarbutton-icon-fill-opacity: .85; > > - --panel-separator-color: ThreeDShadow; > + --panel-separator-color: hsla(210,4%,50%,.2); This regresses high contrast support as well. It should probably set in the linux specific compacttheme.css file. ::: browser/themes/shared/controlcenter/tracking-protection.svg:35 (Diff revision 12) > - <use class="fieldtext" xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout)"/> > + <use fill="context-fill" fill-opacity="context-fill-opacity" xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout)"/> > </g> > > <g id="disabled"> > - <use class="fieldtext" xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout-disabled)"/> > + <use fill="context-fill" fill-opacity="context-fill-opacity" xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout-disabled)"/> minor nit: You can apply fill and fill-opacity on the root <svg> element to avoid setting it on both elements. ::: browser/themes/shared/customizableui/customizeMode.inc.css:469 (Diff revision 12) > + fill: hsla(0,0%,99%,1); > %else > - list-style-image: var(--panel-arrow-image-vertical, > + fill: var(--arrowpanel-background); > - url("chrome://global/skin/arrow/panelarrow-vertical-themed.svg")); > %endif > + list-style-image: var(--panel-arrow-image-vertical, (Not introduced with this patch specifically) but I've noticed --panel-arrow-image-vertical isn't useful in this context (customize mode), since only extension popups use it: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ExtensionPopups.jsm#364 ::: browser/themes/shared/customizableui/panelUI.inc.css:1482 (Diff revision 12) > + background-color: -moz-field; > + background-image: linear-gradient(rgba(204,204,204,.15), rgba(204,204,204,.15)); > + color: -moz-fieldtext; Since compact themes don't use the high contrast colors, this will look off in that configuration. I found this CSS to work quite well for all themes: #appMenu-zoomReset-button { background-color: rgba(204,204,204,.15); border: 1px solid var(--panel-separator-color); border-radius: 10000px; padding: 1px 8px; } #appMenu-zoomReset-button:hover { background-color: rgba(204,204,204,.3); } #appMenu-zoomReset-button:hover:active { background-color: rgba(204,204,204,.6); } ::: browser/themes/windows/compacttheme.css:178 (Diff revision 12) > +@media not all and (-moz-windows-default-theme) { > + .subviewbutton[shortcut]::after { > + color: GrayText; > + opacity: 1; > + } > +} The compact themes doesn't actually use high contrast colors, so this will look off with the compact theme enabled.
Attachment #8918018 - Flags: review?(ntim.bugs) → review+
Attachment #8918018 - Flags: review?(dao+bmo)
Dão and Kate, review ping? I just ran into the broken referencing of panelarrow-vertical-themed.svg which bug 1405539 comment 17 notes this fixes, but unfortunately this patch hasn't had any further feedback over the last 2 weeks...
Flags: needinfo?(khudson)
Flags: needinfo?(dao+bmo)
Will other theme authors have the option to do this? Someone on twitter noticed this problem affects many (all?) themes in amo.
Blocks: 1417880
(In reply to Ben Kelly [:bkelly] from comment #41) > Will other theme authors have the option to do this? Someone on twitter > noticed this problem affects many (all?) themes in amo. Bug 1417880 is filed for this.
Bug 1415812 landed for activity stream removing the double-url usage for `background-image: url(chrome://global/skin/arrow/panelarrow-vertical-themed.svg), url(chrome://global/skin/arrow/panelarrow-vertical@2x.png);` and using the appropriate one per-platform as well as fixing up the size/position to match (18x10 vs 20x10): https://searchfox.org/mozilla-central/search?q=panelarrow&path=activity-stream
See Also: → 1415812
Woot. So glad someone is looking at this. Has bothered me for a while. For websites I highly recommend https://addons.mozilla.org/en-US/firefox/addon/dark-mode-webextension/ BTW.
Comment on attachment 8918018 [details] Bug 1408121 - Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/188896/#review209732 ::: browser/base/content/browser.css:363 (Diff revision 15) > height: 16px; > width: 16px; > } > > +/* Note that these styles are changed in compacttheme.inc.css when > + the Dark compact theme is enabled. */ The Dark theme is not a compact theme ::: browser/themes/osx/compacttheme.css:14 (Diff revision 15) > background: var(--chrome-background-color); > } > > +#customization-panelWrapper > .panel-arrowbox > .panel-arrow[side="top"], > +.panel-arrow { > + /* We can use the CSS variable in compact themes Please rephrase; these themes aren't "compact". ::: browser/themes/shared/customizableui/panelUI.inc.css:1478 (Diff revision 15) > text-align: center; > } > > +#appMenu-zoomReset-button { > + min-height: unset; /* Unset min-height since this is a text-only button. */ > + background-color: rgba(204,204,204,.15); --arrowpanel-dimmed? ::: browser/themes/shared/customizableui/panelUI.inc.css:1485 (Diff revision 15) > + border-radius: 10000px; > + padding: 1px 8px; > +} > + > +#appMenu-zoomReset-button:hover { > + background-color: rgba(204,204,204,.3); --arrowpanel-dimmed-further? ::: browser/themes/shared/customizableui/panelUI.inc.css:1489 (Diff revision 15) > +#appMenu-zoomReset-button:hover { > + background-color: rgba(204,204,204,.3); > +} > + > +#appMenu-zoomReset-button:hover:active { > + background-color: rgba(204,204,204,.6); --arrowpanel-dimmed-even-further? ::: browser/themes/windows/compacttheme.css:178 (Diff revision 15) > #titlebar-close:-moz-lwtheme { > list-style-image: url(chrome://browser/skin/window-controls/close.svg); > } > } > > +@media not all and (-moz-windows-default-theme) { @media (-moz-windows-default-theme: 0) { ::: browser/themes/windows/compacttheme.css:182 (Diff revision 15) > > +@media not all and (-moz-windows-default-theme) { > + .subviewbutton[shortcut]::after { > + /* Increase the opacity to increase the contrast > + with high-contrast themes. */ > + opacity: 1; Won't this make the shortcut use the same color as the main label? ::: toolkit/themes/osx/global/toolbarbutton.css:15 (Diff revision 15) > margin: 0 2px; > padding: 3px 2px; > background-color: transparent; > +} > + > +@media not all and (-moz-mac-yosemite-theme) { @media (-moz-mac-yosemite-theme: 0) { ::: toolkit/themes/osx/global/toolbarbutton.css:17 (Diff revision 15) > background-color: transparent; > +} > + > +@media not all and (-moz-mac-yosemite-theme) { > + toolbarbutton:not(:-moz-lwtheme-brighttext) { > - text-shadow: 0 1px 0 rgba(255, 255, 255, 0.4); > + text-shadow: 0 1px 0 rgba(255, 255, 255, 0.4); So this is only for OS X 10.9? Should we just remove this rule altogether?
Attachment #8918018 - Flags: review?(dao+bmo)
Most of these changes happened in bug 1425868. What hasn't been done is: - using neutral colors for dark/light themes on arrowpanel-dimmed - Removing the text-shadow from the macOS toolbarbutton styling - actual changes in compacttheme.css
Flags: needinfo?(dao+bmo)
Flags: needinfo?(khudson)
(In reply to Dão Gottwald [::dao] from comment #3) > Users visiting only dark sites or installing an addon to make all Web > content dark sounds like an extreme minority case that can't really justify > a significant engineering effort here and the implied maintenance cost. To > users suffering from clinical light sensitivity, I'd recommend using a dark > high contrast theme on Windows; they take care of all these things > automatically. Hi, I am such a user and I work at Mozilla. http://paul.bone.id.au/2017/11/26/how-i-see-the-web/ Having a high contrast reverse view in Linux (I don't know about windows) does not change web content. You have to go further, like you suggested a plugin or something. Firefox mostly picks up the theme from Linux, I don't change Firefox's theme itself at all. But there appear to be minor cases bug 1429288. It might be an extreme minority, however these are web users that we should care about. Their access to the web isn't any less important than your own. To dismiss a group because their numbers are so few (or for other reasons) is called discrimination. Please don't do that.
Keywords: access
(In reply to Paul Bone [:pbone] from comment #48) > (In reply to Dão Gottwald [::dao] from comment #3) > > Users visiting only dark sites or installing an addon to make all Web > > content dark sounds like an extreme minority case that can't really justify > > a significant engineering effort here and the implied maintenance cost. To > > users suffering from clinical light sensitivity, I'd recommend using a dark > > high contrast theme on Windows; they take care of all these things > > automatically. > > Hi, I am such a user and I work at Mozilla. > > http://paul.bone.id.au/2017/11/26/how-i-see-the-web/ > > Having a high contrast reverse view in Linux (I don't know about windows) > does not change web content. You have to go further, like you suggested a > plugin or something. You should probably file a bug on this; it might be a problem in our Gtk widget implementation. This bug wouldn't change that, so while I appreciate your input, your comment is rather off-topic here.
Keywords: access
(In reply to Dão Gottwald [::dao] from comment #49) > (In reply to Paul Bone [:pbone] from comment #48) > > (In reply to Dão Gottwald [::dao] from comment #3) > > > Users visiting only dark sites or installing an addon to make all Web > > > content dark sounds like an extreme minority case that can't really justify > > > a significant engineering effort here and the implied maintenance cost. To > > > users suffering from clinical light sensitivity, I'd recommend using a dark > > > high contrast theme on Windows; they take care of all these things > > > automatically. > > > > Hi, I am such a user and I work at Mozilla. > > > > http://paul.bone.id.au/2017/11/26/how-i-see-the-web/ > > > > Having a high contrast reverse view in Linux (I don't know about windows) > > does not change web content. You have to go further, like you suggested a > > plugin or something. > > You should probably file a bug on this; it might be a problem in our Gtk > widget implementation. This bug wouldn't change that, so while I appreciate > your input, your comment is rather off-topic here. Sorry for the confusion, I didn't mean to suggest that my problems where related to this bug (there are seperate bugs for the relevant parts). But that while discussing this bug you referred to a minority of users with a clinical light sensativity and dismissed that as not important enough to justify the engineering effort. So I'm introducing myself as a member of that group, and of your community, for whom this is a problem. And pointing out that what you did was discrimination (however I don't think you meant to cause any harm). My hope is that I can help you and others see how your comment was discriminatory. Whether my actual bug is with Firefox's detection of a linux theme / a GTK thing / or this bug is irrelevent to my point here. Thanks.
(In reply to Paul Bone [:pbone] from comment #50) > But that while discussing this bug you referred to a minority of users with a > clinical light sensativity and dismissed that as not important enough to > justify the engineering effort. No, I was referring to at least two groups of users, and I said that users with clinical light sensitivity should use a dark high contrast theme because this bug was wrongly being sold as an accessibility feature.
bug 239914 covers high contrast theme detection on Linux. Unfortunately gtk is useless and there is (or at least was, when I last looked at this) no way to know whether a theme is high contrast except "look at the name and apply heuristics" (ie is it called "High Contrast <whatever>" or "HC something" or "foopy Dark" or ...). Perhaps things are better on more recent versions of gtk/gnome/...
Attachment #8918018 - Attachment is obsolete: true
Attachment #8918018 - Flags: review?(khudson)
Attachment #8918018 - Attachment is obsolete: false
Attachment #8918018 - Attachment is obsolete: true
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Assignee: nobody → masinico
Status: NEW → ASSIGNED
Mentor: jaws, mconley, ntim.bugs
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review225054 ::: browser/themes/shared/customizableui/panelUI.inc.css:988 (Diff revision 4) > .PanelUI-subView .toolbaritem-combined-buttons > .subviewbutton:not(.subviewbutton-iconic) { > background-color: #f9f9f9; > border: 1px solid #e1e1e1; > border-radius: 10000px; > padding: 1px 8px; > } can you combine this with your #appMenu-zoomReset-button rule ? Also, the panelUI.inc.css changes (not the compacttheme.inc.css changes) might be better in bug 1417880.
No longer blocks: 1417880
Depends on: 1417880
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review226074 ::: browser/themes/shared/compacttheme.inc.css (Diff revision 8) > > .tab-icon-sound[soundplaying], > .tab-icon-sound[muted] { > filter: none !important; /* removes drop-shadow filter */ > } > - Can you add this line back? I bet your editor is removing the trailing new line.
Attachment #8950011 - Flags: review?(jaws) → review+
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review226078 ::: toolkit/components/extensions/test/browser/browser_ext_themes_dark_theme_arrowpanels.js:27 (Diff revision 8) > + const POPUP_TEXT_COLOR = "rgb(249, 249, 250)"; > + const POPUP_BORDER_COLOR = "#808080"; > + const COMPACT_DARK_ID = "firefox-compact-dark@mozilla.org"; > + > + await BrowserTestUtils.withNewTab({gBrowser, url: "https://example.com"}, async function(browser) { > + LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID); This test doesn't belong in toolkit, as the dark theme is defined in browser.
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review226102 Looks like jaws and ntim already got to this.
Attachment #8950011 - Flags: review?(mconley)
I think we should wait for bug 1417883, since atm, the --arrowpanel-dimmed-* and --panel-separator-color variables need some tweaking to be more visible on dark backgrounds. Bug 1417883 will help by adding [popup-brighttext].
Depends on: 1417883
(In reply to Tim Nguyen :ntim from comment #64) > Comment on attachment 8950011 [details] > Bug 1408121: Dark theme should apply dark colors to Firefox menus. > > https://reviewboard.mozilla.org/r/219290/#review226078 > > ::: > toolkit/components/extensions/test/browser/ > browser_ext_themes_dark_theme_arrowpanels.js:27 > (Diff revision 8) > > + const POPUP_TEXT_COLOR = "rgb(249, 249, 250)"; > > + const POPUP_BORDER_COLOR = "#808080"; > > + const COMPACT_DARK_ID = "firefox-compact-dark@mozilla.org"; > > + > > + await BrowserTestUtils.withNewTab({gBrowser, url: "https://example.com"}, async function(browser) { > > + LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID); > > This test doesn't belong in toolkit, as the dark theme is defined in browser. I'm not really sure that we need a test for this at all...
Depends on: 1441866, 1441727, 1441725, 1441833
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review230086 Looks good! I think we can drop the test though - see below. ::: browser/components/extensions/test/browser/browser_ext_themes_dark_theme_arrowpanels.js:3 (Diff revision 11) > +"use strict"; > + > +const {LightweightThemeManager} = ChromeUtils.import("resource://gre/modules/LightweightThemeManager.jsm", {}); I think I concur with dao here - I'm not sure this test is really required. I suspect we already get the important coverage for this from your pre-existing test in toolkit for panel colours, as well as our screenshots tests for the various built-in themes. Plus, I don't see much evidence of us automatically testing dark themes like this in other parts of browser/components/extensions/test/browser... So let's drop this test for now. I think we're covered.
Attachment #8950011 - Flags: review?(mconley) → review-
Depends on: 1442296
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review230832 ::: browser/themes/shared/compacttheme.inc.css:38 (Diff revision 11) > --url-and-searchbar-background-color: rgb(71, 71, 73); > --url-and-searchbar-color: var(--chrome-color); > --urlbar-separator-color: #5F6670; > + > + /* Arrow panels */ > + --arrowpanel-background: var(--chrome-background-color); I just realized it would be better to do this in the addBuiltInTheme calls in nsBrowserGlue.js as opposed to compacttheme.inc.css, as it would allow the dark theme to benefit from the code in LWTConsumer.jsm. https://dxr.mozilla.org/mozilla-central/rev/71edaf2bd1b01daffb805627623712bd329fb5f9/browser/components/nsBrowserGlue.js#704-712 LightweightThemeManager.addBuiltInTheme({ ... popup: "hsl(240, 5%, 5%)", popup_text: "rgb(249, 249, 250)" popup_border: ... });
Attachment #8950011 - Flags: review-
(In reply to Tim Nguyen :ntim from comment #72) > ::: browser/themes/shared/compacttheme.inc.css:38 > (Diff revision 11) > > --url-and-searchbar-background-color: rgb(71, 71, 73); > > --url-and-searchbar-color: var(--chrome-color); > > --urlbar-separator-color: #5F6670; > > + > > + /* Arrow panels */ > > + --arrowpanel-background: var(--chrome-background-color); > > I just realized it would be better to do this in the addBuiltInTheme calls > in nsBrowserGlue.js as opposed to compacttheme.inc.css, as it would allow > the dark theme to benefit from the code in LWTConsumer.jsm. > > https://dxr.mozilla.org/mozilla-central/rev/ > 71edaf2bd1b01daffb805627623712bd329fb5f9/browser/components/nsBrowserGlue. > js#704-712 > > LightweightThemeManager.addBuiltInTheme({ > ... > popup: "hsl(240, 5%, 5%)", > popup_text: "rgb(249, 249, 250)" > popup_border: ... > }); I disagree. nsBrowserGlue isn't really the place to specify theme details.
(In reply to Dão Gottwald [::dao] from comment #73) > I disagree. nsBrowserGlue isn't really the place to specify theme details. It may not seem beneficial at this point, but once bug 1417883 lands, this will allow the dark theme to take advantage of the [popup-brighttext] mechanism instead of duplicating it in compacttheme.inc.css. Another case where putting the properties in nsBrowserGlue is beneficial is bug 1385518, where it could just re-use the mechanism that will be introduced in bug 1418602 (that would be rather complicated to duplicate). I agree nsBrowserGlue isn't too ideal since no other theme properties are in there though, but I don't have a better idea atm.
Flags: needinfo?(dao+bmo)
Depends on: 1443143
Alternatively we could wait until the conversion to a WE theme, and then theme properties would make more sense in a theme manifest.
(In reply to Tim Nguyen :ntim from comment #74) > (In reply to Dão Gottwald [::dao] from comment #73) > > I disagree. nsBrowserGlue isn't really the place to specify theme details. > > It may not seem beneficial at this point, but once bug 1417883 lands, this > will allow the dark theme to take advantage of the [popup-brighttext] > mechanism instead of duplicating it in compacttheme.inc.css. Hmm, yeah, okay.
Flags: needinfo?(dao+bmo)
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. https://reviewboard.mozilla.org/r/219290/#review234128 Looks good, but I'd like at least bug 1443143 to be fixed before landing :)
Attachment #8950011 - Flags: review?(ntim.bugs) → review+
Blocks: 1445939
Comment on attachment 8950011 [details] Bug 1408121: Dark theme should apply dark colors to Firefox menus. I'll land this on inbound now (needs minor rebasing).
Attachment #8950011 - Flags: review+
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c7e0c97f59a Dark theme should apply dark colors to Firefox menus. r=ntim,jaws
https://hg.mozilla.org/mozilla-central/rev/3c7e0c97f59a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Verified it works in latest Nightly but there is no visible hover (the line should have a different color on mouseover). Is this a known issue?
On Windows 10 there is a visible mouseover hover.
On Apple macOS it's missing.
(In reply to Sören Hentzschel from comment #83) > Verified it works in latest Nightly but there is no visible hover (the line > should have a different color on mouseover). Is this a known issue? (In reply to Sören Hentzschel from comment #85) > On Apple macOS it's missing. Can you please file a bug for this and mark it as blocking this bug?
Flags: needinfo?(cadeyrn)
Depends on: 1448473
Filed bug 1448473.
Flags: needinfo?(cadeyrn)
Depends on: 1448867
Depends on: 1449774
Depends on: 1449616
Depends on: 1449933
Release Note Request (optional, but appreciated) [Why is this notable]: For users with Dark Themes, this is a pretty big change in some primary UI. [Affects Firefox for Android]: No. [Suggested wording]: Dark Themes now affect the Firefox menus and notification panels. [Links (documentation, blog post, etc)]: MSU student website: http://www.capstone.cse.msu.edu/2018-01/projects/mozilla/ https://www.reddit.com/r/firefox/comments/86c806/dark_menus_for_the_dark_theme_just_landed_on/
relnote-firefox: --- → ?
Depends on: 1449547
Depends on: 1451944
Depends on: 1451947
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #88) > [Suggested wording]: > > Dark Themes now affect the Firefox menus and notification panels. The note request was merged with the note we added for https://bugzilla.mozilla.org/show_bug.cgi?id=1417883#c68 with this wording: Dark Themes now affect the menus, notification panels, URL bar and search drop-downs. Release Managers will decide on the potential inclusion to release notes for the beta and final versions for end-users so no change on the flag from me. Thanks
Depends on: 1453430
Depends on: 1454231
Depends on: 1454232
Depends on: 1453722
Depends on: 1455950
Depends on: 1456584
Depends on: 1459480
Depends on: 1459352
Depends on: 1451827
Depends on: 1463720
Depends on: 1462635
Depends on: 1464714
Depends on: 1463062
Depends on: 1458274
Depends on: 1466838
Depends on: 1466841
Depends on: 1468453
Depends on: 1470382
I have reproduced this bug with Nightly 58.0a1 (2017-10-12) on Windows 10 , 64 Bit ! This bug's fix is Verified with latest Beta ! Build ID 20180621125625 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 [bugday-20180620]
Depends on: 1470605
Depends on: 1477277
No longer depends on: 1470605
Regressions: 1544015
Regressions: 1547935
See Also: → 1553682
Regressions: 1597631
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: