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.

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

Jump to bottom

Block Editor: Fix JS error in the 'useTabNav' hook #67102

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Block Editor: Fix JS error in the 'useTabNav' hook #67102

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2eceb24
Block Editor: Fix JS error in the 'useTabNav' hook
Mamaduka Nov 19, 2024
ef210ef
Focus on canvas when there's no section root
Mamaduka Nov 22, 2024
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
15 changes: 9 additions & 6 deletions packages/block-editor/src/components/writing-flow/use-tab-nav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ export default function useTabNav() {
const noCaptureRef = useRef();

function onFocusCapture( event ) {
const canvasElement =
container.current.ownerDocument === event.target.ownerDocument
? container.current
: container.current.ownerDocument.defaultView.frameElement;

// Do not capture incoming focus if set by us in WritingFlow.
if ( noCaptureRef.current ) {
noCaptureRef.current = null;
Expand Down Expand Up @@ -64,17 +69,15 @@ export default function useTabNav() {
.focus();
}
// If we don't have any section blocks, focus the section root.
else {
else if ( sectionRootClientId ) {
Copy link
Member Author

@Mamaduka Mamaduka Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we provide a fallback here? However, I'm not sure what the best one is for Zoom Out mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

@getdave - is the intention that getSectionRootClientId() will always return something? My question is, should we handle a fallback in case there isn't a section root, or is the bug that a sectionRoot should always be defined?

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, technically, it does; it returns an empty string, "," which in the editor means the main root. But it's only an internal marker, has no representation in Canvas, and can't be focused.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's important to be able to access the editor canvas and know it's empty. I'd support moving focus to the canvas wrapper as a fallback. If there are no empty states or focus, we can add those in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of fallback I'd just allow it to focus the canvas if the section root is an empty string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 8920245.

container.current
.querySelector( `[data-block="${ sectionRootClientId }"]` )
.focus();
} else {
// If we don't have any section root, focus the canvas.
canvasElement.focus();
}
} else {
const canvasElement =
container.current.ownerDocument === event.target.ownerDocument
? container.current
: container.current.ownerDocument.defaultView.frameElement;

const isBefore =
// eslint-disable-next-line no-bitwise
event.target.compareDocumentPosition( canvasElement ) &
Expand Down
Loading