Anca Soncutean, Desktop QA Reporter
	Description • 6 years ago

[Affected versions]: 
- 61.0b6 (20180517220057)
- 62.0a1 (2018-05-17)

[Affected platforms]:
- Ubuntu 16.04 x64
- macOS 10.13
- Windows 7 x64
- Windows 10 x64

[Steps to reproduce]:
1. Launch Firefox
2. Enable Dark theme from Menu - Add-ons - Themes
3. Go to Library - Bookmark/History 

[Expected result]:
- Recently Bookmarked/Recent History sections titles should be more more visible.

[Actual result]:
- Recently Bookmarked/Recent History sections titles  are not visible enough. 

[Additional Notes]:
- This issue seems to look worse on Windows platforms.

	Ryan VanderMeulen [:RyanVM]
	Comment 1 • 6 years ago

Agreed that this is pretty painful as a dark theme user. Given that we're touting dark theme improvements as one our Fx61 release notes, this would be a good polish bug to fix.

	Ryan VanderMeulen [:RyanVM]
	Comment 2 • 6 years ago

Tim, do you have cycles to look at this?

	Dão Gottwald [:dao] Assignee
	Comment 3 • 6 years ago

Anca, please don't file legitimate bugs like this as enhancement requests. Thanks!

	Tim Nguyen :ntim
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8980855 [details]
Bug 1462635 - Fix disabled text color in webext theme styled popups.

https://reviewboard.mozilla.org/r/247050/#review253120

Looks mostly fine to me, thanks for looking into this!

::: toolkit/modules/LightweightThemeConsumer.jsm:43
(Diff revision 1)
> +      if (!rgbaChannels) {
> +        element.removeAttribute("lwt-popup-brighttext");
> +        element.removeAttribute("lwt-popup-darktext");
> +        element.style.removeProperty(disabledColorVariable);
> +        return null;
> +      }
> +
> +      let {r, g, b, a} = rgbaChannels;
> +
> +      if (_isTextColorDark(r, g, b)) {
> +        element.removeAttribute("lwt-popup-brighttext");
> +        element.setAttribute("lwt-popup-darktext", "true");
> +      } else {
> +        element.removeAttribute("lwt-popup-darktext");
> +        element.setAttribute("lwt-popup-brighttext", "true");
> +      }

I don't have a major preference, but we can save 2 lines here by removing the 2 attributes unconditionally at the beginning of the function, rather than in each individual case.

::: toolkit/modules/LightweightThemeConsumer.jsm:205
(Diff revision 1)
>        root.removeAttribute("lwthemetextcolor");
> +      root.removeAttribute("lwt-popup-brighttext");
> +      root.removeAttribute("lwt-popup-darktext");

This is probably not needed, we call processColor regardless of whether a theme is active or not: https://searchfox.org/mozilla-central/source/toolkit/modules/LightweightThemeConsumer.jsm#228

So the `if (!rgbaChannels)` should be enough to catch it.

In the lwthemetextcolor case, we want to add the attribute regardless of the validity of the text color, which is why it's being removed here, rather than in `processColor`.

	Dão Gottwald [:dao] Assignee
	Comment 6 • 6 years ago

(In reply to Tim Nguyen :ntim from comment #5)
> ::: toolkit/modules/LightweightThemeConsumer.jsm:43
> (Diff revision 1)
> > +      if (!rgbaChannels) {
> > +        element.removeAttribute("lwt-popup-brighttext");
> > +        element.removeAttribute("lwt-popup-darktext");
> > +        element.style.removeProperty(disabledColorVariable);
> > +        return null;
> > +      }
> > +
> > +      let {r, g, b, a} = rgbaChannels;
> > +
> > +      if (_isTextColorDark(r, g, b)) {
> > +        element.removeAttribute("lwt-popup-brighttext");
> > +        element.setAttribute("lwt-popup-darktext", "true");
> > +      } else {
> > +        element.removeAttribute("lwt-popup-darktext");
> > +        element.setAttribute("lwt-popup-brighttext", "true");
> > +      }
> 
> I don't have a major preference, but we can save 2 lines here by removing
> the 2 attributes unconditionally at the beginning of the function, rather
> than in each individual case.

I'd rather leave this as-is, such that we don't needlessly remove an attribute before setting it again.

	Pulsebot
	Comment 8 • 6 years ago

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b7db28b360d
Fix disabled text color in webext theme styled popups. r=ntim

	Andrei Ciure[:aciure]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9b7db28b360d

	Tim Nguyen :ntim
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8980855 [details]
Bug 1462635 - Fix disabled text color in webext theme styled popups.

https://reviewboard.mozilla.org/r/247050/#review253166

::: toolkit/components/extensions/test/browser/browser_ext_themes_autocomplete_popup.js:237
(Diff revision 2)
>    // Check to see if popup-brighttext and secondary color are not set after
>    // unload of theme
>    Assert.equal(root.getAttribute("lwt-popup-brighttext"),
>                 "",
>                 "brighttext should not be set!");
> +  Assert.equal(root.getAttribute("lwt-popup-dakrtext"),

s/dakrtext/darktext 

Gah, I should have caught this during review.

	Tim Nguyen :ntim
	Comment 11 • 6 years ago

Comment on attachment 8980855 [details]
Bug 1462635 - Fix disabled text color in webext theme styled popups.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1408121
[User impact if declined]: bad visibility of panel text in dark themes
[Is this code covered by automated tests?]: somewhat, not completely
[Has the fix been verified in Nightly?]: 
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: bug 1464633
[Is the change risky?]: low
[Why is the change risky/not risky?]: mostly css changes
[String changes made/needed]: n/a

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago

Comment on attachment 8980855 [details]
Bug 1462635 - Fix disabled text color in webext theme styled popups.

Approved for 61.0b9. Please be sure to uplift the test-only fix in bug 1464633 at the same time.

	Tim Nguyen :ntim
	Comment 13 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/042445c33dc3549e23ab24260b41557c17045bf3

	Anca Soncutean, Desktop QA Reporter
	Comment 14 • 6 years ago

This visibility issue is fixed on Firefox 62.0a1 (2018-05-28) and Firefox 61.0b9 (20180528091514) under  Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13.