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

RichText: fix RTL keyboard interactions #15496

Merged
merged 26 commits into from
May 14, 2019
Merged

RichText: fix RTL keyboard interactions #15496

merged 26 commits into from
May 14, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 8, 2019

Description

Fixes https://core.trac.wordpress.org/ticket/47180.
Fixes arrow left/right navigation and backspace/delete at the edges of rich text areas.
Fixes some boundary issues #15466 (review).

  • In RichText, I removed some left/right key handling. We should let the browser do its thing and only interfere at boundaries.
  • I had the rework some boundary code, so coincidentally it fixes 2 issues where the boundary attribute would disappear on left/right key and on initial focus.
  • In isHorizontalEdge, the horizontal checks need to reversed in RTL.
  • In WritingFlow, the left/right arrow key handling needs to be reversed as right means previous and left means next.
  • I added e2e tests and made sure they fail in master.

Any other broken interactions?

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.

closes #15553 and closes #15364

@ellatrix ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label May 8, 2019
@youknowriad
Copy link
Contributor

Just tested the RichText interactions:

  • arrows work well
  • backspace and del work as expected

But when it comes to cross-block interactions (WritingFlow?), both arrow keys and backspace/del are still broken:

  • The caret is not put at the right place
  • del at the end should merge blocks
  • backspace at the beginning should merge blocks

@ellatrix
Copy link
Member Author

ellatrix commented May 8, 2019

@youknowriad I think I fixed the remaining issues.

Something else I'm seeing is e.g. left/right keys not working correctly in toolbars. Not sure if I should fix here.

@ellatrix
Copy link
Member Author

ellatrix commented May 8, 2019

@youknowriad How is it looking now? :)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

The issues seem fixed to me. Thank @ellatrix

We should explore how to e2e test RTL languages too

@ellatrix
Copy link
Member Author

ellatrix commented May 8, 2019

Whooo :) Working on the e2e tests now, but it's not easy 😅

@youknowriad
Copy link
Contributor

@ellatrix I actually just noticed a bug:

  • Type "shift + enter" at the end of a paragraph
  • type "Backspace"

Instead of removing the "br", it tries to merge with the previous block

@youknowriad youknowriad added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Type] Bug An existing feature does not function as intended labels May 8, 2019
@ellatrix
Copy link
Member Author

ellatrix commented May 9, 2019

Added e2e tests. Looking at the remaining issue now.

@ellatrix ellatrix requested a review from oandregal as a code owner May 9, 2019 10:19
@ellatrix
Copy link
Member Author

ellatrix commented May 9, 2019

All e2e test pass for me locally. Not sure what is going on.

@youknowriad I think everything should work fine now, including the edge case you mentioned.

@youknowriad
Copy link
Contributor

There are some failing tests, I don't know if it's legit.
I confirm that in my testing all the RTL issues are fixed.

@ellatrix
Copy link
Member Author

ellatrix commented May 9, 2019

Yeah, fixed a few already. There's one final failing annotation test.

@ellatrix ellatrix force-pushed the try/fix-rich-text-rtl branch 4 times, most recently from 77b1106 to de2541e Compare May 10, 2019 09:48
@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 14, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Just tested this again. In terms of behavior, it still seems to work as intended.

@ellatrix ellatrix merged commit c64faf4 into master May 14, 2019
@ellatrix ellatrix deleted the try/fix-rich-text-rtl branch August 15, 2019 11:18
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 13, 2019
@@ -378,7 +368,20 @@ export class RichText extends Component {
}

this.recalculateBoundaryStyle();
this.onSelectionChange();

// We know for certain that on focus, the old selection is invalid. It
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's not worth worrying about, but: There are cases where a focus event occurs, but the selection remains the same. Notably, when the tab loses or regains focus (becomes the active tab again). I notice it most when debugging, to add a breakpoint in a code path which triggers on a change to state. The act of focusing DevTools and changing back to the window is enough to cause a change in state, so the breakpoint will fire.

Perhaps it's not a big deal, but could we:

  • Avoid calling onSelectionChange if the selection indices are the same? Perhaps also avoid setState as well.
  • Update the reducer for selectionStart and selectionEnd to avoid creating new object references if the updated offsets are the same as the current state value?

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth onSelectionChange already avoids setting state if the selection is the same. I guess the problem here is that it's set to undefined?

Copy link
Member

Choose a reason for hiding this comment

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

@aduth onSelectionChange already avoids setting state if the selection is the same

Where does this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here:

if ( start === value.start && end === value.end ) {
// Sometimes the browser may set the selection on the placeholder
// element, in which case the caret is not visible. We need to set
// the caret before the placeholder if that's the case.
if ( value.text.length === 0 && start === 0 ) {
fixPlaceholderSelection();
}
return;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants