-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Tiny PR] Move in-between inserter between blocks #18941
Conversation
clientId={ clientId } | ||
rootClientId={ rootClientId } | ||
/> | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure I understand the reasoning? It's clearer for me to have one unique component to show the Block UI, that component can be split into smaller components if needed but while looping through blocks I prefer to have "one entry point" that needs to be optimized.
Also, the BlockListBlock is a pure component which means it doesn't rerender when the list block does, this change wille force us to think about this for the BlockInsertionPoint as well.
Can you clarify a bit more about the reasoning and what is this trying to solve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the "in-between" inserter is nested inside the next block rather than literally in between blocks. This has some problems:
- It prevents us from making the block DOM a lot lighter and moving towards a thin wrapper without any controls in it. (See controls in popover PR.)
- The tab order is a bit strange. First you select the next block, then you select the inserter. In this PR you first tab to the insert, then you select the next block.
- From a data structure point of view, it makes more sense to have "in-between" inserters in between blocks rather than being part of a block. The block component shouldn't manage this inserter component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a styling point of view, this also makes the inserter appear exactly in the middle of two blocks, no matter what the margins are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got it, Would it better if we do the same but move the rendering inside the BlockListBlock
component? by rendering a fragment at the root level of the component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work too if you prefer.
Nice! I can tell this affects the horizontal line that shows up when you hover a block in the top block library, the dropzone indicator, so to speak: The full-wide line is probably fine here, otherwise I can probably make it "block-width". There are numerous benefits to moving as many DOM elements as possible outside of the block itself, if at all possible. It makes it simpler to style complex blocks, especially in situations where there are nested blocks. It can make the CSS necessary to make the editor look like the frontend lighter, and possibly identical to that of the frontend, through not having to worry about random fragment-divs and other elements. Not all can be removed, probably, but every bit helps. But does this also affect the sibling inserter? I.e. this one: I believe there's at least one or two tickets in the archive related to the sibling inserter and its place in the DOM, that would most likely be addressed by this PR. |
@jasmussen Yes, this especially affects the sibling, aka "in-between" inserter. :) |
"inbetweenserter" :D Awesome. I feel like perhaps @ZebulanStanphill may have an idea which ticket(s) if any this might resolve. |
39f7853
to
0ddcb92
Compare
6526986
to
0c8971f
Compare
34ed6ec
to
419e95b
Compare
This is definitely a very frequently recurring accessibility topic as well (cc @afercia). I'm currently failing to find if/where an issue exists to track it. It'd been on my mind lately to revisit the reason we'd had the sibling inserter before the block, vs. after it, and relevant for this pull request I wonder if that's something we should consider as part of these changes. Thinking back, I think it might have been a convenience for intending to show/not show it before the first/last blocks in the editor, but that hardly seems a problem that couldn't be solved by some other means while still retaining a better default tab behavior. Then again, I guess as long as from the perspective of the user, this feels like the inserter is truly before the block (and not the first stop within it), I guess it achieves the same effect? |
@aduth I don't care so much if the inserter is rendered before or after the block. The main thing here is that the inserter is rendered outside of the block. We can always change it later to render it after the block if needed. The current approach makes as little changes as possible. |
Do your changes make the code here unnecessary? gutenberg/packages/block-editor/src/components/block-list/insertion-point.js Lines 28 to 37 in aa8239b
I believe the above existed purely because otherwise being rendered within |
3a819de
to
49c95ed
Compare
@aduth I think you're right. The propagation stopping shouldn't be needed anymore. I removed it now. |
@aduth Also, regarding the before vs after positioning: when the inserter is rendered outside of the block, I don't think it matters much. The position is just in between the blocks and the tab order is fixed. |
Notably, the toolbar always appears on top of the inserter, so it's not as much of an issue as it was the last time. The tab order makes a lot more sense now, and the block edit DOM is a bit better now. 👍 I still think the sibling inserter should only appear after the selected block (#13571), but aside from the aforementioned regression, this is a definite improvement over |
@ZebulanStanphill I see that the inserter doesn't even appear in |
@ZebulanStanphill Should be fixed in with the last commit. Will display: none still allow tabbing to it? I'll test it. |
No, it looks like we can't use |
@ellatrix Good observation, and yeah, the latest commit hadn't shown up on GitHub when I made my comment.
EDIT: Unfortunately, I've discovered a big new issue. See my next comment. |
Unfortunately, I just discovered another issue. The editor styles for the Columns block is messed up due to the This is made more complicated by the fact that the sibling inserter is not always part of the DOM next to the Column block. Sometimes it just suddenly disappears and the Column block spacing suddenly looks fine because of that. I think there's some code somewhere that tries to stop the sibling inserter from appearing next to a Column block, which is partially broken due to the new DOM position of the sibling inserter. This brings up the issue that the new DOM position of the sibling inserter is likely to cause issues with editor styles in some complex 3rd party blocks that use nesting, so a public developer note about the change would probably be a good idea. I think there is a better spot in the DOM for the sibling inserter. The sibling inserter should instead be moved into a popover (just like you're planning to do with the block toolbar). This popover would only appear for the selected block (e.g. #13571). That would prevent stuff like the |
Thank you Zeb for testing, and thank you Ella for fighting the good fight. It's getting into the complicated stuff, but long term this is removing a ton of hacky behavior for much more solid code. The fight is worth it! ✊ |
@ZebulanStanphill I agree that a popover would be a better place for the inserter, but I ran into issues because of the hover behaviour. We need a "target" to be always there so we know when to show the inserter. Options:
Maybe there's more solutions I didn't think about? But so far, option one sounded like the best one. |
It looks like we cannot get this in an ideal state with the current approach, so I'm closing this PR. I have another better idea that might just work out, but I'll do as a PR separate and parallel to #18779. |
@ellatrix If you just make the sibling inserter always visible below the selected block and only visible below the selected block, you don't have to worry about hover behavior at all. |
@ZebulanStanphill I'm not sure how that helps. I want to move the inserter DOM out of the block DOM. And we also want the inserter on hover for all blocks, not just the selected block. |
@ellatrix What I'm suggesting is that we shouldn't be showing the sibling inserter for all blocks. I guess that's out-of-scope for a refactor PR, though. If you want to maintain the current behavior, then yeah, I have no idea how to pull that off with a popover. 🤷♂️ |
Worth noting that the sibling inserter should be available between all blocks, even when no blocks are selected. |
Thanks for the ping. Commenting to clarify even if this PR is now closed. I'd agree that moving the inserter out of the block UI would make its before / after placement related issue pointless. Right now, the problem is that the inserter is within the block. Many users (e.g. keyboard users, screen reader users) navigate the content in a linearized way. When they land within a block, they find several controls that belong to the block UI before they can finally land on the block content. The inserter, the movers, the block toolbar: they all come before the block main content in the source order. This makes these users experience far from ideal. Ideally, users should find the block main content as first thing. After that, the controls for additional actions: move, formatting, add. This would also replicate what is the most logical workflow, where everyone would expect to
As always for accessibility, the logical order and grouping of UI controls is paramount. Worth noting the accessibility team asked a few times to have an option to move also the block toolbar after the block. That said, if the inserter gets moved out from the block, that would work because it's a tab stop less to navigate within the block. Of course, the "external" inserter should not be rendered in the editor navigation mode. |
@ZebulanStanphill I'll find a way :) |
Description
Split out from #18779, an effort to lighten the block component and DOM.
I believe this partly resolves #13571.
How has this been tested?
Screenshots
Types of changes
Checklist: