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

[Tiny PR] Move in-between inserter between blocks #18941

Closed
wants to merge 12 commits into from

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Dec 5, 2019

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:

  • 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 changed the title Move in-between inserter between blocks [Tiny PR] Move in-between inserter between blocks Dec 5, 2019
@ellatrix ellatrix added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Package] Block editor /packages/block-editor labels Dec 5, 2019
@ellatrix ellatrix requested a review from jasmussen December 5, 2019 12:44
clientId={ clientId }
rootClientId={ rootClientId }
/>
) }
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

@youknowriad youknowriad requested a review from aduth December 5, 2019 13:13
@jasmussen
Copy link
Contributor

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:

Screenshot 2019-12-05 at 15 24 24

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:

Screenshot 2019-12-05 at 15 31 36

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.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@jasmussen Yes, this especially affects the sibling, aka "in-between" inserter. :)

@jasmussen
Copy link
Contributor

"inbetweenserter" :D

Awesome. I feel like perhaps @ZebulanStanphill may have an idea which ticket(s) if any this might resolve.

@ellatrix ellatrix force-pushed the try/move-in-between-inserter branch from 39f7853 to 0ddcb92 Compare December 5, 2019 14:46
@ellatrix ellatrix force-pushed the try/move-in-between-inserter branch from 6526986 to 0c8971f Compare December 5, 2019 15:10
@ellatrix ellatrix force-pushed the try/move-in-between-inserter branch from 34ed6ec to 419e95b Compare December 5, 2019 16:18
@aduth
Copy link
Member

aduth commented Dec 5, 2019

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.

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?

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@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.

@aduth
Copy link
Member

aduth commented Dec 5, 2019

Do your changes make the code here unnecessary?

onFocusInserter( event ) {
// Stop propagation of the focus event to avoid selecting the current
// block while inserting a new block, as it is not relevant to sibling
// insertion and conflicts with contextual toolbar placement.
event.stopPropagation();
this.setState( {
isInserterFocused: true,
} );
}

I believe the above existed purely because otherwise being rendered within IgnoreNestedEvents would make the inserter subject to those default behaviors. At least for onMouseDown, we didn't want it to select the block, hence this code. I'm not really sure if the other event handlers are intentional, or if we might run into issues in nested contexts with event bubbling intended to be absorbed by IgnoreNestedEvents.

@ellatrix ellatrix force-pushed the try/move-in-between-inserter branch from 3a819de to 49c95ed Compare December 5, 2019 17:09
@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@aduth I think you're right. The propagation stopping shouldn't be needed anymore. I removed it now.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@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.

@ellatrix ellatrix requested a review from youknowriad December 5, 2019 18:36
@ZebulanStanphill
Copy link
Member

This old issue is back:
image

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 master.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@ZebulanStanphill I see that the inserter doesn't even appear in master if the block is selected. Need to check what code is responsible for that.

@ZebulanStanphill
Copy link
Member

The button is still there for the selected block; it's just invisible now.
image

I guess you could try using display: none on the inserter above the selected block?

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

@ZebulanStanphill Should be fixed in with the last commit. Will display: none still allow tabbing to it? I'll test it.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 5, 2019

No, it looks like we can't use display: none; because then you cannot tab to it when going backwards (behaviour in master).

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Dec 5, 2019

@ellatrix Good observation, and yeah, the latest commit hadn't shown up on GitHub when I made my comment.

Everything is working correctly now. 👍

EDIT: Unfortunately, I've discovered a big new issue. See my next comment.

@ZebulanStanphill
Copy link
Member

Unfortunately, I just discovered another issue. The editor styles for the Columns block is messed up due to the :first-child pseudo-class now matching the sibling inserter rather than the Column block.
image

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.
image

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 :first-child pseudo-class issue from happening.

@jasmussen
Copy link
Contributor

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! ✊

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

@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:

  • A "target" element between the blocks as is done in this PR, so it takes advantage of the native :hover.
  • Listening to mousemove events in the block. This might end up being expensive as it is multiplied by the amount of blocks in the editor. It also isn't aware of the margin between blocks.
  • Listening to mousemove once per editor, but it's not simple to achieve, and you'd also have to render right in time for tabbing.

Maybe there's more solutions I didn't think about? But so far, option one sounded like the best one.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

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 ellatrix closed this Dec 6, 2019
@ellatrix ellatrix deleted the try/move-in-between-inserter branch December 6, 2019 08:31
@ZebulanStanphill
Copy link
Member

@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.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

@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.

@ZebulanStanphill
Copy link
Member

@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. 🤷‍♂️

@jasmussen
Copy link
Contributor

Worth noting that the sibling inserter should be available between all blocks, even when no blocks are selected.

@afercia
Copy link
Contributor

afercia commented Dec 6, 2019

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

  1. first add content
  2. and then format the content, move the content, find additional options and as last thing find the inserter to add a new block

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.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

@afercia That's definitely the plan. #18779 is an effort to move the controls to popovers out of the block. This will eventually be done for the insertion point as well. Ideally it should more between the block or into a popover, but definitely out of the block DOM.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

If you want to maintain the current behavior, then yeah, I have no idea how to pull that off with a popover. 🤷‍♂️

@ZebulanStanphill I'll find a way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overhaul sibling inserter to only appear after a selected block
6 participants