WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.47k stars 4.18k forks source link

Site Editor: Canvas loader is not easily visible #54202

Closed t-hamano closed 1 year ago

t-hamano commented 1 year ago

What problem does this address?

While the site editor is loading, the Progressbar component is used to indicate loading status. However, depending on the color variation of the theme, the track and indicator can be very difficult to see, and sometimes the following warning message appears in the browser console:

index.js:38 wp.components.Theme: The background color provided ("#ff5252") cannot generate a set of grayscale foreground colors with sufficient contrast. Try adjusting the color to be lighter or darker.

What is your proposed solution?

For example, if the track and indicator colors calculated from the global settings of the theme are below a certain contrast value, it would be nice to have a process that increases the contrast ratio.

Screencast

The following screencast is a recording of a loading that outputs a browser warning in TwentyTwentyThree.

Default https://github.com/WordPress/gutenberg/assets/54422211/7da5c8da-9a53-44a6-951c-3b28bd472fa0
Block out https://github.com/WordPress/gutenberg/assets/54422211/2a2e5995-498e-429a-8876-fec9db591324
richtabor commented 1 year ago

I'm confused why editor UI is using colors from the theme. I too find this does not work and had expected the color to be the actual admin accent color. I don't think it's worth the work to catch/resolve/adjust when the color's don't work well. @WordPress/gutenberg-design

Here's Twenty Twenty Four:

https://github.com/WordPress/gutenberg/assets/1813435/68a2e35f-243c-4ecc-9578-82b8235efd9a

Here's Twenty Twenty Three:

https://github.com/WordPress/gutenberg/assets/1813435/bc04ad5b-4822-4a6a-b9e7-aa5b549b69a6

richtabor commented 1 year ago
CleanShot 2023-09-19 at 10 36 46
jasmussen commented 1 year ago

I can't find the discussion now, I thought it appeared in #53032 but that doesn't seem the case. But the short of it is, I had the same feedback for @tyxla when we initially leveraged this progressbar and applied it to the site editor, and even for the same arguments (i.e. it's UI, it's not theme UI).

However ultimately we ended up letting the theme colors affect the progressbar after all, because the blueberry and light gray colors as applied to the progressbar ended up not working in a variety of adjacent colors, where as the use of theme colors for text and background would always work.

To that end, all the videos shared in this seem to show the wrong use case for the progressbar. I.e. the site editor is meant to actually show the background color as it loads, like so:

Screenshot 2023-09-19 at 17 14 10

The console error is likely the source of the error, but to be clear, it seems like the bug here is that the background color isn't applieda s it should be.

richtabor commented 1 year ago

The console error is likely the source of the error, but to be clear, it seems like the bug here is that the background color isn't applieda s it should be.

I'm not so confident that we can expect the colors to always work, even with background color applied. Even TT3 and TT4 from above wouldn't be a different result with the background color.

jameskoster commented 1 year ago

because the blueberry and light gray colors as applied to the progressbar ended up not working in a variety of adjacent colors

Perhaps the frame could be 'empty', or filled with a UI color until the site loads?

Screenshot 2023-09-20 at 11 29 52

I'd +1 that the progress bar should be UI colored. Relying on theme colors seems a bit challenging, and there's a chance the user could mistakenly interpret the progress bar to be a part of their site, IE something they'd see on the frontend too.

t-hamano commented 1 year ago

I know this is not good from a design perspective, but I'll post one idea just in case 😅 The approach is to keep the theme's background color and display a progress bar inside a small modal.

small-dialog

jasmussen commented 1 year ago

@tyxla is AFK today, but I'd really love his feedback. These exact topics came up when he initially built the progressbar, and I recall @mtias quite strongly advocating for using the theme colors as they are now. Did we find out why the style variation background isn't applied when loading, and the console error? IMO that seems like the first issue to solve.

paaljoachim commented 1 year ago

A sidetrack comment coming up. Can we also link in issues/PR's that are working on speeding up making the Site Editor load more efficient/quicker? As it is nice to link these issues/PR's together.

richtabor commented 1 year ago

Did we find out why the style variation background isn't applied when loading, and the console error?

It's actually working fine. Here's TT4 theme with the "Onyx" style variation applied. Those others are just the background color of the themes (which is #FFF for TT3 and #F7F7F7 for TT4).

https://github.com/WordPress/gutenberg/assets/1813435/69fa6815-8406-4adc-8501-ff0de0a36c34

It could work if we only used the text color, with the track at 50% opacity or so and the fill at 100%. We know the text and background colors contrast appropriately — but not any other colors.

jasmussen commented 1 year ago

Thank you for the clarification. In my opinion, so long as it is possible in a style variation to read the text on the background (i.e. in this example, white on dark gray), then it should be possible apply the same colors to the progressbar:

Those colors may not be what the progressbar uses at the moment, but if it did surely that would work in more cases than would a blue on gray progressbar?

tyxla commented 1 year ago

Regarding the color choice for the progress bar, I've left some feedback at https://github.com/WordPress/gutenberg/pull/54611#pullrequestreview-1637742579 and I'd like to get feedback from @mtias who originally brought up much of the idea for using a progress bar there.

Regarding the issue with Theme and being unable to generate shades of gray, it is unfortunate. I'd like to consult with @ciampo on the best approach to handle fallback colors with the Theme component. Furthermore, I know work is being done to significantly advance Theme, I wonder if that could also help (cc @SaxonF).

ciampo commented 1 year ago

Regarding the issue with Theme and being unable to generate shades of gray, it is unfortunate.

Theme as of now does generate shades of gray — you can test it in this interactive Storybook example

I guess I'm a bit confused as to what is the actual issue, and what is actually the spec that we should follow.

tyxla commented 1 year ago

Theme as of now does generate shades of gray — you can test it in this interactive

Yes, but, I guess there are colors for which it declines to do it due to contrast concerns - see the original issue above for info.

ciampo commented 1 year ago

Ok, I would split the issue in two parts

Making sure that the progress bar always looks good

If we can't be sure that the background color will be the same as the theme's background color, then @jasmussen 's suggestion should work. since the text color is computed to ensure enough contrast:

  • For the track, the text color at 10% opacity.
  • For the progressbar, the text color at 100% opacity.
we could therefore doing something like this ```diff diff --git a/packages/components/src/progress-bar/styles.ts b/packages/components/src/progress-bar/styles.ts index e983797d3d..00e94b5dcc 100644 --- a/packages/components/src/progress-bar/styles.ts +++ b/packages/components/src/progress-bar/styles.ts @@ -27,9 +27,10 @@ export const Track = styled.div` width: 100%; max-width: 160px; height: ${ CONFIG.borderWidthFocus }; - background-color: var( - --wp-components-color-gray-300, - ${ COLORS.gray[ 300 ] } + background-color: color-mix( + in srgb, + var( --wp-components-color-foreground, ${ COLORS.gray[ 900 ] } ), + transparent 90% ); border-radius: ${ CONFIG.radiusBlockUi }; `; @@ -43,7 +44,11 @@ export const Indicator = styled.div< { top: 0; height: 100%; border-radius: ${ CONFIG.radiusBlockUi }; - background-color: ${ COLORS.theme.accent }; + background-color: color-mix( + in srgb, + var( --wp-components-color-foreground, ${ COLORS.gray[ 900 ] } ), + transparent 10% + ); ${ ( { isIndeterminate, value } ) => isIndeterminate ```

Improving the algorithm used in Theme to generate shades of gray

This is, IMO, a separate issue from what we're discussing here.

Looking at the code, the warning that is quoted in the issues's description is printed when the "gray 600" and "gray 700" shades don't have enough contrast with the background color:

https://github.com/WordPress/gutenberg/blob/041860abbcc21b1749f4837a2c0637dd89436ce4/packages/components/src/theme/color-algorithms.ts#L62-L66

Lena (the person who coded the algorithm) is unfortunately not around, but I guess the checks are there because these colors are usually used for some text, and therefore we need to make sure that there is enough contrast with the background

richtabor commented 1 year ago

If we can be ensured that the background color will be the same as the theme's background color, then @jasmussen 's suggestion should work. since the text color is computed to ensure enough contrast:

Yes, let's give this a go. I can see this working as text and background colors can be counted on to contrast appropriately.

tyxla commented 1 year ago

Sounds like a good plan to me too 👍

talldan commented 1 year ago

I'm changing this to a bug, as the poor contrast is almost certainly an accessibility issue.

I also think it needs to be solved for 6.4, as it's a bad first experience for users—it's quite hard to see the loading bar at all (even with my 20/20 vision) and so it feels like a crash at first.

tyxla commented 1 year ago

@talldan I'm happy to try to devote some time to it next week.

andrewserong commented 1 year ago

Apologies if this has already been discussed, but would it be worth also looking at the height / thickness of the progress bar, as increasing that could potentially help with the visibility in addition to the colors used?

tyxla commented 1 year ago

Apologies if this has already been discussed, but would it be worth also looking at the height / thickness of the progress bar, as increasing that could potentially help with the visibility in addition to the colors used?

I initially went with 5px when prototyping the component in #53030, but @jameskoster changed that to a thinner value in c6273f4d43a2856b5245e305134393d7fac67941, which I think was in accord with what @jasmussen and @pablohoneyhoney also had in mind previously. I personally find this a bit too thin, but I rarely have strong feelings about design, because it's not one of my strong sides.

andrewserong commented 1 year ago

I rarely have strong feelings about design, because it's not one of my strong sides

Thanks for the extra context, I am much the same! 🙂

jasmussen commented 1 year ago

So long as the contrast is high, the thickness IMO is not a big issue. I would think that it would have to be very thick to fall into the "large type" general area which would permit a lower contrast, so I don't think that would really benefit.

t-hamano commented 1 year ago

I also noticed that the progress bar does not appear at all in Windows high contrast mode. Therefore, I cannot see what is happening when the Site Editor is loading.

https://github.com/WordPress/gutenberg/assets/54422211/92f5f1a6-3f69-4446-a821-1661bc5053ed

ciampo commented 1 year ago

Since there seems to be an agreement on the next step but no one has taken ownership of the task yet, I went ahead and opened https://github.com/WordPress/gutenberg/pull/55285 with the changes discussed so far.

tyxla commented 1 year ago

Thank you for working on it @ciampo ❤️

richtabor commented 1 year ago

Closed by https://github.com/WordPress/gutenberg/pull/55285