-
Notifications
You must be signed in to change notification settings - Fork 29
Custom metric for content-visibility #171
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
Closed
burakguneli
wants to merge
9
commits into
HTTPArchive:main
from
burakguneli:content-visibility-custom-metrics
Closed
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5aa1f76
Custom metric for content-visibility
burakguneli 3a78637
Move content_visibility to css.js
burakguneli 1a8d5fe
Replace expensive DOM traversal with efficient regex-based CSS parsing
burakguneli 6d19ce8
Fix linter issues
burakguneli cad76c8
Add debugging
burakguneli f7522aa
Add try catch
burakguneli a4f7467
Return data JSON-stringified
burakguneli 7faf743
Use parsed_css
burakguneli 9f7e264
Rename file to content_visibility
burakguneli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@pmeenan any concerns with running getComputedStyle on every element in the DOM?
It sounds like a terrible idea for performance but custom metrics are executed off page load so I guess it doesn't really matter for those does 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.
I'm on the fence. I assume it will only force a layout once if one is pending and then can return the element position for everything else without having to recalculate anything expensive but doing anything on ALL page elements freaks me out a bit.
My query selector foo isn't great, but is there a way to query based on content-visibility and only retrieve the elements with the "interesting" content visibility types directly?
I'd recommend trying it on some extreme edge-case pages (like CNN) to see how bad it is.
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.
@burakguneli did suggest doing this for inline styles. But that's very limited. We really need the calculated style, which won't be directly queryable.
The other option is to look at the
parsed_cssdata. I'm not totally familiar with that to be honest and whether that's just "we have a selector that usescontent-visibilitybut no idea if it's used?" or if it actually gives the usage. @burakguneli was going to look further in this.Uh oh!
There was an error while loading. Please reload this page.
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.
@tunetheweb @pmeenan I worked on an alternative way instead of doing expensive calls like
getComputedStyle. I will send my commit and try to see if that works as expectedThere 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'm n ot sure what manually processing the CSS gains over using
parsed_css(which basically does this already)?Uh oh!
There was an error while loading. Please reload this page.
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.
@tunetheweb thanks for checking and your feedback. I think you are right, the best way would be to use already parsed css. I'm kind of trying to learn how these custom metrics work, sorry for the many commits and notifications you are getting because of it. I'm not clear on one thing. Do you know how I can use the already parsed CSS in my custom metric file
content_visibility.js?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.
The idea would be you wouldn’t need this custom metric and instead would get the data you need by selecting from that table.
Try it out for last month for a single URL (make sure to bill the httparchive project).
As I said earlier the downside is that I think it only says if a selector exists using that property. Not how much it’s actually used (or even if at all!). But not familiar with that table so not 100% sure about that and maybe it is better than that.
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.
oh thanks I got it now! For that, I tried running a couple of queries on the 30th of June like that: https://console.cloud.google.com/bigquery?inv=1&invt=Ab19zQ&project=httparchive&ws=!1m10!1m4!4m3!1shttparchive!2ssample_data!3sparsed_css_10k!1m4!1m3!1shttparchive!2sbquxjob_69d0dc73_197c1eb5200!3sUS
but after running this query I got the impression that we don't have it and we need to create a custom metric. Maybe it was my bad and I couldn't write a proper SQL query. I will try to improve it and retry!
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.
This seems to work:
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.
that works amazing thanks!!! @tunetheweb