-
Notifications
You must be signed in to change notification settings - Fork 218
elementFromPoint + elementsFromPoint #642
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
Conversation
93f51de
to
f74647c
Compare
20832dc
to
b8b3f0e
Compare
src/browser/html/document.zig
Outdated
@@ -260,4 +282,14 @@ test "Browser.HTML.Document" { | |||
.{ "document.cookie = 'favorite_food=tripe; SameSite=None; Secure'", "favorite_food=tripe; SameSite=None; Secure" }, | |||
.{ "document.cookie", "name=Oeschger; favorite_food=tripe" }, | |||
}, .{}); | |||
|
|||
try runner.testCases(&.{ | |||
.{ "document.elementFromPoint(0.5, 0.5)", "null" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a problem. Why does it return null
? I mean, I know why it returns null
, but I don't think it should.
This is the HTML document:
<div id="content">
<a id="link" href="foo" class="ok">OK</a>
<p id="para-empty" class="ok empty">
<span id="para-empty-child"></span>
</p>
<p id="para"> And</p>
<!--comment-->
</div>
shouldn't there be something at 0.5, 0.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our renderer is lazy so only renders things that have been registered.
The user cannot really know what the location is of any element without having queried for its location.
I think that returning document.documentElement
is better.
Not sure if we want to (re-)register all elements when we call getElementAtPosition
. The only use case I can currently see is when the script relies on the renderer being filled in a deterministic way, which I think they should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will however likely need to make our rendered a bit less flat.
Meaning a bit of a tree where an element can hold a list of other documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should document very clearly how our renderer works and what is and what is not a valid use case scenario.
We need to avoid falling into a pit where we end up having to implement a full renderer.
I'll start a discussion to document the current design ideas and goals of the renderer, and how that changes what kind of script we support (like no guessing what is as 0.5 0.5).
The only alternative that I see that would be better is if we have an on-the-fly renderer. That only renders everything on demand for a function such as this, but still does not keep track of changes and such.
For now I think we should stick with the no guessing (and no overlaying approach until it breaks)
8971cdd
to
674743f
Compare
src/browser/dom/element.zig
Outdated
const root = try parser.nodeGetRootNode(parser.elementToNode(self)); | ||
if (root != parser.documentToNode(parser.documentHTMLToDocument(state.document.?))) { | ||
return DOMRect{ .x = 0, .y = 0, .width = 0, .height = 0 }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is here for now as I think it definitely should not be in the FlatRenderer.
We may wrap the FlatRenderer in a ViewPort kind of object which would handle these as well as multiple documents and document renderer relationships.
But I do not think this is the right time to go there.
.{ "r4.width", "0" }, | ||
.{ "r4.height", "0" }, | ||
|
||
// Test setup causes WrongDocument or HierarchyRequest error unlike in chrome/firefox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we should not be throwing these errors in this use case, so a pre-exiting bug that is not fixed in the PR
src/browser/dom/element.zig
Outdated
pub fn _getBoundingClientRect(self: *parser.Element, state: *SessionState) !DOMRect { | ||
// Since we are lazy rendering we need to do this check. We could store the renderer in a viewport such that it could cache these, but it would require tracking changes. | ||
const root = try parser.nodeGetRootNode(parser.elementToNode(self)); | ||
if (root != parser.documentToNode(parser.documentHTMLToDocument(state.document.?))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also do:
@intFromPtr(root) == @intFromPtr(state.document.?)
but I'm not sure one is that much better than the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ye, I choose not to as making it a habit means every time that code is copied one would need to make sure the pointers of comparable types. Now if there is a function netsurf.zig that does the conversion it should be safe. But also meh :) I left it as it was.
if (options) |options_| if (options_.composed) { | ||
log.warn("getRootNode composed is not implemented yet", .{}); | ||
}; | ||
return try Node.toInterface(try parser.nodeGetRootNode(self)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to understand.
If @intFromPtr(self) == @intFromPtr(state.document.?)
, you could use the existing parser.nodeOwnerDocument
?
But for cases where the document isn't attached, and possibly also the shadow dom, you use the new nodeGetRootNode
(which is also safe to use if it is attached, just not as efficient).
Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if
self == state.document
than we can returnself
[htmldocument ] - if
self == state.document.documentElement
than we can returnstate.document
[htmldocument ]
In those cases that would also be the same asparser.nodeOwnerDocument
Note that nodeOwnerDocument
is set even for detached nodes. It references the original document until it is appended as child to another document.
So we cannot use nodeOwnerDocument
to determine whether a node is detached.
So yes, if we know that the node is attached parser.nodeOwnerDocument
would yield the correct answer.
I actually now use the difference between getRootNode and nodeOwnerDocument to determine whether an element is or is not detached.
032d381
to
c9ce527
Compare
c9ce527
to
b834fab
Compare
Depends on: #638
Implements the last 2 web apis for Playwright click:
Document.elementFromPoint
Document.elementsFromPoint
https://developer.mozilla.org/en-US/docs/Web/API/Document/elementFromPoint
Reimplemented:
Node.getRootNode
also in netsurfElement.getBoundingClientRect
Element.getClientRects