Fix zoom out mode background color on Safari #60873
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -1.25 kB (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
MaggieCabrera
added
[Type] Bug
MaggieCabrera
added
the
[Feature] Site Editor
| body.is-zoomed-out { | ||
| display: flex; | ||
| flex-direction: column; | ||
| iframe[name="editor-canvas"].edit-site-visual-editor__editor-canvas.is-zoomed-out { |
There was a problem hiding this comment.
Should we style edit-site* classes fromthe block editor package?
There was a problem hiding this comment.
mmh, good question. We are applying the class to the iframe, but we are styling what's inside here too...
There was a problem hiding this comment.
I came here to say the same. It would b good to also support post editor
There was a problem hiding this comment.
I think we should not. Ideally the block editor should not have any visual opinion on how zoomed out background looks. It does the scaling. Then the final implementation deals with styling this by consuming the state and learning it's in zoomed out mode and applying classes to its containers.
There was a problem hiding this comment.
I think it's fine to have opinionated styles if they make sense in all block editors in zoom out mode but yeah we shouldn't target edit-site here
There was a problem hiding this comment.
if they make sense in all block editors
How can we know :)
There was a problem hiding this comment.
How can we know :)
Using good sense.
One way to try is to enable zoom-out setting in the playground story for instance and see if it looks correct.
There was a problem hiding this comment.
what's the playground story?
There was a problem hiding this comment.
packages/edit-site/src/components/block-editor/editor-canvas.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-content-overlay/content.scss
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| &.is-zoomed-out { | ||
| background-color: $gray-300; |
There was a problem hiding this comment.
What if we consistently apply this grey background? Why does it need to be white for non zoom out?
There was a problem hiding this comment.
I think it will affect other parts, like isolated views of the nav block for example. But it's a good lead, I will investigate
There was a problem hiding this comment.
Isolated views also use the gray background; I don't think white is used anywhere on its own.
There was a problem hiding this comment.
Thanks for checking! Why can't we add the background for .has-editor-padding? Might be worth commenting.
MaggieCabrera
force-pushed
the
fix-zoomout-bg-safari
branch
from
f074bc7
to
133b07e
Compare
|
I followed @ellatrix suggestion and simply changed the color of the iframe directly. I first tested changing it to red and navigated all over to see when it was applying, and I don't think it's an issue to keep it like this, since the only moment when it's visible right now is when we are in zoom out mode, but feel free to test it out in case I missed something. I checked the pattern views, the navigation block on isolation mode, the post and site editor in general, and the mobile views. |
Oops! Thanks for testing, it should be ok now. And I the playground storybook wasn't affected by this.change when I tested it running |
|
I have nothing against this PR but we do change backgrounds (gray to white, white to gray, gray to white...) every now and then. So will this be the last time :P cc @jasmussen @jameskoster for awareness as they have been involved in similar things in the past. For instance I know "white" was a conscious decision for the loading state of the site editor because most themes have a white background, so I just want to make sure there's no regression here. |
|
@youknowriad I don't think this PR changes the color, only fixes an issue in Safari where the color was not appearing as expected. So it seems fine to me. The loader was updated to use theme colors (no longer appears on a white background, unless of course the theme has a white background set). Two related issues I'd dearly love to see fixed:
Both issues are most prominent when the theme has a dark background. |
|
@youknowriad The white background colour might have been needed for themes that have no background. But now we add a white background colour inside the iframe, so it's no longer needed for that reason. gutenberg/packages/block-editor/src/components/iframe/index.js Lines 253 to 255 in 40395f9 This actually reminds me that we can now move this default white background from the Here's another thought, in case it's useful: you could make the iframe background color transparent, so it adopts whatever background color from the context, if different background colours are needed depending on the context. Overall I think this PR is great. Previously we were applying content background (white default) outside the iframe, and admin background (zoom-out) inside the iframe. Now that swapped and that makes more sense. |
@jameskoster Wasn't that there before? We actually remove a bunch of white. 🤔 If it was there before, worth looking at separately I think |
The flash was there before, it happens on trunk for me |
That's what I thought, it's probably the default white background in the lines of code I shared above, It probably shows right before the theme styles are loaded. Not sure what we can do it resolve that. The inline style will make the white background show immediately, while the theme styles take some time to load. |
I think fixing this is relevant to this PR. We are adding a background color to With the changes in this PR we don't need that anymore, and we still see the gray background for the mobile views and that fixes the problem with the border radius (at least to my eyes!) |
|
Thanks @MaggieCabrera, there are also light gray backgrounds applied to
I think that was it.
I wonder if we could check for a background color, and only apply the white fallback if one isn't found? Otherwise make it transparent. Totally fine to look at this separately, it shouldn't hold up this PR. |
|
Does that mean that for themes without background, this will result in a gray background in the site editor? |
I removed styles.color.background from my theme and it's rendering the fallback white background, as defined here: gutenberg/packages/block-editor/src/components/iframe/index.js Lines 251 to 256 in 54bf90e |
|
I've been looking at the border radius issue and Jay is right, it's not resolved yet. I tinkered a bit with it and didn't see clearly how to fix it. Let's look into that in a follow-up. I think this one is good to bring in as is, right? |
|
Thank you also for simplifying the rules :) |
|
Thank you all for your input! |
What?
Partially addresses #60601
This fixes the background color on Safari. Like @glendaviesnz correctly points out Safari is not rendering the background color on the area occupied by the element before the transform. The way we were styling it before was working because other browsers weren't behaving as intended.
Why?
It was a bug
How?
By changing the color of the iframe to be gray instead of white.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before (Safari):
After (Safari):