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

feat: get rendered fonts for element #13327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kle0s
Copy link
Contributor

@Kle0s Kle0s commented Nov 25, 2024

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

const nodeId = nodeInfo.nodeId;

const platformFonts = await this.client.send(
'CSS.getPlatformFontsForNode',
Copy link
Collaborator

@OrKoN OrKoN Nov 25, 2024

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).

Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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.

@github-actions github-actions bot added the Stale label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants