Iframe: skip scoping styles #46752
Merged
Iframe: skip scoping styles #46752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
requested review from
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Size Change: +8 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
ellatrix
commented
Dec 22, 2022
| @@ -431,7 +431,6 @@ | |||
| } | |||
|
|
|||
| .block-editor-iframe__body { | |||
| background-color: $white; | |||
There was a problem hiding this comment.
@ntsekouras This isn't a good solution because now this rule has a higher specificity than a theme just using the body selector. Also, if a theme sets it on html, it won't be visible. We need to set the white background on the iframe element so that it's not transparent. I fixed that below.
There was a problem hiding this comment.
In this brunch this issue #46291 doesn't work though since we're scaling the body of the iframe and this is where the background is applied from theme.json. Any ideas to work around that?
There was a problem hiding this comment.
Ah the scaling... I just talked to Riad about that the other day. I think we should moved the scaling outside the iframe. I'll make a separate PR for that then first.
There was a problem hiding this comment.
Would love to see this fix merged! It's breaking a lot of themes in our deployment of Gutenberg, so we've had to remove this line of CSS from our deployment. It seems like load order of CSS (for example, if CSS is concatenated) can break this.
There was a problem hiding this comment.
Ah nice, @fullofcaffeine found that this PR fixes the issue: #46732
ellatrix
force-pushed
the
try/iframe-skip-scoping-css
branch
from
b8eca00
to
683fd3e
Compare
|
Flaky tests detected in ee532e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6146965021
|
ellatrix
force-pushed
the
try/iframe-skip-scoping-css
branch
from
683fd3e
to
cd43f33
Compare
ellatrix
added
the
Backport to WP 6.7 Beta/RC
ellatrix
added
[Type] Plugin Interoperability
|
@ellatrix is this a regression? We're already in RC so should only backport fixes for actual regressions from 6.2. |
|
@tellthemachines That's arguable. This is the first release where all editors are iframed by default with the post editor included, so it's part of that feature. Scoping styles seems to cause some problems with escaped CSS property names, but this PR is way less important than Allow styles to be changed dynamically through editor settings. Worst case, people need to avoid escaped property names for now. That said, it would be nice if everything worked normally. |
|
The code change in specificity looks good to me at a glance, so I'd just in general encourage good testing. |
|
Let's add this early in the next release cycle so we have a whole cycle to test. |
|
@ellatrix for 6.4 you mean? |
|
Yes |
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
ellatrix
force-pushed
the
try/iframe-skip-scoping-css
branch
from
53455bc
to
90702f8
Compare
ellatrix
force-pushed
the
try/iframe-skip-scoping-css
branch
from
90702f8
to
ee532e8
Compare
ntsekouras
approved these changes
Sep 11, 2023
youknowriad
reviewed
Sep 14, 2023
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| // We use :where to keep specificity minimal. | |||
| // https://css-tricks.com/almanac/selectors/w/where/ | |||
| html :where(.editor-styles-wrapper) { | |||
| :where(.editor-styles-wrapper) { | |||
There was a problem hiding this comment.
Don't you think the changes in this "reset" file might impact non-framed editors?
There was a problem hiding this comment.
Changing this line has created a regression within the iframe when using some classic themes.
#57176
|
@ellatrix @ntsekouras It seems this PR created some issues with some backward compatible styles, see conversation at #56293 |
This was referenced Nov 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This is unnecessary when the content is iframed so we can skip all this work for better performance.
Fixes #52767 (comment) (CSS parser stumbling over escaped characters)
Why?
Right! Why scope if we don't have to? :)
How?
Iframes!
Testing Instructions
Check content styles are not prefix and still work in iframed editors.
Testing Instructions for Keyboard
Screenshots or screencast