Closed Bug 1462635 Opened 2 years ago Closed 2 years ago

Make the Recently Bookmarked/Recent History sections titles more visible in Dark theme

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + verified
firefox62 + verified

People

(Reporter: Anca, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image screenshot.png
[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.
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.
Tim, do you have cycles to look at this?
Flags: needinfo?(ntim.bugs)
Anca, please don't file legitimate bugs like this as enhancement requests. Thanks!
Severity: enhancement → normal
Blocks: 1408121
Keywords: regression
Priority: -- → P2
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(ntim.bugs)
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`.
Attachment #8980855 - Flags: review?(ntim.bugs) → review+
(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.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b7db28b360d
Fix disabled text color in webext theme styled popups. r=ntim
Blocks: 1459352
https://hg.mozilla.org/mozilla-central/rev/9b7db28b360d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Depends on: 1464633
Blocks: 1457099
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
Attachment #8980855 - Flags: approval-mozilla-beta?
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.
Attachment #8980855 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1464714
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.