Invert foreground color when background is light #15230

sangwoo108 · 2022-09-27T03:07:17Z

Resolves brave/brave-browser#25392
Resolves brave/brave-browser#25393

For readability, the foreground color is inverted when the background is light. This is based on WCGA2.0 spec.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Automated

test-unit ColorUtil.test.ts

Manual

Even when using light solid color background, we should be read stats UI easily.

Storybook

Go to "regular"
opt out "sponsored image" on right side control panel.
try changing background from "defaultImage" on the right side control panel.

brave-builds · 2022-09-27T08:09:47Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-09-27T14:25:01Z

A Storybook has been deployed to preview UI for the latest push

For readability, the foreground color is inverted when the background is light. This is based on WCGA2.0 spec.

brave-builds · 2022-09-27T15:46:07Z

A Storybook has been deployed to preview UI for the latest push

fallaciousreasoning

Okay, so this looks pretty good to me but I had an idea as I was reading the last bit of the PR: What if instead of adding all of these JS props we optionally added a

--override-readability-color-rgb: 0, 0, 0;
--override-readability-color: rgb(var(--override-readability-color-rgb));

css variable. Then each component that might be overridden could do this:

styled.whatever`
    // Fall back to white
    color: var(--override-readability-color, #ffffff);
`

styled.other`
    // Fall back to --interactive9
   color: var(--override-readability-color, var(--interactive9));
`

styled.withAlpha`
    // Fallback to white, either with 0.8 opacity
    color: rgba(var(--override-readability-color-rgb, 255, 255, 255), 0.8);
`

Then we wouldn't have to pass these JS variables around all over the place!

You could set the variables in components/brave_new_tab_ui/containers/newTab/index.tsx

useEffect(() => {
	if (currentColorsAreReadable) return;

    const style = document.documentElement.style;
	style.setProperty('--override-readability-color-rgb', '0, 0, 0');
	style.setProperty('--override-readability-color', 'rgb(var(--override-readability-color))');
    return () => {
		style.removeProperty('--override-readability-color-rgb')
		style.removeProperty('--override-readability-color')
    }
}, [background, otherThingsThatChangeTheBackground]);

(you don't have to change anything, it's just an idea I came up with)

components/brave_new_tab_ui/components/default/page/index.tsx

fallaciousreasoning · 2022-09-27T20:55:52Z

components/brave_new_tab_ui/components/default/page/index.tsx

  margin-right: ${p => p.textDirection === 'ltr' && '8px'};
  margin-left: ${p => p.textDirection === 'rtl' && '8px'};
-  border-right: ${p => p.textDirection === 'ltr' && '1px solid rgba(255, 255, 255, 0.6)'};
-  border-left: ${p => p.textDirection === 'rtl' && '1px solid rgba(255, 255, 255, 0.6)'};
+  border-right: ${p => p.textDirection === 'ltr' && `1px solid ${p.color + '99' /* == 0.6 */}`};


It's annoying that you can destructure a color, right? There's actually a proposal for it but I don't think any browsers implement it yet.

https://www.w3.org/TR/css-color-5/#relative-RGB

Oh, yeah. It looks awesome. I guess it's still in WD status, so not started implementing it yet?

components/brave_new_tab_ui/components/default/stats/style.ts

components/brave_new_tab_ui/components/default/widget/assets/ellipsis.ts

components/brave_new_tab_ui/containers/newTab/index.tsx

components/brave_new_tab_ui/helpers/colorUtil.ts

When we use setProperty(), a color value defined in reset.css wins over our css variables. In order to avoid this, use createGlobalStyle and override color value in resets.css when we need more readable color

sangwoo108

Thanks @fallaciousreasoning ! I love the "variable idea" so much. But there was a weird thing I couldn't understand. I made an alternative way to stick to the idea. Could you please take a look again?

components/brave_new_tab_ui/components/default/widget/assets/ellipsis.ts

components/brave_new_tab_ui/helpers/colorUtil.ts

sangwoo108 · 2022-09-28T09:42:09Z

components/brave_new_tab_ui/components/default/page/index.tsx

  margin-right: ${p => p.textDirection === 'ltr' && '8px'};
  margin-left: ${p => p.textDirection === 'rtl' && '8px'};
-  border-right: ${p => p.textDirection === 'ltr' && '1px solid rgba(255, 255, 255, 0.6)'};
-  border-left: ${p => p.textDirection === 'rtl' && '1px solid rgba(255, 255, 255, 0.6)'};
+  border-right: ${p => p.textDirection === 'ltr' && `1px solid ${p.color + '99' /* == 0.6 */}`};


Oh, yeah. It looks awesome. I guess it's still in WD status, so not started implementing it yet?

sangwoo108 · 2022-09-28T09:46:32Z

components/brave_new_tab_ui/components/default/page/index.tsx

+
+      // override color property in resets.css. Not sure why this happens but,
+      // the value in the stylesheet wins over variables above.
+      color: inherit


I couldn't figure out why this happens. This happens when we declare variables with setProperty(). I tried setting property for body, but it didn't work. As a workaround, I override color here.

The previous trial can be found at 27a0d34

I think I like yours better anyway 😄 It's weird we need to set the color: inherit though - I just tested removing it and it seems to work without

css` --override-readability-color-rgb: 0, 0, 0; --override-readability-color: rgb(0, 0, 0); `

Ah yeah, I found the culprit. I used wrong variable for some UI.

- color: var(--override-readability-color-rgb, #FFFFFF); <--! oops, rgb is 0, 0, 0 --> + color: var(--override-readability-color, #FFFFFF);

brave-builds · 2022-09-28T12:07:44Z

A Storybook has been deployed to preview UI for the latest push

fallaciousreasoning

Still LGTM @sangwoo108 - I like your solution with createGlobalStyle more than my useEffect anyway - it's much more elegant.

components/brave_new_tab_ui/components/default/page/index.tsx

components/brave_new_tab_ui/components/default/widget/widgetMenu.tsx

fallaciousreasoning · 2022-09-28T21:00:56Z

components/brave_new_tab_ui/components/default/page/index.tsx

+
+      // override color property in resets.css. Not sure why this happens but,
+      // the value in the stylesheet wins over variables above.
+      color: inherit


I think I like yours better anyway 😄 It's weird we need to set the color: inherit though - I just tested removing it and it seems to work without

css` --override-readability-color-rgb: 0, 0, 0; --override-readability-color: rgb(0, 0, 0); `

components/brave_new_tab_ui/components/default/page/index.tsx

brave-builds · 2022-09-29T08:35:55Z

A Storybook has been deployed to preview UI for the latest push

sangwoo108 · 2022-09-29T10:06:08Z

Thank you so much @fallaciousreasoning ! I learned a lot from you as always!

github-actions bot assigned sangwoo108 Sep 27, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Sep 27, 2022

sangwoo108 force-pushed the sko/color branch 5 times, most recently from 746f984 to 4759542 Compare September 27, 2022 07:30

sangwoo108 force-pushed the sko/color branch from 4759542 to 0fe6982 Compare September 27, 2022 13:47

Invert foreground color when background is light

fa9393c

For readability, the foreground color is inverted when the background is light. This is based on WCGA2.0 spec.

sangwoo108 force-pushed the sko/color branch from 0fe6982 to fa9393c Compare September 27, 2022 15:06

sangwoo108 requested review from fallaciousreasoning, petemill and simonhong September 27, 2022 15:07

fallaciousreasoning approved these changes Sep 27, 2022

View reviewed changes

sangwoo108 added 3 commits September 28, 2022 18:30

Address code review: remove prop drilling

27a0d34

Use styled-component's createGlobalStyle to avoid a bug

473a4bc

When we use setProperty(), a color value defined in reset.css wins over our css variables. In order to avoid this, use createGlobalStyle and override color value in resets.css when we need more readable color

Override text color in GridSites

0dca146

sangwoo108 force-pushed the sko/color branch from 9cc10ce to 341c5ff Compare September 28, 2022 09:57

sangwoo108 commented Sep 28, 2022

View reviewed changes

Clean up

46371b6

sangwoo108 force-pushed the sko/color branch from 341c5ff to 46371b6 Compare September 28, 2022 11:18

fallaciousreasoning approved these changes Sep 28, 2022

View reviewed changes

More clean up and fix wrong variable usage

3236609

sangwoo108 merged commit c6023bb into master Sep 29, 2022

sangwoo108 deleted the sko/color branch September 29, 2022 10:05

github-actions bot added this to the 1.46.x - Nightly milestone Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invert foreground color when background is light #15230

Invert foreground color when background is light #15230

sangwoo108 commented Sep 27, 2022

brave-builds commented Sep 27, 2022

brave-builds commented Sep 27, 2022

brave-builds commented Sep 27, 2022

fallaciousreasoning left a comment •

edited

Loading

fallaciousreasoning Sep 27, 2022

sangwoo108 Sep 28, 2022

sangwoo108 left a comment

sangwoo108 Sep 28, 2022

sangwoo108 Sep 28, 2022

fallaciousreasoning Sep 28, 2022

sangwoo108 Sep 29, 2022

brave-builds commented Sep 28, 2022

fallaciousreasoning left a comment

fallaciousreasoning Sep 28, 2022

brave-builds commented Sep 29, 2022

sangwoo108 commented Sep 29, 2022

Invert foreground color when background is light #15230

Invert foreground color when background is light #15230

Conversation

sangwoo108 commented Sep 27, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Automated

Manual

Storybook

brave-builds commented Sep 27, 2022

brave-builds commented Sep 27, 2022

brave-builds commented Sep 27, 2022

fallaciousreasoning left a comment • edited Loading

Choose a reason for hiding this comment

fallaciousreasoning Sep 27, 2022

Choose a reason for hiding this comment

sangwoo108 Sep 28, 2022

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

sangwoo108 Sep 28, 2022

Choose a reason for hiding this comment

sangwoo108 Sep 28, 2022

Choose a reason for hiding this comment

fallaciousreasoning Sep 28, 2022

Choose a reason for hiding this comment

sangwoo108 Sep 29, 2022

Choose a reason for hiding this comment

brave-builds commented Sep 28, 2022

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning Sep 28, 2022

Choose a reason for hiding this comment

brave-builds commented Sep 29, 2022

sangwoo108 commented Sep 29, 2022

fallaciousreasoning left a comment •

edited

Loading