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

Implement browsingContext.locateNodes #547

Merged
merged 86 commits into from
Nov 2, 2023
Merged

Implement browsingContext.locateNodes #547

merged 86 commits into from
Nov 2, 2023

Conversation

jimevans
Copy link
Collaborator

@jimevans jimevans commented Sep 14, 2023

Fixes #150.

This PR implements the browsingContext.locateNodes command, which allows the user to locate nodes in the DOM using means other than returning them via JavaScript. This also allows for future expansion of location strategies should they become available.


Preview | Diff

Copy link

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

Very excited to see this in the spec :)

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

So in general, it's unclear to me what level this API is supposed to work at. It seems like the original intent was to push something like Playwright's API directly into the WebDriver protocol. But I think I'd prefer not to try to transcribe the current state of the art into a JSON API, but instead provide the necessary primitives without baking in lots of internal looping.

Also from a spec point of view, I think this really should assume that's it's not running in a js context, and that implementations might work by directly calling internal browser methods on the DOM, rather than going via a scripting context.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Future of browsingContext.locateNodes.

The full IRC log of that discussion <AutomatedTester> Topic: Future of browsingContext.locateNodes
<AutomatedTester> github: https://github.com//pull/547
<gsnedders> RRSAgent, make these logs public
<RRSAgent> I have made the request, gsnedders
<gsnedders> GitHub: https://github.com//pull/547
<AutomatedTester> Jim Evans: We have reached the point in this PR that gives us the bare minimum to hava locators of elements to map over classic
<AutomatedTester> ... this was initially discussed at TPAC
<AutomatedTester> ... there are some questions around the PR. Do we want to land as is or do we want to start discussing command batching?
<jgraham_> q+
<AutomatedTester> ?
<AutomatedTester> ack next
<gsnedders> github: https://github.com//pull/547
<AutomatedTester> jgraham: My last comment on the PR is that I can see both sides on this. When looking at PR and in particular text locators is that its not what Selenium, playwright, or puppeteer do
<AutomatedTester> ... I am concerned that we will standardise prematurely here
<AutomatedTester> ... and the PR is closer to what Selenium does.
<AutomatedTester> ... and if we standardise prematurely that people might not use it and inject JS to get around it
<AutomatedTester> ... so I have a specific question that I have is "looking at the PR for the command batching does this look like a good use of the protocol?"
<AutomatedTester> ... I see 2 approaches. Have a generic batching command system or we have one for each command as it appears
<AutomatedTester> q+
<AutomatedTester> ... so I see that this conversation should happen in parallel to this conversation
<jrandolf> q+
<gsnedders> RRSAgent, draft the minutes
<AutomatedTester> ... i feel like with actions we should have built a more generic appraoch
<RRSAgent> I have made the request to generate https://www.w3.org/2023/10/11-webdriver-minutes.html gsnedders
<jgraham_> ScribeNick: jgraham
<jrandolf> q-
<shs96c> q+
<jrandolf> q+
<jgraham_> AutomatedTester: I don't want us to derail too much. To your point of this could be a fun engineering problem, I think there's a concern that we spend too much time prematurely optimising to get batching correct. I hope we can get something working and can build the more generic implementation later. With locators, I'm worried that if we carry on with the "inject js where we need it" we're relying too
<jgraham_> much on the community to build the functionality. Selenium don't have the capacity to build something here.
<orkon> q+
<jgraham_> ScribeNick: AutomatedTester
<AutomatedTester> ack next
<AutomatedTester> ack next
<AutomatedTester> shs96c: a couple of points; firstly it's a very important feature to have. We should get it landed and iterated over that. secondly, in classic we did talk about moving to innertext that we could hide behind capability
<jgraham_> q+
<AutomatedTester> ... thirdly <discusses innerText>
<AutomatedTester> ack next
<jgraham_> q- jgraham_
<AutomatedTester> jgraham_ (IRC): how close is it to classic? It's pretty close but classic is a lot less flexible. and to David's comment it was 20 lines of code so it's not to be hard
<AutomatedTester> shs96c: but you did mention that it can't do ShadowDOM and that needs to be handled. There might be other places that this could come up, we just know this 1 place
<jgraham_> q+
<AutomatedTester> jrandolf: regarding batching. Looking at how batching generally works. Why cant we just have the generic and just let the server handle it like <something in kubenetes>
<AutomatedTester> shs96c: Selenium wants the batching because they mostly use service providers. This means they use the client and a large amount of the internet.
<AutomatedTester> jrandolf: my thought is that has made a client could inject the batching and then the server just reads it and breaks it down.
<AutomatedTester> ... so each driver could do this
<AutomatedTester> shs96c: if each driver made their own there wouldn't be great interop. There are also providers who are trying to route this round and then having to figure things out could be extra complex
<AutomatedTester> ... and I agree with jgraham
<AutomatedTester> q?
<AutomatedTester> ... one command output could be the input for the next command
<AutomatedTester> ack next
<AutomatedTester> orkon: about the PR and landing it. We could land things here and we don't have objections of landing but we would need to do a final review and prototyping
<AutomatedTester> ... and batching we need it to be it's own discussion
<AutomatedTester> ack next
<AutomatedTester> jgraham: the use is to remove latency and there are user flows
<AutomatedTester> ... <gives an example where one locator flows into another especially around Shadow piercing>
<AutomatedTester> ... it's kinda like a Promise graph that needs to be sent across the wire. But if people see there is a lot of value in getting it landed let's do that
<JimEvans> q+
<AutomatedTester> ack next
<AutomatedTester> Jim Evans: as I mentioned in the issue around batching is that if we do it then I think it will open a wealth of new features but I have a lot of ideas and use cases that I can collaborate with. I think this could be a really massive game changing feature
<shs96c> q+
<AutomatedTester> ack next
<jrandolf> q+
<jrandolf> q+
<jrandolf> q-
<AutomatedTester> shs96c: Do we have someone that is able to do this feature or is it going to be a nice to have and won't be worked on in 3-6 months
<jrandolf> q+
<AutomatedTester> ack jrandolf
<jrandolf> q-

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jimevans
Copy link
Collaborator Author

Beginnings of WPT tests at web-platform-tests/wpt#42565

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

LGTM

@jimevans jimevans merged commit 44aa704 into main Nov 2, 2023
3 checks passed
@jimevans jimevans deleted the jimevans/locate-nodes branch November 2, 2023 13:38
github-actions bot added a commit that referenced this pull request Nov 2, 2023
SHA: 44aa704
Reason: push, by jimevans

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add browsingContext.locateNodes command to find nodes within a page