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

LibWebView: Add keyboard navigation to the Inspector #628

Merged

Conversation

circl-lastname
Copy link
Contributor

Now keep a track of the DOM nodes which are actually visible in the Inspector, and allow the user to navigate the tree using the keyboard.

@circl-lastname circl-lastname force-pushed the inspector_arrow_movement branch 3 times, most recently from ee4c320 to 0871b87 Compare July 15, 2024 11:11
Base/res/ladybird/inspector.js Show resolved Hide resolved
Base/res/ladybird/inspector.js Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/DOM/Document.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/DOM/Document.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/DOM/Document.h Outdated Show resolved Hide resolved
@circl-lastname
Copy link
Contributor Author

Let me know if that's an okay caching solution

Comment on lines +1913 to +1914
// 1. Let candidate be the DOM anchor of the focused area of this DocumentOrShadowRoot's node document.
Node* candidate = focused_element();
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly tangential to your work, but I think our concept of a focused_element() might be wrong. The spec links from this step say:

Focusable areas can be elements, parts of elements, or other regions managed by the user agent.

Each focusable area has a DOM anchor, which is a Node object that represents the position of the focusable area in the DOM.

If I'm reading this right, it could be a different kind of Node than just an Element. This makes the verify_cast<Element>(candidate) below a bit dangerous. I think maybe we should instead have focused_node(), but I'm not entirely confident I'm understanding this right.

Copy link
Contributor Author

@circl-lastname circl-lastname Jul 18, 2024

Choose a reason for hiding this comment

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

#628 (comment)

I've addressed that here, the focused_element will as far as I know always be an element in the current implementation.
Specifically, you can see the piece of code I linked that prevents the focused_element from becoming the document, contrary to the spec, so I think it is safe to keep for now.

@AtkinsSJ
Copy link
Contributor

To comment on actually trying this out: It's very nice! I'd like if the left and right arrow keys also worked to expand/contract elements and navigate to their parent or child, but that can go in a separate PR.

Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

I think we'll end up addressing the "which node is really focused" stuff when we revisit this, so for now this is sweet! More DX for the Inspector is always appreciated

@ADKaster ADKaster merged commit e59048e into LadybirdBrowser:master Jul 18, 2024
6 checks passed
@circl-lastname circl-lastname deleted the inspector_arrow_movement branch July 18, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants