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

Block: remove inner div wrapper #19593

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Block: remove inner div wrapper #19593

merged 1 commit into from
Jan 15, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 13, 2020

Description

This PR removes the inner div wrapper of blocks. This wrapper was previously used to contain the block content so event listeners on this wrapper would ignore events from the block controls. Since the controls are now removed from the block DOM, this wrapper is no longer needed.

Looks like this improves performance by another 3%.

Additionally, this is part of an effort to lighten the block DOM, with the ultimate goal to make it easier for theme author to style the editor content and to enable a semantic relationship between a block and inner blocks.

How has this been tested?

Block alignments, blocks with inner blocks, and multi-selection.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@ellatrix ellatrix force-pushed the try/remove-block-inner-div branch from c8374b3 to b2220b9 Compare January 13, 2020 17:27
@ellatrix ellatrix requested a review from jasmussen January 14, 2020 08:20
@ellatrix ellatrix marked this pull request as ready for review January 14, 2020 08:20
@ellatrix ellatrix added [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts Needs Dev Note Requires a developer note for a major WordPress release cycle labels Jan 14, 2020
@jasmussen
Copy link
Contributor

Impressive work.

These last few wrappers are hard to remove, though! A lot of blocks, if not core blocks, target [data-block] as the indicator of just the block contents.

Let me zoom back a little bit, and bear with me as it might get long. I'm trying to understand the consequences myself as well.

It gets to the question of: what is a block? The correct answer is, it's anything you as a developer decide is a block.

It does not just map to HTML. Take the Paragraph block, for instance — right now it maps one to one: a single p, a single Paragraph block:

<p>Your content</p>

However you could create a block that simply output this:

<p>Your content</p>
<p>Also your content</p>

This would be a silly block, but it's possible!

Now let's say I was a developer that wanted to affect every single block with my plugin. For example maybe I wanted to add a global option to adjust the size of the spacing between each block. I cannot predict the markup of every block out there, so I look for a generic HTML element present in the markup that I know will be there.

For now, and I've seen this in a few 3rd party blocks, this has been [data-block]: the closest element to the contents of the block, that is consistent across every block.

Could I target .wp-block instead? Should I? Could we move the [data-block] attribute to the parent container instead? Probably?

Don't get me wrong — for every wrapping element you remove, we make the future development experience better for ever new block created. But for one, I think we'll always need at least one wrapping element to define the outermost boundary of the block. And this PR, specifically, will need to be tested with quite a few plugins to see if/how it breaks things. For example, I know that it will break https://wordpress.org/plugins/layout-grid/.

I'm not sure it will work, but can we move the [data-block] attribute to the parent?

@ellatrix
Copy link
Member Author

@jasmussen Yes, we could add [data-block] to the wrapper to have more compatibility. The outer wrapper is now the one that is closest to the block contents and also only contains the block contents. There's no reason for the inner wrapper to exist anymore.

And yes, you're right. We'll always need the wrapping element. It was never my intention to remove the outer wrapper for all block. Maybe I miscommunicated that. It's my intention to make leaving away the outer wrapper opt-in by the block. This could be useful in the list and table block for example.

@jasmussen
Copy link
Contributor

Yes, we could add [data-block] to the wrapper to have more compatibility.

I think that's worth a shot. If you want to try, I have a couple of plugins I can test!

@ellatrix
Copy link
Member Author

Added the [data-block] attribute back.

@jasmussen
Copy link
Contributor

With the restoration of the data-block attribute, so far my test results with a few plugins including the aformentioned layout-grid and coblocks, the results are promising. Things seem to work right!

I think it would be good if more people could test this with a variety of content and plugins, though — the change is as big as it is important. Good for it to receive lots of eyes.

@ellatrix ellatrix requested a review from a team January 14, 2020 10:06
@ellatrix
Copy link
Member Author

Thanks for testing @jasmussen! I'll let it sit a bit longer.

@aduth
Copy link
Member

aduth commented Jan 14, 2020

Considering we needed to change several of the core blocks to accommodate this change, would it be reasonable to expect this might be the case for other third-party custom block implementations? I would think it should fall within allowable changes, but wonder if it would be something to consider for a Dev Note.

(Aside: I was hoping to use wpdirectory.net to try to search for some potential plugin/theme conflicts, but it doesn't appear to be working at the moment)

@ellatrix
Copy link
Member Author

@aduth Yes, I've already added the "Needs Dev Note" label. I think there can be tiny visual changes for more complex blocks, but nothing more. Blocks will continue to function normally without errors. This div element makes it actually more difficult for complex blocks to style them.

@aduth
Copy link
Member

aduth commented Jan 14, 2020

@aduth Yes, I've already added the "Needs Dev Note" label. I think there can be tiny visual changes for more complex blocks, but nothing more. Blocks will continue to function normally without errors. This div element makes it actually more difficult for complex blocks to style them.

Ah! I missed that, sorry.

I'd agree with you on your general point. The potential conflicts here are more likely of the sort where the theme is anticipating some rigid DOM hierarchy, which this change would appear to mitigate to a degree.

@ellatrix ellatrix force-pushed the try/remove-block-inner-div branch from 62a9ecf to 4f80081 Compare January 14, 2020 16:00
@mapk
Copy link
Contributor

mapk commented Jan 15, 2020

I really appreciate these "lightening the DOM" PRs. Thanks Ella!

I tested this out with most blocks using the Twenty Twenty theme and didn't see any visual problems. 👍

@ellatrix
Copy link
Member Author

Let's do it. This allows us to move forward with further improvements. I don't see any issues with removing it. At most a plugin or theme will have to make a small correction.

@ellatrix ellatrix merged commit 17e5c2d into master Jan 15, 2020
@ellatrix ellatrix deleted the try/remove-block-inner-div branch January 15, 2020 20:33
@youknowriad
Copy link
Contributor

Awesome work as always.

@jasmussen
Copy link
Contributor

I am as excited for the lean markup we have now, and what it enables for simpler block dev as I can be. I do think we should perhaps make the dev note bold, because it will break some blocks. It's worth it, and it's stellar work, but it's a biggish change!

@ellatrix
Copy link
Member Author

@jasmussen What do you mean with making it bold? I'm all for more notes and more communication about it.

@jasmussen
Copy link
Contributor

What do you mean with making it bold?

Oh I mean it literally. Make the text in the dev note bold! :)

@ellatrix ellatrix mentioned this pull request Jan 16, 2020
6 tasks
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
@ktmn
Copy link

ktmn commented Jan 23, 2020

That change is nice to see, the selectors were ridiculously long.

If I understand correctly, with #19010 as well, a nested block selector changed from:

.wp-block > .block-editor-block-list__block-edit > [data-block] > .my-parent > .block-editor-inner-blocks > .block-editor-block-list__layout > .wp-block > .block-editor-block-list__block-edit > [data-block] > .my-child

to:

.wp-block > .my-parent > .block-editor-inner-blocks > .block-editor-block-list__layout > .wp-block > .my-child

Could this be simplified further? Is .block-editor-inner-blocks > .block-editor-block-list__layout entirely necessary or could it be only 1 wrapper for children?

.wp-block > .my-parent > .block-editor-inner-blocks > .wp-block > .my-child

Could it be 0?

.wp-block > .my-parent > .wp-block[data-inner="true"] > .my-child

.wp-block[data-block="~uid"] > .my-parent > .wp-block[data-parent="~uid"] > .my-child

@ellatrix
Copy link
Member Author

@ktmn There's definitely plans for further simplification, but I couldn't do everything in a single release. .block-editor-inner-blocks will disappears, .wp-block and .block-editor-block-list__layout will be opt-out.

@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
pablinos added a commit to Automattic/jetpack that referenced this pull request Feb 26, 2020
In WordPress/gutenberg#19593 a wrapper `div` was removed, which had a
`position` value of `relative`. Our overlay `div` to prevent the iframe
from being interacted with, relies on a parent element being
`positino: relative`. This is still the case when the block is in the
main flow/centre aligned, but when the block is left or right aligned,
it can't be selected, and clicks are handled by the iframe.

I think the above PR is in Gutenberg 7.3 and higher, so this problem can
be replicated with the plugin installed, and therefore also on WPCOM.
mdbitz pushed a commit to Automattic/jetpack that referenced this pull request Feb 27, 2020
In WordPress/gutenberg#19593 a wrapper `div` was removed, which had a
`position` value of `relative`. Our overlay `div` to prevent the iframe
from being interacted with, relies on a parent element being
`positino: relative`. This is still the case when the block is in the
main flow/centre aligned, but when the block is left or right aligned,
it can't be selected, and clicks are handled by the iframe.
mdbitz pushed a commit to Automattic/jetpack that referenced this pull request Feb 27, 2020
#14820)

In WordPress/gutenberg#19593 a wrapper `div` was removed, which had a
`position` value of `relative`. Our overlay `div` to prevent the iframe
from being interacted with, relies on a parent element being
`positino: relative`. This is still the case when the block is in the
main flow/centre aligned, but when the block is left or right aligned,
it can't be selected, and clicks are handled by the iframe.
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants