Add a dumb renderer to get coordinates #501
Conversation
krichprollsch
left a comment
krichprollsch
left a comment
There was a problem hiding this comment.
Thanks for this PR, it looks fine to me 👍
Page.click looks correct to me. The only thing we could change would be to dispatch a PointerEvent instead of a simple Event. See https://html.spec.whatwg.org/multipage/webappapis.html#fire-a-synthetic-pointer-event
But it implies some changes:
- creating a whole prototype chain of structs to represent the hierarchy:
Event <- UIEvent <- MouseEvent <- PointerEvent(https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent) (example w/ xhr progress event: https://github.com/lightpanda-io/browser/blob/main/src/xhr/progress_event.zig - add the progress event enum: https://github.com/lightpanda-io/browser/blob/main/src/netsurf/netsurf.zig#L520-L523
- set the internal event type on pointer event creation: https://github.com/lightpanda-io/browser/blob/main/src/xhr/progress_event.zig#L50
- add a case to cast the event correctly: https://github.com/lightpanda-io/browser/blob/main/src/events/event.zig#L65
FlatRenderer looks good to me.
Regarding tests I don't have strong opinion. We will add end-to-end test for click at least...
src/cdp/domains/page.zig
Outdated
|
|
||
| // TODO: hard coded ID | ||
| bc.loader_id = "AF8667A203C5392DBE9AC290044AA4C2"; | ||
| bc.url = url; |
There was a problem hiding this comment.
I'm afraid this one is not correct is case of redirection...
There was a problem hiding this comment.
This was existing code, but I agree. I removed the url state from the browsercontext and instead added a getURL() []const u8 method which gets the URL from the session's page.
I think there's more of this encapsulation / ownership we need to do, and I think that, in most cases, the Browser Session/Page should be the authoritative source.
|
libdom has a |
src/cdp/domains/input.zig
Outdated
|
|
||
| const bc = cmd.browser_context orelse return; | ||
| const page = bc.session.currentPage() orelse return; | ||
| const click_result = (try page.click(cmd.arena, params.x, params.y)) orelse return; |
There was a problem hiding this comment.
according with https://chromedevtools.github.io/devtools-protocol/tot/Input/#method-dispatchMouseEvent we should dispatch the event corresponding to the type given in parameter.
So I think we should:
- call
page.clickif the event type ismousePressedormouseReleased - add a
TODOto dispatch only the corresponding event in case ofmouseMoved,mouseWheel(and we don't want to click in this case)
There was a problem hiding this comment.
I made it all more generic, page.click -> page.mouseEvent, taking in a struct when an event type enum)
I switched to the mouse_event, but one of the parameters is a |
it looks like it's the It is inherited from the UIEvent: https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view |
|
@karlseguin can you rebase to fix the conflict please? |
FlatRenderer positions items on a single row, giving each a height and width of 1. Added getBoundingClientRect to the DOMelement which, when requested for the first time, will place the item in with the renderer. The goal here is to give elements a fixed position and to make it easy to map x,y coordinates onto an element. This should work, at least with puppeteer, since it first requests the boundingClientRect before issuing a click.
Add BrowserContext.getURL which gets the URL from the session.page.
@krichprollsch ready |
2 participants
This isn't complete.
Can't test because scripts run into other blockers before reaching this point (i.e. Dom.resolveNode). I do think it's pretty safe to merge this as-is though.
Looking for feedback on the
FlatRendererinbrowser.zigand Page.click handling (netsurf code is a bit tedious to us).Besides the inability to manually test. I'm also looking for feedback about how to test this. These flows are starting to get more complicated and I'm not sure they're reasonable for unit tests.