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

Fix: Make ⌘A's select all the blocks again #7731

Merged
merged 2 commits into from
Jul 6, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jul 5, 2018

Description

Fixes: #7711
Regressed in: #7644

Moves the logic to handle meta+a to be before the check for event.nativeEvent.defaultPrevented. The event was default prevented by TinyMCE.

How has this been tested?

Some smoke testing.
Added multiple paragraphs with text and checked that pressing ⌘A's multiple selected all the blocks.

@jorgefilipecosta jorgefilipecosta requested a review from a team July 5, 2018 16:00
@@ -190,7 +190,7 @@ class WritingFlow extends Component {

// Aobrt if navigation has already been handled (e.g. TinyMCE inline
Copy link
Member

Choose a reason for hiding this comment

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

There's a typo here "Aobrt"

@aduth
Copy link
Member

aduth commented Jul 5, 2018

End-to-end test failure is legitimate. This is basically undoing what was meant to be fixed by #7644 / #6712

aduth
aduth previously requested changes Jul 5, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Re-regresses #7644 . _navigationHandled isn't a thing. It's basically the same as if the condition isn't there at all.

@afercia
Copy link
Contributor

afercia commented Jul 5, 2018

Also related: #7445

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Consecutive-A-do-not-work-anymore branch 3 times, most recently from f51f5b8 to 42cfffe Compare July 5, 2018 17:02
@jorgefilipecosta
Copy link
Member Author

This PR was updated it now reorders the check for ⌘A event to be before the check for event.nativeEvent.defaultPrevented. It seems TinyMCE applies a prevent default in the event.

@aduth
Copy link
Member

aduth commented Jul 5, 2018

Future tasks:

  • Split WritingFlow#onKeyDown into more sensible isolated function handlers (we shouldn't have both block traversal and Cmd+A behaviors in the same function, it contributed to the introduction of the regression)
  • Try to interoperate better with TinyMCE's preventDefault, e.g. perhaps exploring the SelectAll command which is the source of the preventDefault, either stripping the default TinyMCE behavior, refactoring our 2x Cmd+A to act in response to the SelectAll command (similar to Undo/Redo propagation), or abandoning effort to use native preventDefault in favor of an implementation-specific property like which had existed prior to Rich Text: Detect handled horizontal navigation by preventDefault #7644

On glance, this looks good. Will take a closer look soon. Travis appears to be having a bad day.

@jorgefilipecosta jorgefilipecosta added this to the 3.2 milestone Jul 5, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jul 5, 2018
@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Jul 5, 2018
} else if ( ! this.verticalRect ) {
this.verticalRect = computeCaretRect( target );
}

if ( ! isNav ) {
Copy link
Member

Choose a reason for hiding this comment

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

Worth leaving a comment on the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well remembered, the comment was added.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Consecutive-A-do-not-work-anymore branch from 42cfffe to 2bb7efe Compare July 6, 2018 10:07
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Added an e2e test to ensure we don't break it in the future. Arrow nav still works as expected.

@jorgefilipecosta jorgefilipecosta dismissed aduth’s stale review July 6, 2018 10:30

The PR was updated to use another approach.

@jorgefilipecosta jorgefilipecosta merged commit c33bb2a into master Jul 6, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/Consecutive-A-do-not-work-anymore branch July 6, 2018 10:31
@jorgefilipecosta
Copy link
Member Author

Thank you for the review and for suggesting this approach @aduth. Thank you @iseulde for the review and for the end to end test 👍

@aduth
Copy link
Member

aduth commented Jul 6, 2018

Thanks for the added end-to-end test!

@mcsf
Copy link
Contributor

mcsf commented Oct 15, 2018

Just noting that, at least as of today:

  • Firefox requires three ⌘A presses.
  • Safari doesn't seem to care about this feature at all.

@mcsf
Copy link
Contributor

mcsf commented Oct 15, 2018

I see the above is sort of tracked in #8180, so disregard. :)

@designsimply designsimply added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants