Skip to content

Experimental Image Cropper: Ensure focus is on canvas when dragging #77591

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.

Sign up for GitHub

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

Merged
andrewserong merged 2 commits into from
Apr 24, 2026
Merged

Experimental Image Cropper: Ensure focus is on canvas when dragging #77591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30bda47
Experimental Image Cropper: Ensure focus is on canvas when dragging
andrewserong Apr 23, 2026
ffb8357
Add a test and ensure focus has been mocked to get things passing
andrewserong Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/media-editor/src/image-editor/core/interaction-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,18 @@ export class InteractionController {
}
e.preventDefault();

// Blur any focused handle so its focus ring doesn't linger.
// Blur any focused handle so its focus ring doesn't linger,
// but leave the canvas itself alone so keyboard control works after drag.
const ownerDoc = el.ownerDocument;
if ( ownerDoc?.activeElement instanceof HTMLElement ) {
if (
ownerDoc?.activeElement instanceof HTMLElement &&
ownerDoc.activeElement !== el
Comment on lines +254 to +255

@ramonjd ramonjd Apr 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, good call. This works well. I'm also thinking that when the keyboard is panning the dim layer could also lighten like it does when mouse dragging.

2026-04-23.19.41.10.mp4

Here's how I tested:

diff 
diff --git a/packages/media-editor/src/image-editor/core/interaction-controller.ts b/packages/media-editor/src/image-editor/core/interaction-controller.ts
index 343a21a0883..3ed18d990e4 100644
--- a/packages/media-editor/src/image-editor/core/interaction-controller.ts
+++ b/packages/media-editor/src/image-editor/core/interaction-controller.ts
@@ -167,6 +167,12 @@ export class InteractionController {
 	/** Whether a wheel gesture is currently active. */
 	private wheelGestureActive = false;
 
+	/** Timer for keyboard pan gesture debounce. */
+	private keyboardGestureTimer: ReturnType< typeof setTimeout > | undefined;
+
+	/** Whether a keyboard pan gesture is currently active. */
+	private keyboardGestureActive = false;
+
 	/** Current requestAnimationFrame ID. */
 	private rafId = 0;
 
@@ -778,6 +784,25 @@ export class InteractionController {
 	handleKeyDown( e: KeyboardEvent ): void {
 		const currentState = this.options.getState();
 
+		if (
+			e.key === 'ArrowUp' ||
+			e.key === 'ArrowDown' ||
+			e.key === 'ArrowLeft' ||
+			e.key === 'ArrowRight'
+		) {
+			if ( ! this.keyboardGestureActive ) {
+				this.keyboardGestureActive = true;
+				this.setStatus( { isDragging: true } );
+				this.options.onGestureStart?.();
+			}
+			clearTimeout( this.keyboardGestureTimer );
+			this.keyboardGestureTimer = setTimeout( () => {
+				this.keyboardGestureActive = false;
+				this.setStatus( { isDragging: false } );
+				this.options.onGestureEnd?.();
+			}, DOUBLE_TAP_TIME );
+		}
+
 		switch ( e.key ) {
 			case 'ArrowUp': {
 				e.preventDefault();
@@ -903,6 +928,7 @@ export class InteractionController {
 		cancelAnimationFrame( this.rafId );
 		clearTimeout( this.zoomTimer );
 		clearTimeout( this.wheelGestureTimer );
+		clearTimeout( this.keyboardGestureTimer );
 		this.touchCleanup?.();
 		this.touchCleanup = null;
 		this.pointerCleanup?.();

On a side note, I activated showGrid locally. Do you think we should show the grid by default?

@ramonjd ramonjd Apr 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, good call. This works well. I'm also thinking that when the keyboard is panning the dim layer could also lighten like it does when mouse dragging.

On second thought, this is just an interesting nice to have. I think the keyboard functionality we have here is way more valuable than screen bling. I'm just wondering if it's worth thinking about how to indicate visually when the image is focussed and therefore pannable. Or if it's worth it at all.

@andrewserong andrewserong Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm just wondering if it's worth thinking about how to indicate visually when the image is focussed and therefore pannable. Or if it's worth it at all.

I do think it's valuable to think about for sure! My main goal at the moment was to try to look at it in little pieces. I'm not too sure about the focused state of the canvas itself, but when it comes to the drag handles, I could see having a focus style on them when dragging with the mouse, so it's clear what's focused, and we could do a similar thing of allowing keyboard controls at that stage, too.

Do you think we should show the grid by default?

My preference would be to not show the grid as it interferes with looking at the image itself. Something for us to play with could be either a) for the grid to show only while dragging, rotating, moving the crop etc, and then have it fade out, or b) have a toggle in the sidebar to control display. But I reckon let's look at the grid separately (good thinking, though!)

For now, I'll fix up the tests in this PR and see how we go. Then in a follow-up, I'd like to look at focusing the drag handles, and play around with visually indicating what's focused. What do you reckon?

@andrewserong andrewserong Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For a follow-up, just jotting down what Google Photos does for the drag handles. If you tab to them it's got a nice big focus styles to make it clear what you've got selected:

image

(It also uses circles instead of squares, not sure if we have a preference there)

In any case, happy to play around with it!

@ramonjd ramonjd Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For now, I'll fix up the tests in this PR and see how we go.

Sounds good to me. All these ideas are good. We can test them out later.

@ramonjd ramonjd Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like the browser takes care of it by default:

Screenshot 2026-04-24 at 10 16 26 am Screenshot 2026-04-24 at 10 17 28 am

) {
ownerDoc.activeElement.blur();
}

el.focus();

// Capture pointer so drag works across iframe boundaries.
el.setPointerCapture( e.pointerId );

Expand Down
11 changes: 11 additions & 0 deletions packages/media-editor/src/image-editor/core/test/interaction-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ function createMockElement(): HTMLElement & {
} {
const listeners: Record< string, EventListener[] > = {};
return {
focus: jest.fn(),
setPointerCapture: jest.fn(),
releasePointerCapture: jest.fn(),
addEventListener: jest.fn( ( type: string, fn: EventListener ) => {
Expand Down Expand Up @@ -331,6 +332,16 @@ describe( 'InteractionController', () => {
expect( onGestureEnd ).toHaveBeenCalledTimes( 1 );
} );

it( 'focuses the element on pointerdown', () => {
const state = makeState( { zoom: 2 } );
const { controller } = createController( state );
const el = createMockElement();

controller.handlePointerDown( createPointerEvent(), el );

expect( el.focus ).toHaveBeenCalled();
} );

it( 'reports isDragging via onStatusChange', () => {
const state = makeState( { zoom: 2 } );
const onStatusChange = jest.fn();
Expand Down
Loading