-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
feat: get rendered fonts for element #13327
base: main
Are you sure you want to change the base?
Conversation
f31bbcc
to
65ef6a3
Compare
const nodeId = nodeInfo.nodeId; | ||
|
||
const platformFonts = await this.client.send( | ||
'CSS.getPlatformFontsForNode', |
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 would not like to bring this method to Puppeteer as the method exists primarily for the DevTools frontend and it is very specific to the UI logic there.
How about we document instead how to call this using a custom session created using createCDPSession? Also, what is the use case for getting the font data via Puppeteer? If the use case is important, I think we could add this method as experimental but let us fix the upstream method first so that it does not require DOM and CSS domains to be enabled and so that it accepts a backend node ID without the need to rely on the fragile DOM nodeIds (they reset on every call to DOM.getDocument so the method might fail if called concurrently).
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.
For me, the use case is testing whether or not stylesheets are written correctly, but I am sure we can find more use cases for it. I prefer to "hide" the implementation in puppeteer and not access private properties, as they tend to break down more frequently.
I also would have preferred having the upstream method fixed so it would accept a backend node ID / remote object ID. I try to avoid basing things on the nodeIds in my other projects as well, as I have experienced them changing frequently in some cases.
This PR can wait if this change can happen soon
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.
With #13328 the private property access will not be needed. Would you like to contribute to the fixes in Chromium?
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.
Sure. I am not familiar enough with the code base but this seems like a relatively minor issue, so maybe I can start with it
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.
here are the getting started instructions for building/testing https://www.chromium.org/developers/how-tos/get-the-code/ , it would require updating https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/devtools_protocol/browser_protocol.pdl;l=1;drc=93fc8bf077eb95148a50f71b9917ebe6bd5a02d3 to add optional objectId and backendNodeId params, then re-build to update the generated classes, and then in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_css_agent.cc;l=1911;drc=b59cf80380080c28b981e2dcd491ef3efa9d8808 use the overload from here https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_dom_agent.cc;l=430;drc=1651676a30cd7abcd177975f7cd0e37bd945f663 Then the check if the agent is enabled can be removed and a test needs to be added verifying that it works with the new ids and also without the CSS domain enabled.
What kind of change does this PR introduce?
feature
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
Yes
Summary
Adds the ability to get the rendered fonts (and some metadata) for text nodes, based on element handle.
Solves #9757
Does this PR introduce a breaking change?
No
Other information