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

VERIFIED FIXED in Firefox 61

Status

()

defect
P2
normal
VERIFIED FIXED
11 months ago
11 months ago

People

(Reporter: Anca, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ verified, firefox62+ verified)

Details

Attachments

(2 attachments)

Posted 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)
(Assignee)

Comment 3

11 months ago
Anca, please don't file legitimate bugs like this as enhancement requests. Thanks!
Severity: enhancement → normal
(Assignee)

Updated

11 months ago
Blocks: 1408121
Keywords: regression
Priority: -- → P2
(Assignee)

Updated

11 months ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request)

Comment 5

11 months 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`.
Attachment #8980855 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 6

11 months 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.
Comment hidden (mozreview-request)

Comment 8

11 months 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
(Assignee)

Updated

11 months ago
Blocks: 1459352

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b7db28b360d
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Comment 10

11 months 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.

Updated

11 months ago
Depends on: 1464633

Updated

11 months ago
Blocks: 1457099

Comment 11

11 months 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
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+

Updated

11 months ago
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.