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

Fix regression with column block being selected. #14876

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Apr 9, 2019

The columns block is currently slightly fiddly, due to the nesting nature of Columns > Column > Blocks structure. All three levels are "Blocks" in the block editor sense, and all can be selected. This is pending enhancements in #9628.

However in the mean time, we have a hack in place, this uses pointer-events to prevent the mouse selection of the individual column blocks. This makes it drastically simpler to select the Columns block, which is where you can adjust the number of columns in the block. The individual column blocks, on the other hand, is currently largely inactionable. You can set alignments on them, which is why #9628 is urgent, but until we have a better system for selecting parent blocks, this pointer-events hack is arguably the better stopgap solution.

That hack regressed in master. This PR restores it, and makes it slightly better.

Things to test:

  • Insert a columns block with content, and verify you can select both child and parent block.
  • Insert an "Archives" block inside a column, and verify that the block itself is clickable, but the links inside are not (it's using component-disabled to make archive links unfollowable)

This branch:

this branch

Master:

master

The columns block is currently slightly fiddly, due to the nesting nature of Columns > Column > Blocks structure. All three levels are "Blocks" in the block editor sense, and all can be selected. This is pending enhancements in #9628.

However in the mean time, we have a hack in place, this uses `pointer-events` to prevent the _mouse_ selection of the individual column blocks. This makes it drastically simpler to select the Columns block, which is where you can adjust the number of columns in the block. The individual column blocks, on the other hand, is currently largely inactionable. You can set alignments on them, which is why #9628 is urgent, but until we have a better system for selecting parent blocks, this pointer-events hack is arguably the better stopgap solution.

That hack regressed in master. This PR restores it, and makes it slightly better.

Things to test:

- Insert a columns block with content, and verify you can select both child and parent block.
- Insert an "Archives" block inside a column, and verify that the block itself is clickable, but the links inside are not (it's using component-disabled to make archive links unfollowable)
@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Apr 9, 2019
@jasmussen jasmussen self-assigned this Apr 9, 2019
@jasmussen jasmussen requested a review from a team April 9, 2019 08:29
@gziolo gziolo requested a review from getdave April 9, 2019 09:02
@gziolo gziolo added the [Block] Columns Affects the Columns Block label Apr 9, 2019
:not(.components-disabled) > .wp-block-columns > .editor-inner-blocks > .editor-block-list__layout > [data-type="core/column"] > .editor-block-list__block-edit > * {
pointer-events: all;
//pointer-events: all;
Copy link
Member

Choose a reason for hiding this comment

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

Should this whole section be removed as it doesn't contain any active CSS rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aargh, is it monday? Sorry about that, and thanks: 118fbfe

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Is there any sense of how this regression happened? Given the presence of block-editor- class names, it is possible to have been a result of splitting editor from block-editor, though those issues 'til now have largely been about use of a wrong or misnamed class. That doesn't seem to be the case here. I wonder if it could be a change in the order in which stylesheets are loaded, in which case it would signal fragile assumptions around order with equal specificities, possibly still problematic even after these changes.

}
}

// This selector re-enables clicking on any child of a column block.
:not(.components-disabled) > .wp-block-columns > .editor-inner-blocks > .editor-block-list__layout > [data-type="core/column"] > .editor-block-list__block-edit > * {
Copy link
Member

@aduth aduth Apr 9, 2019

Choose a reason for hiding this comment

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

Will we be causing an issue in the fact we're removing this specific handling for "Disabled" context?

From #11620:

It improves the UI around reusable blocks, by fixing an issue where even though contents should be disabled when in a reusable block, it was still selectable if there were nested blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for this reminder. I did forget to test reusable blocks. Thankfully this seems to work also as intended. GIF:

gif

Here's how the code works:

.wp-block-columns [data-type="core/column"] {
	pointer-events: none;

	// This selector re-enables clicking on any child of a column block.
	.block-core-columns .block-editor-block-list__layout {
		pointer-events: all;
	}
}

Basically the column block, and every child below, is made unclickable.

After that, only the .block-editor-block-list__layout element that is a child, is made clickable again, which appears to be sufficient.

The specificity is lower than that of any of the .components-disabled blocks, such as archives or reusable blocks, therefore the pointer-events: none attached to those is unaffected.

In the previous version, both > and * were used to make elements clickable again, which was both unnecessary because of inheritance, but also too specific, needing the :not selector.

@jasmussen
Copy link
Contributor Author

Thank you for the review!

Is there any sense of how this regression happened?

No, I'm not quite sure. I'm assuming it happened due to the classname changes that were part of the move to a generic block editor, but I'm not totally sure. From just parsing the existing rules, it could be a change in the markup — the previous rules were heavy on adjacent sibling selectors, so even a single added fragment to some of the block markup or to the block editor itself could do it.

The new markup should be slightly more resilient in that vein, even though I still consider it "stopgap". Given that the selectors are simpler now, and do not rely on :not's, I would suggest this makes it more resilient, but I'd appreciate thoughts on how to better accomplish this.

@getdave getdave added this to the 5.5 (Gutenberg) milestone Apr 10, 2019
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

As far as I can see this all looks good. I've tested and it stops selection of the single column Blocks.

@mapk
Copy link
Contributor

mapk commented Apr 10, 2019

we have a hack in place, this uses pointer-events to prevent the mouse selection of the individual column blocks.

Tested and it worked great. This just means I can no longer adjust the vertical alignment of the content within an individual column, correct?

Master

cols

This branch

cols-branch

@jasmussen
Copy link
Contributor Author

Tested and it worked great. This just means I can no longer adjust the vertical alignment of the content within an individual column, correct?

No, you can still select these using the keyboard through arrowkey navigation through blocks, it steps through every nested block.

Or you can use the mouse to select the block navigation button at the top, to pick those.

I agree it would be nice to easily select the column block by simply clicking in the block itself, but I believe that's why we need to prioritize #9628 as the better solution for that. In the mean time, we have a choice between making it possible to select the columns block where you can set the amount of columns and overall vertical alignments, OR, select the individual column blocks where you can set the vertical alignment of individual columns. This PR suggests the former is more valuable, until we can have both (through #9628).

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

I agree it would be nice to easily select the column block by simply clicking in the block itself, but I believe that's why we need to prioritize #9628 as the better solution for that. In the mean time, we have a choice between making it possible to select the columns block where you can set the amount of columns and overall vertical alignments, OR, select the individual column blocks where you can set the vertical alignment of individual columns. This PR suggests the former is more valuable, until we can have both (through #9628).

This sounds great — I totally agree. I'm hoping to start pushing on #9628 soon, so hopefully we can get that in place. In the meantime, this helps make selecting column blocks easier in general.

This PR in general is working well for me. I'm unable to select individual columns with the mouse, but I can do so with the keyboard. 👍

It sounds like that discussion above around reusable blocks is resolved too, so if that's the case this seems good to go.

@jasmussen
Copy link
Contributor Author

Thank you Kjell.

Since this PR makes fixes a columns block regression, can we make the next release for this one? Otherwise the columns block is going to be a bad experience for a whole release.

@youknowriad
Copy link
Contributor

yep let's merge @jasmussen

@jasmussen jasmussen merged commit b149150 into master Apr 11, 2019
@jasmussen jasmussen deleted the fix/columns-regression branch April 11, 2019 15:05
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Fix regression with column block being selected.

The columns block is currently slightly fiddly, due to the nesting nature of Columns > Column > Blocks structure. All three levels are "Blocks" in the block editor sense, and all can be selected. This is pending enhancements in WordPress#9628.

However in the mean time, we have a hack in place, this uses `pointer-events` to prevent the _mouse_ selection of the individual column blocks. This makes it drastically simpler to select the Columns block, which is where you can adjust the number of columns in the block. The individual column blocks, on the other hand, is currently largely inactionable. You can set alignments on them, which is why WordPress#9628 is urgent, but until we have a better system for selecting parent blocks, this pointer-events hack is arguably the better stopgap solution.

That hack regressed in master. This PR restores it, and makes it slightly better.

Things to test:

- Insert a columns block with content, and verify you can select both child and parent block.
- Insert an "Archives" block inside a column, and verify that the block itself is clickable, but the links inside are not (it's using component-disabled to make archive links unfollowable)

* Fix mess ¯\_(ツ)_/¯
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Fix regression with column block being selected.

The columns block is currently slightly fiddly, due to the nesting nature of Columns > Column > Blocks structure. All three levels are "Blocks" in the block editor sense, and all can be selected. This is pending enhancements in #9628.

However in the mean time, we have a hack in place, this uses `pointer-events` to prevent the _mouse_ selection of the individual column blocks. This makes it drastically simpler to select the Columns block, which is where you can adjust the number of columns in the block. The individual column blocks, on the other hand, is currently largely inactionable. You can set alignments on them, which is why #9628 is urgent, but until we have a better system for selecting parent blocks, this pointer-events hack is arguably the better stopgap solution.

That hack regressed in master. This PR restores it, and makes it slightly better.

Things to test:

- Insert a columns block with content, and verify you can select both child and parent block.
- Insert an "Archives" block inside a column, and verify that the block itself is clickable, but the links inside are not (it's using component-disabled to make archive links unfollowable)

* Fix mess ¯\_(ツ)_/¯
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Fix regression with column block being selected.

The columns block is currently slightly fiddly, due to the nesting nature of Columns > Column > Blocks structure. All three levels are "Blocks" in the block editor sense, and all can be selected. This is pending enhancements in #9628.

However in the mean time, we have a hack in place, this uses `pointer-events` to prevent the _mouse_ selection of the individual column blocks. This makes it drastically simpler to select the Columns block, which is where you can adjust the number of columns in the block. The individual column blocks, on the other hand, is currently largely inactionable. You can set alignments on them, which is why #9628 is urgent, but until we have a better system for selecting parent blocks, this pointer-events hack is arguably the better stopgap solution.

That hack regressed in master. This PR restores it, and makes it slightly better.

Things to test:

- Insert a columns block with content, and verify you can select both child and parent block.
- Insert an "Archives" block inside a column, and verify that the block itself is clickable, but the links inside are not (it's using component-disabled to make archive links unfollowable)

* Fix mess ¯\_(ツ)_/¯
This was referenced Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants