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

Revert img, iframe styles to block editor container scope #18287

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 5, 2019

Previously: #14407
Alternative to: #18240, #18281

This pull request seeks to revert styling applied globally to img and iframe (Update: and figcaption) to that which existed prior to #14407. Prior to #14407, these styles were scoped to only apply within the editor. As of #14407, they apply to these elements globally (including on the front-end), thus introducing some conflict.

There is discussion on an ideal solution to this scenario in #18240. However, for the purposes of a release in time for 5.3, the proposed changes here limit the scope to reverting a previously-known state.

Specific change being reverted: https://github.com/WordPress/gutenberg/pull/14407/files#diff-556b1933de5650dc555c469c584accc2L91

@aduth aduth added the [Type] Bug An existing feature does not function as intended label Nov 5, 2019
@aduth aduth requested a review from jasmussen November 5, 2019 14:03
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

This seems wise enough. If @jasmussen can still have a look that'd be ace, but here's my approval.

@aduth
Copy link
Member Author

aduth commented Nov 5, 2019

I notice the stylesheet for WordPress 5.2.4 doesn't include any styles for the barebone figcaption element either, this was introduced with #14366. It doesn't seem that anything really depends on it having been moved, though based on the preceding comment I guess it was the intention to apply globally regardless of theme defining. My primary concern is that these styles aren't limited to blocks, they can impact any content anywhere on any page of a site.

@jasmussen
Copy link
Contributor

This one does remove the figcaption rule, from what I can tell. I'm personally fine with that, and the context for the rule is here: #14407 (comment) — choice quote:

The idea is that we know we want to apply a modicum of styles to blocks, either to support them structurally (function in the first place), or to make sure we don't provide a broken or unstyled-looking experience. In the case of the figcaption element as is the core of that other PR, this element is born without margins, which means it sits right up next to the image unless it receives a margin.

The question is, should Gutenberg have such a base stylesheet that cascades? I've collected the two existing rules we already have, for img and iframe, and once that other PR goes through, we'd also have figcaption on this list. Should p also be there? And should there be a way to opt-in to this vanilla stylesheet, or opt out of it? These are questions that would be good to figure out, and I would appreciate your thoughts.

But like I said it's likely fine to do this.

But it does seem to be beyond the scope of this PR initially, to just remove the img and iframe styles. I'm fine with either, and on this one will defer to those of you skilled in picking cherries.

👍 👍

Thanks so much for this, Andrew!

@aduth
Copy link
Member Author

aduth commented Nov 5, 2019

I certainly don't mean to undo the efforts involved in #14407, and I'm especially conscious there was quite a bit of prior discussion. Essentially, I can agree with the stated goal of having some "modicum of styles to blocks", even to those blocks which don't opt-in. My issue is that these styles aren't scoped to blocks, they apply to every element on the page.

Looking through a theme repository search, I could see how the previous style might inadvertently impact a number of themes (e.g. Customizr's author info, Covernews' posts grid widget, etc).

As I understand it, removing the style as proposed here should at least not regress on existing blocks until we can decide how to avoid those conflicts.

@mcsf
Copy link
Contributor

mcsf commented Nov 5, 2019

The idea is that we know we want to apply a modicum of styles to blocks

I still think this is something to strive for, but ideally we’d be able to do this in a way that only affects blocks, no?

@jasmussen
Copy link
Contributor

Yes that seems sensible. I'm 👍👍 on this for the technical considerations.

@aduth
Copy link
Member Author

aduth commented Nov 5, 2019

I observed after my last changes that #14407 was largely about moving caption styles to apply to the theme opt-in stylesheet. Thus, in order to not regress in applying margins (which are expected to apply to this subset of blocks without opt-in), it was necessary to reintroduce those styles explicitly to the style.scss for each block (see 9e8da84).

Effectively reinstates:

@aduth aduth merged commit 0d8ee84 into master Nov 5, 2019
@aduth aduth deleted the revert/img-iframe-editor-style branch November 5, 2019 16:04
jorgefilipecosta pushed a commit that referenced this pull request Nov 5, 2019
* Revert img, iframe styles to block editor container scope

* Block Library: Remove all vanilla styles

* Block Library: Restore non-theme front-end margins for figcaption
jorgefilipecosta pushed a commit that referenced this pull request Nov 5, 2019
* Revert img, iframe styles to block editor container scope

* Block Library: Remove all vanilla styles

* Block Library: Restore non-theme front-end margins for figcaption
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 5, 2019
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Nov 5, 2019
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 5, 2019
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Nov 5, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
miya0001 pushed a commit to cjk4wp/wordpress that referenced this pull request Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants