Input Interaction: take into account container padding

[ellatrix]

Description

Fixes #14702. We need to take into account container padding to see if the caret is at the vertical edge of the container. This is relevant for e.g. paragraphs with a background color.

Why does it work in some themes? In some cases the padding may be small enough to get ignored with the buffer we have added.

How has this been tested?

Ensure cross block navigation works with a paragraph with a background color in e.g. Twenty Fifteen.

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.

[ellatrix]

Added an e2e test. Thanks to the test I discovered a flaw in in my fix. :)

[ellatrix]

Again (different) intermittent failures:

Summary of all failing tests
FAIL specs/block-deletion.test.js (16.674s)
  ● block deletion - › deleting the third block using the Remove Block menu item › results in two remaining blocks and positions the caret at the end of the second block
    expect(value).toMatchSnapshot()
    Received value does not match stored snapshot "block deletion - deleting the third block using the Remove Block menu item results in two remaining blocks and positions the caret at the end of the second block 1".
    - Snapshot
    + Received
    @@ -2,6 +2,8 @@
      <p>First paragraph</p>
      <!-- /wp:paragraph -->
      
      <!-- wp:paragraph -->
      <p>Second paragraph</p>
    - <!-- /wp:paragraph -->"
    + <!-- /wp:paragraph -->
    + 
    + <!-- wp:block {"ref":"reusable1"} /-->"
      39 | 
      40 | 			await clickOnBlockSettingsMenuItem( 'Remove Block' );
    > 41 | 			expect( await getEditedPostContent() ).toMatchSnapshot();
         | 			                                       ^
      42 | 
      43 | 			// Type additional text and assert that caret position is correct by comparing to snapshot.
      44 | 			await page.keyboard.type( ' - caret was here' );
      at Object.toMatchSnapshot (specs/block-deletion.test.js:41:43)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (specs/block-deletion.test.js:9:103)
      at _next (specs/block-deletion.test.js:11:194)
  ● block deletion - › deleting the third block using the Remove Block menu item › results in two remaining blocks and positions the caret at the end of the second block
    expect(value).toMatchSnapshot()
    Received value does not match stored snapshot "block deletion - deleting the third block using the Remove Block menu item results in two remaining blocks and positions the caret at the end of the second block 2".
    - Snapshot
    + Received
      "<!-- wp:paragraph -->
      <p>First paragraph</p>
      <!-- /wp:paragraph -->
      
      <!-- wp:paragraph -->
    - <p>Second paragraph - caret was here</p>
    - <!-- /wp:paragraph -->"
    + <p>Second paragraph</p>
    + <!-- /wp:paragraph -->
    + 
    + <!-- wp:block {"ref":126} /-->"
      43 | 			// Type additional text and assert that caret position is correct by comparing to snapshot.
      44 | 			await page.keyboard.type( ' - caret was here' );
    > 45 | 			expect( await getEditedPostContent() ).toMatchSnapshot();
         | 			                                       ^
      46 | 		} );
      47 | 	} );
      48 | 
      at Object.toMatchSnapshot (specs/block-deletion.test.js:45:43)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (specs/block-deletion.test.js:9:103)
      at _next (specs/block-deletion.test.js:11:194)
Snapshot Summary
 › 2 snapshots failed from 1 test suite. Inspect your code changes or run `npm run test-e2e -- -u` to update them.
Test Suites: 1 failed, 34 passed, 35 total
Tests:       1 failed, 1 skipped, 175 passed, 177 total
Snapshots:   2 failed, 124 passed, 126 total
Time:        423.968s, estimated 434s

[ellatrix]

The intermittent failures seems to be related to reusable blocks (perhaps not being removed synchronously). Cc @noisysocks @aduth.

[aduth]

There seem to be a lot more intermittent failures lately, and for many assorted tests rather than a few specific. It does make me wonder if there is some asynchronous behavior introduced somewhere in common interactions, as most of the tests I see failing and their corresponding functionality haven't otherwise been modified recently (including this reusable blocks failure).

[ellatrix]

@aduth That may be. I've seen these two in particular fail several times in other PRs. It's hard to guess what's going on. :/

[aduth]

Odd; testing this, I see some improvement in that I'm not longer "stuck" in the colored paragraph block, but it sometimes (not always) takes two ArrowUp or ArrowDown to escape from the block.

Were you ever able to reproduce the original issue? I can reproduce with the Twentynineteen theme, which you mentioned to be using.

I'll try to debug a bit as well...

[aduth]

In my testing, the issue seems to occur as a result of rangeRect being zero'd for all values.

I feel like I'd seen this before as well, and tracking it back to getRectangleFromRange and how we conditionally consider collapsed ranges, and perhaps something with the ZWSP we also-only-sometimes insert. I recall getting some stability improvements (unsure at what expense) with normalizing this behavior in some way (removing the ! range.collapsed condition? always inserting the ZWSP ?)

[ellatrix]

@aduth Thanks, I'll have a closer look soon.

[ellatrix]

@aduth It seems like in the case you're mentioned above, where you have to press twice, the ZWNBSP is being inserted. The problem is that it gets inserted at the right edge of a line break element, so the ZWNBSP will visually end up on the next line. It seems to be a limitation of this hack. I'll think about how to solve it. In the meantime, I do think this fixes the original issue?

[ellatrix]

@aduth I'm sometimes starting to think that we might be better off using a ZWNBSP as padding for an empty rich text area (and padding if it ends in a line break). Element line breaks bring a lot of bugs with them. Anyway, I think this might be better solved separately.

[ellatrix]

The e2e test shows that this PR does fix an issue. If you run it against master, it fails.

[ellatrix]

@aduth I fixed the other issue in #14846.

[aduth]

Considering ZWSP as a separate issue, this makes sense on its own to account for the padding.

[ellatrix]

Thanks!