Keyboard navigation for media editor #2108

wassgha · 2020-05-29T07:54:02Z

Summary

Adds ability to move the cropped image/video using keyboard arrows.

To-do

~~Fix warning while setting framesLayer ref~~
~~Restore keyboard events for dragging after finishing (otherwise dragging by keyboard will be disabled)~~
Refactor code to prevent duplication between drag callbacks and keyboard callbacks
Add integration testing

Testing Instructions

Insert a media element
Double click on the element
Resize the crop window to be smaller than the original element
Use keyboard arrows to move the original element
Press tab or click on the image size slider to focus it
Press left-right arrow keys to change size
Press image again (or press shift+tab) to return focus to image
Press keyboard arrows to move the original element

Fixes #682

codecov · 2020-05-29T07:56:05Z

Codecov Report

Merging #2108 into main will increase coverage by 0.48%.
The diff coverage is 92.10%.

@@            Coverage Diff             @@
##             main    #2108      +/-   ##
==========================================
+ Coverage   82.87%   83.35%   +0.48%     
==========================================
  Files         830      831       +1     
  Lines       14498    14521      +23     
==========================================
+ Hits        12015    12104      +89     
+ Misses       2483     2417      -66

Flag	Coverage Δ
#karmatests	`71.40% <92.10%> (+0.62%)`	⬆️
#unittests	`66.09% <13.15%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../src/edit-story/components/canvas/useCanvasKeys.js	`77.04% <80.00%> (-3.55%)`	⬇️
...s/src/edit-story/elements/media/editPanMoveable.js	`88.57% <85.71%> (+84.40%)`	⬆️
assets/src/edit-story/elements/media/edit.js	`100.00% <100.00%> (+71.42%)`	⬆️
assets/src/edit-story/elements/media/scalePanel.js	`100.00% <100.00%> (+56.25%)`	⬆️
assets/src/edit-story/utils/getKeyboardMovement.js	`100.00% <100.00%> (ø)`
assets/src/edit-story/elements/shared/index.js	`100.00% <0.00%> (+4.16%)`	⬆️
...ets/src/edit-story/app/media/pagination/reducer.js	`50.00% <0.00%> (+5.00%)`	⬆️
assets/src/edit-story/utils/useDoubleClick.js	`96.00% <0.00%> (+8.00%)`	⬆️
assets/src/edit-story/elements/media/index.js	`100.00% <0.00%> (+9.09%)`	⬆️
... and 7 more

github-actions · 2020-05-29T07:58:18Z

Size Change: +490 B (0%)

Total Size: 1.19 MB

Filename	Size	Change
`assets/js/edit-story.js`	490 kB	+207 B (0%)
`assets/js/stories-dashboard.js`	537 kB	+283 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	268 B	0 B
`assets/css/stories-dashboard.css`	305 B	0 B
`assets/css/web-stories-embed-block.css`	515 B	0 B
`assets/js/chunk-web-stories-template-0-********************.js`	10.2 kB	0 B
`assets/js/chunk-web-stories-template-1-********************.js`	10.3 kB	0 B
`assets/js/chunk-web-stories-template-2-********************.js`	10 kB	0 B
`assets/js/chunk-web-stories-template-3-********************.js`	10.5 kB	0 B
`assets/js/chunk-web-stories-template-4-********************.js`	10.6 kB	0 B
`assets/js/chunk-web-stories-template-5-********************.js`	6.86 kB	0 B
`assets/js/chunk-web-stories-template-6-********************.js`	9.91 kB	0 B
`assets/js/chunk-web-stories-template-7-********************.js`	9.78 kB	0 B
`assets/js/web-stories-activation-notice.js`	63.6 kB	0 B
`assets/js/web-stories-embed-block.js`	16.9 kB	0 B

_{compressed-size-action}

assets/src/edit-story/elements/media/editPanMovable.js

barklund · 2020-07-20T15:04:06Z

So this PR fixes #1426 or?

And why is #1426 closed as a duplicate of itself by the way? 😮

# Conflicts: # assets/src/edit-story/components/canvas/canvasProvider.js # assets/src/edit-story/components/canvas/framesLayer.js # assets/src/edit-story/elements/media/editPanMoveable.js

google-cla · 2020-08-19T20:08:16Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

assets/src/edit-story/elements/media/editPanMoveable.js

barklund · 2020-08-19T20:09:57Z

@googlebot I consent.

miina · 2020-08-20T09:31:29Z

From testing:

Otherwise works great, just one detail: there doesn't seem to be a visual difference if the keyboard focus is on the image or not -- e.g. using Tab to move to the slider and then back to the image doesn't make a visual difference for the image outline. Perhaps that can be deferred to the future though.

miina · 2020-08-20T09:42:58Z

One more from testing:
While in edit mode and focusing away, e.g. clicking somewhere on the design panel, and then using arrow keys, the whole element is moved up/down even though it's in the edit mode. Should in edit mode the panned image move instead of the whole element?

merapi · 2020-08-20T09:49:50Z

@barklund @miina Can you confirm this issue on Safari?
When you enter the edit mode and press TAB it should focus on the slider but it doesn't work for me (I can also end up in a state where arrows move the thole element with edit mode).

miina · 2020-08-20T09:57:25Z

assets/src/edit-story/elements/media/editPanMoveable.js

+      }
+      const panFocalX = getFocalFromOffset(width, mediaWidth, offsetX - dx);
+      const panFocalY = getFocalFromOffset(height, mediaHeight, offsetY - dy);
+      setProperties({


It seems to lag currently when not clicking the keys slowly -- would be good to create a follow-up issue for looking into that.

Yeah, there's a lot of lag currently. It's beginning to be a problem 😢

miina · 2020-08-20T10:18:01Z

When you enter the edit mode and press TAB it should focus on the slider but it doesn't work for me

@merapi Confirmed -- indeed that focuses on the design panel items instead.

assets/src/edit-story/components/canvas/useCanvasKeys.js

assets/src/edit-story/elements/media/edit.js

barklund · 2020-08-20T16:05:34Z

@miina, @merapi, I addressed your comments inline. Please see these two threads on disabling moving editmode elements at all and focus management.

miina

Unit tests are failing currently, otherwise LGTM (let's create a performance-related follow-up issue, too).

merapi · 2020-08-21T10:04:07Z

Oh, and regarding the fact that in Safari you cannot tab from the image to the slider, that is due to Safari's default of not actually using tab to navigate elements. You have to enable that in settings (which I assume all keyboard-using Safari users do):

Ok, that works, how about this one? (maybe it will surface in other places):
on every other browser when you change the zoom (via mouse), the slider becomes focused (blue), on Safari - it's not (you can't adjust zoom via keyboard after that).

Add key handler for editor panning

0f2de85

googlebot added the cla: yes label May 29, 2020

wassgha commented May 29, 2020

View reviewed changes

assets/src/edit-story/elements/media/editPanMovable.js Outdated Show resolved Hide resolved

wassgha mentioned this pull request Jun 4, 2020

Arrow keys in edit mode move the outer element, not the mask #1426

Closed

swissspidy changed the base branch from master to main July 20, 2020 10:22

swissspidy added the Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Aug 15, 2020

Merge branch 'main' into image-editor/keyboard-nav

c987b3d

# Conflicts: # assets/src/edit-story/components/canvas/canvasProvider.js # assets/src/edit-story/components/canvas/framesLayer.js # assets/src/edit-story/elements/media/editPanMoveable.js

google-cla bot added cla: no and removed cla: yes labels Aug 19, 2020

barklund reviewed Aug 19, 2020

View reviewed changes

assets/src/edit-story/elements/media/editPanMoveable.js Outdated Show resolved Hide resolved

google-cla bot added cla: yes and removed cla: no labels Aug 19, 2020

Generalized some shared logic

347bde7

barklund force-pushed the image-editor/keyboard-nav branch from bef1f3b to 347bde7 Compare August 19, 2020 20:18

barklund removed the Status: Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Aug 19, 2020

Morten Barklund added 3 commits August 19, 2020 21:52

Added karma tests for image editmode using both mouse and keyboard

efbab30

Added resetting to karma test

c856561

Fixed scale panel so "enter" activates

fb68c88

barklund marked this pull request as ready for review August 20, 2020 02:54

barklund requested review from miina and merapi August 20, 2020 02:54

miina reviewed Aug 20, 2020

View reviewed changes

swissspidy added Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Elements: Image Elements: Video Group: Media Type: Enhancement New feature or improvement of an existing feature labels Aug 20, 2020

barklund self-assigned this Aug 20, 2020

Morten Barklund added 2 commits August 20, 2020 11:46

Merge branch 'main' into image-editor/keyboard-nav

596361e

Disable moving elements by keyboard when editing

a9cb9a4

barklund reviewed Aug 20, 2020

View reviewed changes

assets/src/edit-story/components/canvas/useCanvasKeys.js Show resolved Hide resolved

barklund reviewed Aug 20, 2020

View reviewed changes

assets/src/edit-story/elements/media/edit.js Show resolved Hide resolved

Fixed reference to element for keyboard access

5b7031e

barklund requested a review from miina August 20, 2020 16:08

miina approved these changes Aug 20, 2020

View reviewed changes

Fixed unit test

bdd0d6a

barklund assigned csossi and unassigned csossi Aug 20, 2020

Merge branch 'main' into image-editor/keyboard-nav

4b19377

barklund merged commit f805c1e into main Aug 31, 2020

barklund deleted the image-editor/keyboard-nav branch August 31, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard navigation for media editor #2108

Keyboard navigation for media editor #2108

wassgha commented May 29, 2020 •

edited by barklund

codecov bot commented May 29, 2020 •

edited

github-actions bot commented May 29, 2020 •

edited

barklund commented Jul 20, 2020

google-cla bot commented Aug 19, 2020

barklund commented Aug 19, 2020

miina commented Aug 20, 2020

miina commented Aug 20, 2020

merapi commented Aug 20, 2020

miina Aug 20, 2020

barklund Aug 20, 2020 •

edited

miina commented Aug 20, 2020

barklund commented Aug 20, 2020

miina left a comment

merapi commented Aug 21, 2020 •

edited

Keyboard navigation for media editor #2108

Keyboard navigation for media editor #2108

Conversation

wassgha commented May 29, 2020 • edited by barklund

Summary

To-do

Testing Instructions

codecov bot commented May 29, 2020 • edited

Codecov Report

github-actions bot commented May 29, 2020 • edited

barklund commented Jul 20, 2020

google-cla bot commented Aug 19, 2020

barklund commented Aug 19, 2020

miina commented Aug 20, 2020

miina commented Aug 20, 2020

merapi commented Aug 20, 2020

miina Aug 20, 2020

Choose a reason for hiding this comment

barklund Aug 20, 2020 • edited

Choose a reason for hiding this comment

miina commented Aug 20, 2020

barklund commented Aug 20, 2020

miina left a comment

Choose a reason for hiding this comment

merapi commented Aug 21, 2020 • edited

wassgha commented May 29, 2020 •

edited by barklund

codecov bot commented May 29, 2020 •

edited

github-actions bot commented May 29, 2020 •

edited

barklund Aug 20, 2020 •

edited

merapi commented Aug 21, 2020 •

edited