Sibling inserter click redirect: add e2e tests #19729
Conversation
ellatrix
added
[Type] Bug
ellatrix
requested review from
ajitbohra,
nerrad,
ntwb,
talldan and
youknowriad
as code owners
There was a problem hiding this comment.
This fixes a lot of the issues I was seeing with #19719 👍
It's the same on master, but one thing I notice is there must be a very small gap somewhere, or a specific browser difference in transitioning from the contenteditable to the wrapper (with cursor: text) that causes the cursor to flicker. It is very subtle, but quite nagging when you finally notice it:
https://cloudup.com/cn1pGnLf6hC
One thought I have is: Should we just want the entire paragraph to be styled as cursor: text so there is no transition? It makes me wonder as well, considering that not every block is text based, whether defaulting to cursor: text in these gaps is a sensible default, especially for more layout-heavy posts/pages. I guess this is the same question on the spectrum of the block editor behaving like a text editor vs. a layout arranger. In any case, if we did decide to move this to be a style of the paragraph block specifically, it would serve as something of a solution to this potential concern.
| const x = paragraphRect.x + ( 2 * paragraphRect.width / 3 ); | ||
| const y = paragraphRect.y + paragraphRect.height + 10; | ||
|
|
||
| await page.mouse.move( x, y, { steps: 10 } ); |
| await page.keyboard.type( '2' ); | ||
| await page.keyboard.press( 'ArrowUp' ); | ||
|
|
||
| const paragraph = await page.$( '[data-type="core/paragraph"]' ); |
There was a problem hiding this comment.
What is this logic doing? Can we add an inline comment?
e.g.
| const paragraph = await page.$( '[data-type="core/paragraph"]' ); | |
| // Find a point outside the paragraph between the blocks where it's not | |
| // expected that the sibling inserter would be placed. | |
| const paragraph = await page.$( '[data-type="core/paragraph"]' ); |
The exercise of writing this out has me wondering though: The logic as implemented isn't really specific to choosing a point where the inserter isn't going to be, it's only "hoping" that because the sibling inserter is at the centerpoint, that a point 2/3 of the horizontal width of a paragraph would be enough displaced from the sibling inserter to be able to click on.
Could we instead select the inserter element itself, and do something like x = inserterBox.right + 1; ?
There was a problem hiding this comment.
The inserter isn't there yet at this time. We're moving the mouse so it appears in the right place. I added your (adjusted) comment and updated the code.
| await page.keyboard.type( '1' ); | ||
| await page.keyboard.press( 'Enter' ); | ||
| await page.keyboard.type( '2' ); | ||
| await page.keyboard.press( 'ArrowUp' ); |
There was a problem hiding this comment.
What's the ArrowUp for? Is it just to get the contextual toolbar out of the way? Can we leave a comment? Is there another way we can "deselect" the block to have a stronger guarantee the toolbar isn't there?
There was a problem hiding this comment.
It's to select the first paragraph since we're testing if the selection moves to the second.
I don't think that's possible. We're not styling the paragraph itself but the areas between the paragraphs. We can fine tune the cursor style later. Alternatively, we can make it the same as the one for a button. The flicker you see is, I think, caused by the inserter appearing after a small delay. |
| opacity: 0; | ||
| pointer-events: none; |
There was a problem hiding this comment.
I've noticed one negative impact of this style: When trying to scroll the block list, if the cursor happens to be placed over the sibling inserter when using the scroll wheel, the scroll will not happen.
There was a problem hiding this comment.
Hm, that's not good. :)
There was a problem hiding this comment.
Hm, it seems to be happening before this change as well.
There was a problem hiding this comment.
The problem is that the pointer is over a Popover, which won't scroll the page.
There was a problem hiding this comment.
I notice the same thing with the block toolbar. It's not possible to scroll with the pointer over it.
There was a problem hiding this comment.
The strange thing is that it is rendered in the scroll container.
There was a problem hiding this comment.
Is there anything in the popover rendering that would be preventing the scroll? pointer-events or event.preventDefault ?
There was a problem hiding this comment.
It's not immediately obvious to me. I commented out all pointer-events rules and it's still not scrolling. Also no scroll preventing I think.
There was a problem hiding this comment.
The strange thing is that it is rendered in the scroll container.
In some further debugging:
- It is part of the container where scrolling is applied (
.edit-post-editor-regions__content) - But it's not part of the content which is causing the scroll (
edit-post-visual-editor)
It's not immediately clear to me what the expected behavior here would be, but this could explain why the scroll doesn't happen.
There was a problem hiding this comment.
I found the issue. The popover container's position is fixed which means it's positioned relative to the viewport. Relatively positioning the scroll container won't help unless we absolutely position the popover. It sounds like this will need some adjustment in the Popover component.
Description
See #19719. This PR fixes the visible inserter tooltip from the inserter above a selected block. It also adds e2e tests for the whole feature.
How has this been tested?
Screenshots
Types of changes
Checklist: