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

Make the editor canvas friendly towards colored backgrounds #6406

Merged
merged 5 commits into from
May 17, 2018

Conversation

jasmussen
Copy link
Contributor

One long term vision for the editor, is to allow you to load the theme stylesheet into the editor canvas itself, so you have an almost if not completely WYSIWYG experience where the theme background and fonts apply to the editor.

In the interim, we should make it easier to load editor styles into the canvas. This already works today, but the block UI is not really ready for it — white backgrounds are used here and there, and borders are colored light grays which quickly disappear on some colors.

This PR takes an initial stab at alleviating that, by moving to using various levels of opacities of the darkest gray we have, to emulate the range of opaque grays we have. As such, this PR includes a range of those opacities. For example, shown on a white background, $dark-opacity-500 should look the same as $dark-gray-500, but the latter should look good on colored backgrounds as well.

This is going to be a fair bit of work, and this is only the beginnings of it. It styles the borders, the outermost toolbar borders, the naked mover/settings/more icons, and the title field, as well as the appender. In doing this, it now means you can at least change the background color and still sort of use the editor.

Asking for thoughts now, before I do more work — what do you think of this approach?

If there's positive agreement on this direction, I will fix the drag and drop shadows/colors. Note that this PR makes the UI work on light backgrounds. If you cross a certain threshold of luminosity into the dark background territory, we need to invert the colors. This PR already contains the opacities for that, but we'll need to detect the background color and apply a body class, something like body.has-dark-background, so that I can invert the UI for that. Would appreciate thoughts on this too.

Video: https://cloudup.com/cDX4dvMYFvs

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Apr 25, 2018
@jasmussen jasmussen self-assigned this Apr 25, 2018
@jasmussen jasmussen requested a review from a team April 25, 2018 10:47
@paulwilde
Copy link
Contributor

paulwilde commented Apr 25, 2018

Looks good from what I saw in the video.

I also had the same considerations with different opacities based on if the BG is light/dark. To be honest, I cannot think of a different approach than this to add background support... so I think this is the best approach on that basis.

Something I've picked up upon from adding this on a prior site built in Gutenberg and watching the video you've attached is that the hover states of the buttons need a white background as otherwise the border looks kinda ugly.

@jasmussen
Copy link
Contributor Author

Cool, thanks for the initial sanity check, Paul. I'll let this branch sit for a bit further, but then take a stab at making further improvements.

Something I've picked up upon from adding this on a prior site built in Gutenberg and watching the video you've attached is that the hover states of the buttons need a white background as otherwise the border looks kinda ugly.

Yep — that's a solid point, and something I'll add. 🤘

@afercia
Copy link
Contributor

afercia commented Apr 25, 2018

Hmm... giving themes the power to modify the UI could potentially break any colors contrast ratio for accessibility. I'd tend to think Gutenberg should preserve the accessibility of its own interface, maybe reserving some light background around its controls.

Also, using opacity will make a bit difficult calculating the contrast ratio... as we won't be able to pick up the actual color from the CSS (or browser inspector), inteaad we'll have to use tools that pick up the colors rendered on screen 🙂

@karmatosed
Copy link
Member

karmatosed commented Apr 25, 2018

In theory this seems like a good idea on unification. I absolutely agree with breaking it down. Before I review though, how does (if at all) fit in with: #5360?

I also wonder a little from an accessibility view but right now we have that with editor styles. Maybe we can consider what boundaries we do/don't let people style?

@jasmussen
Copy link
Contributor Author

Before I review though, how does (if at all) fit in with: #5360?

Ah yes, excellent memory, thanks for bringing that back on my mind, I'd forgotten it totally.

Re-reading that post, it seems like this PR could still be complementary. I.e. we want to allow themes to load an editor style into the editor, and style every block. If a theme then opts-out of "opinionated styles" as Matías calls it, those styles would simply not load in the editor, and the theme would not have to deal with them, either in the editor or on the frontend. In that vein it would be similar to a theme that has not opted into wide images — they can still style the editor with an editor style.

@jasmussen
Copy link
Contributor Author

Responding separately due to the feedback from two angles:

I'd tend to think Gutenberg should preserve the accessibility of its own interface, maybe reserving some light background around its controls.

and

I also wonder a little from an accessibility view but right now we have that with editor styles. Maybe we can consider what boundaries we do/don't let people style?

Both good observations and thoughts. Given the overall positive reception this PR has engendered, I think this is what I'll tackle next. I feel like we have many options on the table, such as just always showing the filled-out buttons behind the side UI, or looking at things like inversion filters or mix-blend-mode, to guarantee sufficient contrast in the case of selection styles or placeholder colorings. Those are vague concepts I haven't yet dived into, but they are tools in our toolbelt that we can explore, and I think they can help us get to a point where the UI will always have contrast (for example a total inversion, when necessary, should always have sufficient contrast by definition).

That said, a badly designed theme will always be able to work around these for non-UI aspects, such as providing light gray text on a white background. Enforcing that seems like a task for the theme-review team, if at all.

@weavertheme
Copy link

I created a more or less duplicate issue #6451 (sorry to dup - I find it very hard to search existing issues)

I thnk some of the default colors on these edit controls certainly can use improvement, but I also now think this could be a theme requirement. It is possible to get very nice looking controls using just code listed in #6451. Letting the theme handle control styling alleviates trying to get a one-size-suites all solution.

@jasmussen jasmussen force-pushed the try/bg-color-friendly branch from e54bfdb to 1f0dcee Compare May 1, 2018 08:35
@jasmussen jasmussen force-pushed the try/bg-color-friendly branch 3 times, most recently from d46948f to 087c016 Compare May 1, 2018 11:17
jasmussen pushed a commit that referenced this pull request May 2, 2018
This updates the extensibility document, and adds a section around loading editor styles. It gives examples for how to change the colors of the editor (which will look good once #6406 gets merged), as well as how to change the width of the editor. The last part fixes #1483.

CC: @maddisondesigns
One long term vision for the editor, is to allow you to load the theme stylesheet into the editor canvas itself, so you have an almost if not completely WYSIWYG experience where the theme background and fonts apply to the editor.

In the interim, we should make it easier to load _editor styles_ into the canvas. This already works today, but the block UI is not really ready for it — white backgrounds are used here and there, and borders are colored light grays which quickly disappear on some colors.

This PR takes an initial stab at alleviating that, by moving to using various levels of opacities of the darkest gray we have, to emulate the range of opaque grays we have. As such, this PR includes a range of those opacities. For example, shown on a white background, $dark-opacity-500 should look the same as $dark-gray-500, but the latter should look good on colored backgrounds as well.

This is going to be a fair bit of work, and this is only the beginnings of it. It styles the borders, the outermost toolbar borders, the naked mover/settings/more icons, and the title field, as well as the appender. In doing this, it now means you can at least change the background color and still sort of use the editor.

Asking for thoughts now, before I do more work — what do you think of this approach?

If there's positive agreement on this direction, I will fix the drag and drop shadows/colors. Note that this PR makes the UI work on _light_ backgrounds. If you cross a certain threshold of luminosity into the dark background territory, we need to invert the colors. This PR already contains the opacities for that, but we'll need to detect the background color and apply a body class, something like `body.has-dark-background`, so that I can invert the UI for that. Would appreciate thoughts on this too.
@jasmussen jasmussen force-pushed the try/bg-color-friendly branch from 087c016 to 05386b8 Compare May 2, 2018 07:44
@jasmussen jasmussen force-pushed the try/bg-color-friendly branch from 7bde1c8 to 5f5868e Compare May 2, 2018 08:35
@jasmussen jasmussen added Needs Design Feedback Needs general design feedback. and removed [Status] In Progress Tracking issues with work in progress labels May 2, 2018
@jasmussen
Copy link
Contributor Author

Okay, pushed some polish, and I think this is ready for final review.

This PR now:

  • Adjusts all the UI on the canvas to be friendly to theme styles. Previously white backgrounds are now transparent, and previously gray buttons are now semi transparent.
  • Multi selections now use mix-blend-mode to ensure that the selection is always visible.

Please review any contrast issues that might have come up as part of this — I've tried my best to match the new opacity based grays to the existing grays, but if there are any regressions, they are easy to adjust.

Default — this should ideally look the same as before:

default

Light theme:

light theme

I also included colors that work based on dark themes. But this is not yet activated anywhere. Essentially we need to apply an is-dark-theme class to the body element, to invert the UI colors to use shades of white instead of shades of dark gray for the side UI and borders. This PR is ready for that class to exist, but how that class is best added should probably be explored separately. What do you think? Here's how the dark theme looks:

dark theme

Let me know your feedback.

jasmussen added a commit that referenced this pull request May 2, 2018
)

* Update theme extensibility documentation to include editor widths

This updates the extensibility document, and adds a section around loading editor styles. It gives examples for how to change the colors of the editor (which will look good once #6406 gets merged), as well as how to change the width of the editor. The last part fixes #1483.

CC: @maddisondesigns

* Simplify syntax a bit.

* Trim whitespace
border: 1px solid $dark-opacity-light-500;

.is-dark-theme & {
border: 1px solid $light-opacity-light-500;
Copy link
Contributor

@paulwilde paulwilde May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border-color: $light-opacity-light-500;

}

.edit-post-header-toolbar__block-toolbar & {
border-left: 1px solid $light-gray-500;
Copy link
Contributor

@paulwilde paulwilde May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border-left-color: $light-gray-500;

border: 1px solid $dark-opacity-light-500;

.is-dark-theme & {
border: 1px solid $light-opacity-light-500;
Copy link
Contributor

@paulwilde paulwilde May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border-color: $light-opacity-light-500;

outline: 1px dashed $dark-opacity-light-500;

.is-dark-theme & {
outline: 1px dashed $light-opacity-light-500;
Copy link
Contributor

@paulwilde paulwilde May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outline-color: $light-opacity-light-500;

border-right: 1px solid $dark-opacity-light-500;

.is-dark-theme & {
border-right: 1px solid $light-opacity-light-500;
Copy link
Contributor

@paulwilde paulwilde May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border-right-color: $light-opacity-light-500;

border-left: 1px solid $dark-opacity-light-500;

.is-dark-theme & {
border-left: 1px solid $light-opacity-light-500;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border-left-color: $light-opacity-light-500;

outline: 1px solid $dark-opacity-light-500;

.is-dark-theme & {
outline: 1px solid $light-opacity-light-500;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outline-color: $light-opacity-light-500;

@paulwilde
Copy link
Contributor

paulwilde commented May 2, 2018

Just left a few comments where it's probably best to set only the colour when is-dark-theme is applied. Otherwise you would need to update the border/outline widths/styles twice in future.

Just a small nitpick really, heh.

@noisysocks noisysocks removed the Needs Design Feedback Needs general design feedback. label May 16, 2018
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! 🌈✨ I tested this locally and the only bug I noticed was that, when using a dark background, the controls in a column block don't have visible icons:

column buttons

@@ -67,7 +67,7 @@

// This is a focus style shown for blocks that need an indicator even when in an isEditing state
// like for example an image block that receives arrowkey focus.
.edit-post-visual-editor .editor-block-list__block:not( .is-selected ) {
.edit-post-visual-editor .editor-block-list__block:not( .is-selected ):not( [data-type="core/paragraph"] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not paragraphs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This branch has been around long enough that I don't recall adding this — could it have come in as part of a rebase and later been removed?

In any case I removed it again becaues I couldn't see any regressions as a result.

@@ -1,6 +1,6 @@
.components-draggable__clone {
& > .editor-block-list__block > .editor-block-list__block-draggable {
background: $white;
background: $white; // @todo: ensure this works with themes that invert the color
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and I'm not sure that this can be done. At least I'm open to ideas here.

The thing is, the draggable clone has to have a background, or you can see the text below through it. It would be nice if we could eyedrop the color of the body, but even then a user could've applied a background image or pattern.

.edit-post-header-toolbar__block-toolbar & {
border-left-color: 1px solid $light-gray-500;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a new line at the end of the file here.

@jasmussen
Copy link
Contributor Author

Thanks for the review, @noisysocks! I pushed some changes to address your comments. Except for this one #6406 (comment) because I'm not sure how to best address that.

However, given that is-dark-theme is currently sort of a "hidden feature", I feel like the white background is a decent enough compromise for light themes, and then we can hopefully revisit this aspect if/when we enable dark theme support.

@jasmussen jasmussen added this to the 2.9 milestone May 16, 2018
@jasmussen jasmussen merged commit af65418 into master May 17, 2018
@jasmussen jasmussen deleted the try/bg-color-friendly branch May 17, 2018 07:12
@jasmussen
Copy link
Contributor Author

CC: @jorgefilipecosta — can you rebase your nested blocks for cover image branch and see if this improves things?

@mcsf
Copy link
Contributor

mcsf commented May 19, 2018

I believe this introduced regression #6836, fixed in #6844.

@@ -3,8 +3,8 @@
// value is designed to work with).

$z-layers: (
'.editor-block-list__block-edit:before': 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not certain why this change was required but it broke the PlainText inside blocks (code block for instance). It should be fixed by this though #6844

@jasmussen
Copy link
Contributor Author

My apologies for the regression, and thank you for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants