Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Site Editor: Canvas loader is not easily visible #54202

Closed
t-hamano opened this issue Sep 6, 2023 · 26 comments
Closed

Site Editor: Canvas loader is not easily visible #54202

t-hamano opened this issue Sep 6, 2023 · 26 comments
Assignees
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

t-hamano commented Sep 6, 2023

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
77e4aca3cdc39165b1ca80a43452a9c2.mp4
Block out
44f89370df99ddfa39b4f0153da031d8.mp4
@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Sep 6, 2023
@richtabor
Copy link
Member

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:

CleanShot.2023-09-19.at.10.26.52.mp4

Here's Twenty Twenty Three:

CleanShot.2023-09-19.at.10.27.29.mp4

@richtabor richtabor added the Needs Design Feedback Needs general design feedback. label Sep 19, 2023
@richtabor
Copy link
Member

CleanShot 2023-09-19 at 10 36 46

@jasmussen
Copy link
Contributor

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
Copy link
Member

richtabor commented Sep 19, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

richtabor commented Sep 20, 2023

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).

CleanShot.2023-09-20.at.17.58.49.mp4

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
Copy link
Contributor

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:

  • For the track, the text color at 10% opacity.
  • For the progressbar, the text color at 100% opacity.

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
Copy link
Member

tyxla commented Sep 21, 2023

Regarding the color choice for the progress bar, I've left some feedback at #54611 (review) 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
Copy link
Contributor

ciampo commented Sep 21, 2023

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
Copy link
Member

tyxla commented Sep 21, 2023

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
Copy link
Contributor

ciampo commented Sep 21, 2023

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 --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:

grays:
colord( background ).contrast( gray[ 600 ] ) >= 3 &&
colord( background ).contrast( gray[ 700 ] ) >= 4.5
? undefined
: `The background color provided ("${ background }") cannot generate a set of grayscale foreground colors with sufficient contrast. Try adjusting the color to be lighter or darker.`,

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
Copy link
Member

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
Copy link
Member

tyxla commented Sep 28, 2023

Sounds like a good plan to me too 👍

@talldan talldan added [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Oct 6, 2023
@talldan
Copy link
Contributor

talldan commented Oct 6, 2023

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
Copy link
Member

tyxla commented Oct 6, 2023

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

@andrewserong
Copy link
Contributor

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
Copy link
Member

tyxla commented Oct 9, 2023

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 c6273f4, 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

49d045d54621cd9fba6cad61ee11ebad.mp4

@ciampo
Copy link
Contributor

ciampo commented Oct 11, 2023

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 #55285 with the changes discussed so far.

@tyxla
Copy link
Member

tyxla commented Oct 11, 2023

Thank you for working on it @ciampo ❤️

@richtabor
Copy link
Member

Closed by #55285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants