Implement DOM.describeNode
Categories
(Remote Protocol :: CDP, task, P1)
Tracking
(firefox76 fixed)
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
()
Details
(Whiteboard: [puppeteer-beta-mvp])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Another API which is used a lot at least in Puppeteer unit tests, and is causing a lot of those to fail.
Puppeteer itself only uses the objectId
for referencing the node. Every other optional argument is not in use. The result is a node description, whereby only its backendNodeId
and frameId
properties are used within Puppeteer.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
This is according to whimboo one of the currently highest ranking failures in Puppeteer unit tests.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
I'm struggling in what backendNodeId
actually should be. It is very hard to find something in Chromium's code about that.
Assignee | ||
Comment 3•5 years ago
|
||
So we actually have to implement an element store similar to the one from Marionette. With that we have our own mirror of DOM nodes as present in the various targets. Once an element is requested it needs to be added to that store, and at the same time it will get a unique id. Given that we don't have to diverge between the front-end and backend it looks like that nodeId
and backgroundNodeId
can be the same.
As temporary solution (which even makes some Puppeteer tests pass) I simply return the node's data. And by that I got nearly all of the available properties filled with data:
puppeteer:protocol SEND ► {"sessionId":1,"method":"DOM.describeNode","params":{"objectId":"0b2954a7-6584-7a49-88f6-08d4e221540d"},"id":19} +1ms
puppeteer:protocol ◀ RECV {"sessionId":1,"id":19,"result":{"node":{"nodeType":1,"nodeName":"IFRAME","localName":"iframe","nodeValue":"","childNodeCount":0,"attributes":["id","foo","src","<div>a</div>"],"frameId":"11"}}} +2ms
Some still have a wrong value (nodeId
, childNodeCount
), which needs to be fixed. But most of the work is still the implementation of the element store.
Does it make sense to land your temporary solution now and follow up with the element store implementation later? Or rather do all of it in this bug?
Looking at Puppeteer source, the minimum needed the objectId parameter and the backendNodeId property on the return value (which can be the same as the nodeId, as you say.)
I expect the next method we'll need is DOM.resolveNode.
Assignee | ||
Comment 5•5 years ago
|
||
The backendNodeId is a custom id we internally have to set. We cannot return a random number without being able to store the correlation between this id and the element reference. Otherwise different ids would be returned each time the method gets called.
So we clearly need the element store, which indeed will be worked on all on this bug.
The way gutenberg depends on this is through Puppeteer's Page.waitForSelector -- the call ends up in https://github.com/puppeteer/puppeteer/blob/14b236965000c2821aece8deae72fb927ab33930/lib/ExecutionContext.js#L189-L201
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
The last try build had some problems that are fixed now. Lets see how many Puppeteer tests unexpectedly start failing or passing now. Locally I can see a couple more passing but I assume it's the same race, which I saw on bug 1612174.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e58f4a087cabbc5cd659a33623252a80c658631
Assignee | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Updated•4 years ago
|
Description
•