WordPress / gutenberg

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

Local block appearance controls should reflect values from global styles / theme.json #37752

Open jameskoster opened 2 years ago

jameskoster commented 2 years ago

Current implementation

My theme.json declares a text color for h2s like so:

"color": {
    "text": "var(--wp--preset--color--grey-light)"
}

When I select a h2 in the Editor and open the color panel, I see this:

Screenshot 2022-01-06 at 14 57 58

The canvas is telling me one thing, but the control is telling my something else. I can clearly see that a color is set, but the 🚫 icon in the control makes it look like no color is set. When I open the color picker, the transparent texture confuses things further by suggesting that the color is set to transparent. This combines to erode my confidence on what will actually appear on the frontend.

Expectation

My expectation would be to see something like this, where the control reflects the code exactly:

Screenshot 2022-01-06 at 15 00 48

Suggestion

We show all values that are inherited from theme.json / Global Styles. Here's an example showing the Button block as styled globally, showing its values — colors, font size, padding, radius — in the inspector even locally.

Inheritance option 8

This would be a first stap that doesn't necessarily preclude further steps taken in #49278 to visualize where the inheritance is coming from, but it might also reduce the issue enough that it'd be less urgent, perhaps even irrelevant. It would also make clearer when the contrast notice appears or doesn't; at the moment the values compared for contrast are not aware of inheritance, but with this change they could be made to.

Issue updated Aug 1, 2024.

Mamaduka commented 2 years ago

I can't remember the issue/comment right now, but I believe the main problem is getting values from CSS variables.

Do you get the same results if you provide color value as HEX?

jameskoster commented 2 years ago

Do you get the same results if you provide color value as HEX?

Yep, it's exactly the same.

jameskoster commented 2 years ago

I noticed a similar thing happens with the border radius control. If my theme sets a radius on the Image block, that is applied on the canvas and the frontend as expected, but the control appears empty:

Screenshot 2022-01-06 at 17 21 36
karmatosed commented 2 years ago

The canvas is telling me one thing, but the control is telling my something else.

I second this expectation. If the canvas says something the control to me should reflect this. I have a feeling the sidebar hasn't caught this for a while as I seem to remember this disconnect being there for a while. However, I think it should get resolved as it feels less easy the further into site editing things go.

I put on needs dev as to me the solution is fairly agreed on of showing the setting, but labels can always be adjusted from there.

carolinan commented 2 years ago

Isn't this true for all theme.json settings? That none of them are reflected in the corresponding control?

aaronrobertshaw commented 2 years ago

Isn't this true for all theme.json settings? That none of them are reflected in the corresponding control?

I believe it is not just theme.json but also Global Styles.

I'm not up-to-date on the inner workings at the moment but it was the case that the block editor doesn't get the merged theme.json/global styles in an accessible object within any data stores. That meant, short of inspecting every block and determining computed styles there was no way to determine what value a control should default to.

If I recall correctly, the merged global styles were available in an __experimentalStyles property on the store's settings object. This might now be limited only to the mobile context. There was an open PR exploring making these styles available within the block editor. I'm not sure if the approach there is still valid or if there is now a new method for accessing the merged global styles. Perhaps @jorgefilipecosta might have more insight here?

Once we can access those style values, we could close the gap here between what we see in the editor canvas and the block's sidebar controls.

Edit: It appears that global styles are now provided via context within the site editor. We might still need to apply a similar approach for the block editor.

jameskoster commented 2 years ago

I think there's potentially another element to this that I overlooked – when the color is inherited from a stylesheet (not necessarily theme.json). The control should account for those scenarios as well.

jorgefilipecosta commented 2 years ago

I think there's potentially another element to this that I overlooked – when the color is inherited from a stylesheet (not necessarily theme.json). The control should account for those scenarios as well.

On https://github.com/WordPress/gutenberg/pull/34178, I try to implement the case where we should the colors when they come from theme.json.

when the color is inherited from a stylesheet (not necessarily theme.json). The control should account for those scenarios as well.

This would be complex. The stylesheet changing the color may not even be loaded on the editor. The markup on the editor may be a little different, making the colors different. But of course, we could try to should the current color rendered on the editor.

jameskoster commented 2 years ago

On https://github.com/WordPress/gutenberg/pull/34178, I try to implement the case where we should the colors when they come from theme.json.

Thanks for the effort there. I would love to see the PR merged, but I understand it may be a gnarly one.

This would be complex.

For sure. We need further design exploration on how to handle raw styles. We can do this in https://github.com/WordPress/gutenberg/issues/43082.

jameskoster commented 1 year ago

I'm going to close this as it is essentially captured by https://github.com/WordPress/gutenberg/issues/49278 which is more comprehensive.

jameskoster commented 7 months ago

Re-opening since https://github.com/WordPress/gutenberg/issues/49278 is a large issue, and this part is particularly important.

jasmussen commented 7 months ago

I've lightly refreshed this issue after it was reopened above. @jorgefilipecosta any chance #34178 could be revisited?

jasmussen commented 6 months ago

@annezazu let me know your thoughts on this latest, and if there's some additional triage we can do.

annezazu commented 6 months ago

Thanks so much for updating this with fresh designs. I want to include only one related issue for 6.6. Right now, we have three possibilities for what to include. The title of this better matches what feels most likely for 6.6 so I'd love to see this defined more as an iteration issue with Design tools: Better handling of unset / inherited values as a sub issue of Styles: show a hierarchy of styles clearly (theme, user, global unspecific, global specific, local, etc) as the tracking issue. I see this all laddering up within the overall surfacing of the style hierarchy efforts that are far more complex.

How does that sound? I can quickly make some changes if so.

jasmussen commented 6 months ago

At a quick evening glance, that sounds about right. This is one small step (just showing inherited values right in the controls themselves) on the path to bigger and better things (showing where those values are inherited from, or what they override).

fabiankaegy commented 6 months ago

Question for that interim step:

Should it only show inherited options from theme.json / global styles, or should it also show any inherited values from parent blocks etc.?

I would suggest that it should not show the inherited values for inherited values from parent blocks because oftentimes, you want to control the text color of all elements at the group level compared to the individual blocks within the heading. But if the individual blocks within show the color selected in their color picker I think that leads to confusion since users can no longer determine whether the color is set on the individual paragraph or the group.

jasmussen commented 6 months ago

Should it only show inherited options from theme.json / global styles, or should it also show any inherited values from parent blocks etc.?

Ideally either of those. Show the computed end result, wherever that comes from. Except of course if it's coming from custom CSS, we can't know that.

But if the individual blocks within show the color selected in their color picker I think that leads to confusion since users can no longer determine whether the color is set on the individual paragraph or the group.

We know we want to follow up with with a UI for showing where values are inherited from, or when a local value overrides an inherited value.

There's confusion today, no matter how you look at it: even when a group clearly has text and background colors (from global styles perhaps), the inspector shows no colors being set: that's confusing. And because we don't show those computed valued, it'll be even more confusing when you see a contrast warning, with zero context on why: white text on a white background on a black background

Competing editors in this space consistently show the computed value for controls, even when it's inherited. In most cases, those editors don't even show where values are inherited from. In those editors, it feels like a complete non-issue, and it feels less confusing overall. I see black text, and here's the color control showing black text. I see a 13px font size, and it shown. Here's Google Docs:

screenshot of Google Docs toolbar

11px black Arial font. Except those are inherited from the "Normal" text style.

We can and should do better than other editors in the space. But doing so in two steps instead of keeping the status quo until future releases, feels pragmatic, especially since the harm in doing so seems mostly theoretical, whereas the problem it solves is clear.

richtabor commented 6 months ago

This is one thing I've seen folks consistently hang up on. Not knowing what size, color—style in general—is currently applied to a block. It would add much more visual context to the UI, like if you could see the background color of a button then you know where to change it.

Thinking a bit further ahead, but even with the section styles effort, it's not particularly ideal to have a style applied and not see the controls that are applying those values reflect those. You have to deduce the correct control to manipulate further, rather than seeing it presented with the applied change.

Current Proposed
CleanShot 2024-05-01 at 07 17 41 CleanShot 2024-05-01 at 07 15 22
jameskoster commented 6 months ago

But if the individual blocks within show the color selected in their color picker I think that leads to confusion since users can no longer determine whether the color is set on the individual paragraph or the group.

Isn't this basically a question of showing style inheritance (https://github.com/WordPress/gutenberg/issues/49278)?

I still think that's important, but I don't know that it should hold up closing this issue, which causes a lot of confusion by itself.

jarekmorawski commented 6 months ago

Agreed. I've struggled with this and saw others click around to figure out why their styles don't work correctly (even if they do). I don't think we need an extra interface element to let the user know the values are inherited: the controls themselves are clear enough. It'd be handy, however, to make it easy for the user to reset to inherited values when they did manual overrides.

Reset

In such case, we'd also need to provide a way to clear all customizations and sever the connection with the pattern. For instance, if my paragraph block inherits the background color from the parent block and I change it, I may want to reset it to use the parent's background color again or clear all styles and start fresh.

Clear

Isn't this basically a question of showing style inheritance (https://github.com/WordPress/gutenberg/issues/49278)?

It might be, but I think the goal is to find a good stopgap solution before the UI for displaying and managing inherited styles is ready.

richtabor commented 6 months ago

If we did this, "Reset" would only render for each that has a value that is different from the inherited value, indicating the value is not inherited (which is how it works today in essence).

I'm not sure we need "Clear all" either (or at least that's a separate issue independent of inheritance).

annezazu commented 6 months ago

A longstanding piece of feedback has been around the confusion that adding a background color adds padding. When adding background color, could we also show that padding is added? Not sure what's technically possible here but it feels like the work happening here could positive impact a longstanding point of feedback.

carolinan commented 6 months ago

I think it is important that the classic themes are considered too. One of the difficulties with making a classic theme compatible with new features is that they are not included in the planning from the start. And I mean that this includes an early decision about what should work in a classic theme with and without theme.json.

jasmussen commented 6 months ago

And I mean that this includes an early decision about what should work in a classic theme with and without theme.json.

What are technical options here?

carolinan commented 6 months ago

For example, a feature can be limited to block themes only, or to any theme that has a theme.json, there are already checks for both.

jasmussen commented 6 months ago

I mainly mean in context of this particular effort, showing inherited color values in the color controls. Is there a technical way to compute those values if they are provided by CSS instead of theme.json?

carolinan commented 6 months ago

Not that is worth pursuing, I think.

annezazu commented 6 months ago

Giving an update as we have had some dev work done but need more! A recent PR was merged to add global styles to settings using existing context code. As Aaron notes on that PR, this PR will give the post editor access to the merged global styles values. With that, the inspector controls could be updated to reflect those values as "defaults". The latter half is what we need dev work on so, if folks can help out, please do.

jorgefilipecosta commented 6 months ago

I've lightly refreshed this issue after it was reopened above. @jorgefilipecosta any chance #34178 could be revisited?

Yes we should revisit https://github.com/WordPress/gutenberg/pull/34178 and the related PR's. It seems @Mamaduka assigned this issue so I guess soon we will have some progress here, happy to help with reviews/insights.

Mamaduka commented 6 months ago

@jorgefilipecosta, what other PRs are related to #34178? I'm going through related issues/discussions to create a mental map for the feature and how the editor should handle the inherited defaults.

By the way, #34178 only seems to get values defined for the block type. Is this what we want to achieve as an interim solution, or was it just PoC?

jorgefilipecosta commented 5 months ago

Hi @Mamaduka, I guess the related PR's are https://github.com/WordPress/gutenberg/pull/46894 where some of the code in https://github.com/WordPress/gutenberg/pull/34178 was included at https://github.com/WordPress/gutenberg/commit/5ec1a9211cc3e6a42e0fc85009c6f7f782e1853b and also https://github.com/WordPress/gutenberg/pull/47098.

By the way, https://github.com/WordPress/gutenberg/pull/34178 only seems to get values defined for the block type. Is this what we want to achieve as an interim solution, or was it just PoC?

It was PoC to start with the value for the block, ideally, it should support any style defined in a theme.json but it will be complex as it involves specificity rules and inheritance.

carolinan commented 5 months ago

Is there a status update for this?

annezazu commented 5 months ago

This didn't land in time. Going to remove from the 6.6 board.

annezazu commented 4 months ago

In digging into https://github.com/WordPress/gutenberg/issues/57537 for 6.6, it's clear this problem presents itself anew with this new option. Namely, you apply a style for a section, including inner blocks, but the inherited styling isn't reflected for the inner blocks. When we fix this, let's ensure it plays nicely with section styling too as it would greatly improve the UX of that feature.

Mamaduka commented 4 months ago

I removed my assignment for this issue.

I realized I'm less familiar with this codebase than I thought. While I usually don't mind digging into different parts of the project, I've some other items that I want to ship in 6.7.

jasmussen commented 4 months ago

Thanks for looking! Thinking more about this I also wanted to try some other options for showing visualization, ones that combine the unset appearance while still surfacing the inherited values. This is a bit rough, but it furthers the idea:

Inheritance option 1

Here's another option for the color swatch:

Inheritance option 2

In the above, I think the stacked version of the color swatch works better, though it might confuse in other cases where that same stack is used not for inheritance. The italics inherited input values works quite well, IMO, it might not even ned to be italic, as it's been tested and "proven" by the Image block, which arguably has the first instance of this:

image block with auto values

There are more options to explore here, but given we're in 6.7 territory, might be worth kicking off again. @WordPress/gutenberg-design

Figma.

jameskoster commented 4 months ago

I think these all work quite well in isolation, though I'd avoid the stacked swatches because there are situations where multiple colors will appear stacked, and we need to communicate inheritance. Another option could be a slightly different stacked appearance:

swatches

By having per-control implementations I do worry that we're adding a lot of complexity to the interface (and the underlying components). It's a lot to learn, and adds a lot of noise... just looking at the mockup above makes me a bit 😵‍💫.

To stress-test, ideally we mockup all control types. Off the top of my head the following are missing:


Have we totally exhausted the idea of showing the inherited value in the control, and doing something with the label to indicate inheritance? One strong argument for that approach (imo) is that it's a single pattern for users to learn – we can apply the same treatment across all controls. It's also much simpler to implement.

jasmussen commented 4 months ago

I quite like that little dot.

jasmussen commented 4 months ago

Have we totally exhausted the idea of showing the inherited value in the control, and doing something with the label to indicate inheritance?

Not necessarily! I think if the right design presents itself, there are still options. Relatedly, I also wanted to bring up this interface as a place to show inheritance:

Screenshot 2024-07-03 at 10 51 33

"No color selected", that big white spot could potentially hold some inheritance information. As we start to employ itemgroups in more places, that whole flyout might present even more options for showing more advanced inheritance aspects: it's a good way to manage prominence for all the tools that may be necessary for pushing in various directions.

I also realized we're already showing inheritance in global styles, for the line-height here:

Screenshot 2024-07-03 at 10 53 45

It's working so well that no-one noticed it. That feels validating to showing inherited input values as gray text. Across that, the dot for colors, the broader space available in the flyout which should afford comlexity, the remaining piece is the sliders, which are tricky. Any instincts there?

jameskoster commented 4 months ago

That feels validating to showing inherited input values as gray text

The only downside to this is that the gray text could be mistaken for a placeholder. I agree that doing something with the text in these inputs works well though.

Italicising is another option. Or if we need something stronger:

inherited

We probably also need to communicate the inherited unit 😅

Edit: I just noticed Github has a similar treatment:

Screenshot 2024-07-03 at 10 13 59
jasmussen commented 4 months ago

Worth a shot!

paaljoachim commented 4 months ago

From my understanding of this issue is that any theme.json values should also be reflected in the UI controls of a theme.

Use case. Default theme Twenty Twenty Four has a lot of images with rounded corners. From looking at the radi UI controls one can not see this. Default themes can be used by user to learn how to set a specific setting to create a specific effect. This means that any default setting be in the top and bottom margin of Twenty Twenty Four, radi controls or other default settings need to be reflected in the UI controls so that users see the correlation between an effect and the sidebar setting controls (or Global controls) used to create the specific effect.

annezazu commented 4 months ago

My only concern with the dot is how hard it will be to see a light color with how small the dot is. With that said, always game to see a prototype and to play around with it first :) Excited to see the momentum here.

jameskoster commented 4 months ago

This came up in a discussion yesterday and there was some consensus around going back to the original idea (showing the inherited color value in the control), but providing more explanation in the popover. Very rough mockup:

Screenshot 2024-07-04 at 11 23 47

Before committing to any approach I still think it would be good to ensure we find a solution for all control types. We seem to be on to something for color and text-based controls (selects, text fields, etc). But there are also the following to consider:

I could see the text treatment we've been exploring working for several of these.

carolinan commented 4 months ago

Indicating something with color only can be an accessibility problem. As long as the control is set to the correct value, does it really matter for this issue where it is inherited from?

There are other issues open about indicating where the style is from.

jasmussen commented 4 months ago

A quick remix based on the conversation above.

Inheritance option 3

jameskoster commented 4 months ago

That looks pretty good to me. Should we do something with the unit buttons? Currently there's no way to tell if they're set locally or inherited. Could they be grey when inherited like the value and blueberry when set locally? That would align with the range/segmented control treatment.

I know there aren't that many, but I'm also missing a design for toggles (e.g. drop cap).

jasmussen commented 4 months ago

That looks pretty good to me. Should we do something with the unit buttons? Currently there's no way to tell if they're set locally or inherited. Could they be grey when inherited like the value and blueberry when set locally? That would align with the range/segmented control treatment.

Could work:

Inheritance option 3

I know there aren't that many, but I'm also missing a design for toggles (e.g. drop cap).

Do we have any toggles that can inherit values? The point is valid enough, just wondering if the context can present options.

jameskoster commented 4 months ago

Do we have any toggles that can inherit values?

Good question. The only one I can find is 'Expand on click' on the Image block. But I appreciate the UI for that might change.

richtabor commented 4 months ago

I get a sense that seeing a background color applied on the Inspector, then reading "No color selected" is from the popover may be confusing. Reading "Inherited" may also be confusing for folks.

I'm not sure this works, but perhaps something like this:

CleanShot 2024-07-12 at 16 12 24

If it's too strong to have a color applied in the picker as well, an alt:

CleanShot 2024-07-12 at 16 13 44

I kind of like the idea that we're connecting "Styles" here. Perhaps in a future iteration we could link to the relative style in global styles even.