Closed Bug 1563206 Opened 5 years ago Closed 5 years ago

Add temporary support for native key events for gutenberg

Tracking

(firefox71 fixed)

Status:

RESOLVED FIXED

Tracking Flags:

Tracking

Status

firefox71

---

fixed

People

(Reporter: jdescottes, Assigned: impossibus)

References

Details

Attachments

(2 files)

Bug 1563206 - Test top key combinations that affect gutenberg; r=#remote-protocol-reviewers 5 years ago Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1563206 - Call editor commands directly to emulate some native key bindings; r=ato 5 years ago Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) 47 bytes, text/x-phabricator-request		Details \| Review

Julian Descottes [:jdescottes]

Reporter

Description

•

5 years ago

See Bug 1563205 for the overall issue.

In scope of the gutenberg test suite support, we will identify a shortlist of shortcuts that need to be supported, and will implement them directly in our Input domain to increase coverage.

The goal of this bug is just to be a temporary solution to fix gutenberg tests until EventUtils is improved in Bug 1563205.

(if by any chance Bug 1563205 is resolved sooner than this one, then we should close it as a duplicate, but we assume Bug 1563205 will be non-trivial)

Julian Descottes [:jdescottes]

Reporter

Comment 1

•

5 years ago

Check how many tests are impacted

Flags: needinfo?(jdescottes)

Priority: P2 → P3

David Burns :automatedtester

Comment 2

•

5 years ago

Julian, are you planning on doing this bug or can I reassign it?

Julian Descottes [:jdescottes]

Reporter

Comment 3

•

5 years ago

Hi David, feel free to reassign!

Flags: needinfo?(jdescottes)

David Burns :automatedtester

Updated

•

5 years ago

Assignee: nobody → mjzffr

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Comment 4

•

5 years ago

It's worth noting that Puppeteer doesn't emulate native shortcuts (i.e. shortcuts handled by OS, not Chrome). The gutenberg tests are working around this for Cmd+A by synthesizing the necessary events with EventTarget.dispatchEvent

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Comment 5

•

5 years ago

gutenberg tests use a helper, pressKeyWithModifier, to dispatch shortcuts, and the first parameter is often a generic alias for common combinations on different platforms:

    primary: ( _isApple ) => _isApple() ? [ COMMAND ] : [ CTRL ],
    primaryShift: ( _isApple ) => _isApple() ? [ SHIFT, COMMAND ] : [ CTRL, SHIFT ],
    primaryAlt: ( _isApple ) => _isApple() ? [ ALT, COMMAND ] : [ CTRL, ALT ],
    secondary: ( _isApple ) => _isApple() ? [ SHIFT, ALT, COMMAND ] : [ CTRL, SHIFT, ALT ],
    access: ( _isApple ) => _isApple() ? [ CTRL, ALT ] : [ SHIFT, ALT ],

Affected tests

Ignoring primary+A which doesn't touch CDP (see previous comment)

git grep -H pressKeyWithModifier | grep await | grep -v "\'primary\', \'a\'" | sort | uniq

packages/e2e-tests/specs/a11y.test.js:      await pressKeyWithModifier( 'access', 'h' );
packages/e2e-tests/specs/a11y.test.js:      await pressKeyWithModifier( 'ctrl', '~' );
packages/e2e-tests/specs/a11y.test.js:      await pressKeyWithModifier( 'shift', 'Tab' );
packages/e2e-tests/specs/block-deletion.test.js:            await pressKeyWithModifier( 'access', 'z' );
packages/e2e-tests/specs/block-deletion.test.js:            await pressKeyWithModifier( 'shift', 'ArrowUp' );
packages/e2e-tests/specs/block-hierarchy-navigation.test.js:        await pressKeyWithModifier( 'ctrl', '`' );
packages/e2e-tests/specs/block-hierarchy-navigation.test.js:    await pressKeyWithModifier( 'access', 'o' );
packages/e2e-tests/specs/block-switcher.test.js:        await pressKeyWithModifier( 'alt', 'F10' );
packages/e2e-tests/specs/blocks/classic.test.js:        await pressKeyWithModifier( 'shift', 'Tab' );
packages/e2e-tests/specs/blocks/heading.test.js:        await pressKeyWithModifier( 'primary', 'A' );
packages/e2e-tests/specs/blocks/list.test.js:       await pressKeyWithModifier( 'primary', 'm' );
packages/e2e-tests/specs/blocks/list.test.js:       await pressKeyWithModifier( 'primary', 'z' );
packages/e2e-tests/specs/blocks/list.test.js:       await pressKeyWithModifier( 'primaryShift', 'm' );
packages/e2e-tests/specs/blocks/list.test.js:       await pressKeyWithModifier( 'shift', 'Enter' );
packages/e2e-tests/specs/change-detection.test.js:      await pressKeyWithModifier( 'primary', 'S' );
packages/e2e-tests/specs/keyboard-navigable-blocks.test.js: await pressKeyWithModifier( 'ctrl', '`' );
packages/e2e-tests/specs/links.test.js:     await pressKeyWithModifier( 'primary', 'K' );
packages/e2e-tests/specs/links.test.js:     await pressKeyWithModifier( 'primary', 'k' );
packages/e2e-tests/specs/links.test.js:     await pressKeyWithModifier( 'shiftAlt', 'ArrowLeft' );
packages/e2e-tests/specs/multi-block-selection.test.js:     await pressKeyWithModifier( 'shift', 'ArrowDown' );
packages/e2e-tests/specs/multi-block-selection.test.js:     await pressKeyWithModifier( 'shift', 'ArrowRight' );
packages/e2e-tests/specs/multi-block-selection.test.js:     await pressKeyWithModifier( 'shift', 'ArrowUp' );
packages/e2e-tests/specs/multi-block-selection.test.js:     await pressKeyWithModifier( 'shift', 'Enter' );
packages/e2e-tests/specs/navigable-toolbar.test.js:         await pressKeyWithModifier( 'alt', 'F10' );
packages/e2e-tests/specs/plugins/block-icons.test.js:   await pressKeyWithModifier( 'access', 'o' );
packages/e2e-tests/specs/plugins/deprecated-node-matcher.test.js:       await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/plugins/format-api.test.js:        await pressKeyWithModifier( 'shiftAlt', 'ArrowLeft' );
packages/e2e-tests/specs/plugins/templates.test.js:         await pressKeyWithModifier( 'primary', 'A' );
packages/e2e-tests/specs/preview.test.js:       await pressKeyWithModifier( 'shift', 'Home' );
packages/e2e-tests/specs/publish-panel.test.js:     await pressKeyWithModifier( 'shift', 'Tab' );
packages/e2e-tests/specs/rich-text.test.js:     await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/rich-text.test.js:     await pressKeyWithModifier( 'primary', 'i' );
packages/e2e-tests/specs/rich-text.test.js:     await pressKeyWithModifier( 'primary', 'z' );
packages/e2e-tests/specs/rich-text.test.js:     await pressKeyWithModifier( 'shift', 'ArrowLeft' );
packages/e2e-tests/specs/rich-text.test.js:     await pressKeyWithModifier( 'shift', 'Tab' );
packages/e2e-tests/specs/rtl.test.js:       await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/rtl.test.js:       await pressKeyWithModifier( 'shift', 'Enter' );
packages/e2e-tests/specs/shortcut-help.test.js:     await pressKeyWithModifier( 'access', 'h' );
packages/e2e-tests/specs/sidebar.test.js:       await pressKeyWithModifier( 'ctrl', '`' );
packages/e2e-tests/specs/splitting-merging.test.js:     await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/splitting-merging.test.js:     await pressKeyWithModifier( 'primary', 'z' );
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' );
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 1st block.
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 1st paragraph text.
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 2nd block.
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 2nd paragraph text.
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 3rd block.
packages/e2e-tests/specs/undo.test.js:      await pressKeyWithModifier( 'primary', 'z' ); // Undo 3rd paragraph text.
packages/e2e-tests/specs/writing-flow.test.js:          await pressKeyWithModifier( 'alt', 'Backspace' );
packages/e2e-tests/specs/writing-flow.test.js:          await pressKeyWithModifier( 'primary', 'Backspace' );
packages/e2e-tests/specs/writing-flow.test.js:      await pressKeyWithModifier( 'primary', 'ArrowDown' );
packages/e2e-tests/specs/writing-flow.test.js:      await pressKeyWithModifier( 'primary', 'b' );
packages/e2e-tests/specs/writing-flow.test.js:      await pressKeyWithModifier( 'primary', 'i' );
packages/e2e-tests/specs/writing-flow.test.js:      await pressKeyWithModifier( 'shift', 'Enter' );

Most common shortcuts

git grep pressKey | grep await | cut -d ":" -f 2 | sed -e 's/^[[:space:]]*//' | sort | uniq -c | sort -r

  28 await pressKeyWithModifier( 'primary', 'a' ); # doesn't use CDP
  27 await pressKeyWithModifier( 'primary', 'b' );
  18 await pressKeyWithModifier( 'shift', 'Enter' );
  16 await pressKeyWithModifier( 'shiftAlt', 'ArrowLeft' );
  10 await pressKeyWithModifier( 'ctrl', '`' );
   9 await pressKeyWithModifier( 'primary', 'K' );
   7 await pressKeyWithModifier( 'primary', 'm' );
   6 await pressKeyWithModifier( 'primary', 'z' );
   6 await pressKeyWithModifier( 'primary', 'S' );
...

If you look at the log of CDP method calls in the test log (run on macos), you get a slightly different picture of top shortcuts:

29 Shift + ArrowLeft ( "type":"rawKeyDown","modifiers":8,"windowsVirtualKeyCode":37,"code":"ArrowLeft","key":"ArrowLeft","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
27 Meta + b ("type":"rawKeyDown","modifiers":4,"windowsVirtualKeyCode":66,"code":"KeyB","key":"b","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
19 Shift + Enter ("type":"keyDown","modifiers":8,"windowsVirtualKeyCode":13,"code":"Enter","key":"Enter","text":"\r","unmodifiedText":"\r","autoRepeat":false,"location":0,"isKeypad":false)
16 Alt + Shift + ArrowLeft ("type":"rawKeyDown","modifiers":9,"windowsVirtualKeyCode":37,"code":"ArrowLeft","key":"ArrowLeft","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
12 Meta z ("type":"rawKeyDown","modifiers":4,"windowsVirtualKeyCode":90,"code":"KeyZ","key":"z","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
12 Ctrl + BackQuote ("type":"rawKeyDown","modifiers":2,"windowsVirtualKeyCode":192,"code":"Backquote","key":"`","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
11 Meta K or k ("type":"rawKeyDown","modifiers":4,"windowsVirtualKeyCode":75,"code":"KeyK","key":"K","text":"","unmodifiedText":"","autoRepeat":false,"location":0,"isKeypad":false)
...

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Comment 6

•

5 years ago

I've taken the liberty to investigate related issues while working on this. One conclusion is that the temporary solution described in Comment 0 won't be necessary to increase coverage in gutenberg: as far as I can tell, most (if any) tests are not affected by problematic shortcuts.

So far, the patch for this is a whole bunch of tests, and maybe a "temporary solution" for one key combination. I'll clean that up and push it after the weekend.

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Comment 7

•

5 years ago

Attached file Bug 1563206 - Test top key combinations that affect gutenberg; r=#remote-protocol-reviewers — Details

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Comment 8

•

5 years ago

Attached file Bug 1563206 - Call editor commands directly to emulate some native key bindings; r=ato — Details

Depends on D47032

Phabricator Automation

Updated

•

5 years ago

Attachment #9095059 - Attachment description: Bug 1563206 - Test top key combinations that affect gutenberg; r=ato → Bug 1563206 - Test top key combinations that affect gutenberg; r=#remote-protocol-reviewers

Pulsebot

Comment 9

•

5 years ago

Pushed by mjzffr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1ce1b20dcc20 Test top key combinations that affect gutenberg; r=ato,remote-protocol-reviewers,jdescottes

Oana Pop-Rus

Comment 10

•

5 years ago

Backed out changeset 1ce1b20dcc20 (bug 1563206) for mochitest remote perma failures in browser_input_dispatchKeyEvent.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=271922007&resultStatus=pending%2Crunning%2Csuccess%2Csuperseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Ctest-linux64-asan%2Fopt-mochitest-remote-e10s%2Cm%28remote%29&revision=1ce1b20dcc2015fb6a4a244bb851cafec84bb184

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271922007&repo=autoland&lineNumber=1701

Backout: https://hg.mozilla.org/integration/autoland/rev/b92c29aab7c6442c3886adef72b301e31c07a6b2

Flags: needinfo?(mjzffr)

Pulsebot

Comment 11

•

5 years ago

Pushed by mjzffr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/67c1068e06d4 Test top key combinations that affect gutenberg; r=ato,remote-protocol-reviewers,jdescottes https://hg.mozilla.org/integration/autoland/rev/63cc54f12356 Call editor commands directly to emulate some native key bindings; r=ato,remote-protocol-reviewers

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Assignee

Updated

•

5 years ago

Flags: needinfo?(mjzffr)

Arthur Iakab [arthur_iakab]

Comment 12

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/67c1068e06d4
https://hg.mozilla.org/mozilla-central/rev/63cc54f12356

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox71: --- → fixed

Resolution: --- → FIXED

Alexandru Michis [:malexandru]

Updated

•

5 years ago

Regressions: 1589844

Nobody; OK to take it and work on it

Updated

•

4 years ago

Component: CDP: Input → CDP

You need to log in before you can comment on or make changes to this bug.