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

Performance 2022: CWV gaming #37

Merged
merged 13 commits into from
May 31, 2022
Merged

Performance 2022: CWV gaming #37

merged 13 commits into from
May 31, 2022

Conversation

rviscomi
Copy link
Member

@rviscomi rviscomi commented May 28, 2022

@rviscomi rviscomi marked this pull request as ready for review May 28, 2022 04:59
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
@rviscomi rviscomi requested review from mel-ada and 25prathamesh May 28, 2022 05:05
@rviscomi rviscomi removed their assignment May 28, 2022
dist/performance.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mel-ada mel-ada left a comment

Choose a reason for hiding this comment

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

Looks good, thanks y'all!

In order to accurately detect the LCP overlay hacks, we need to make sure the element that we're evaluating for overlay styles is the actual LCP element. I will add this and my other suggestions in a separate branch.

dist/performance.js Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
dist/performance.js Outdated Show resolved Hide resolved
@mel-ada
Copy link
Contributor

mel-ada commented May 29, 2022

@25prathamesh @rviscomi
I've created a work in progress Draft PR with my suggestions. I've made as much progress as I can with the time I have available today, and I've listed the current bugs in the PR description. Any insight/help is greatly appreciated! Prathamesh, feel free to work from this branch if you agree with the direction - I will have some limited time tomorrow to continue working.

@rviscomi
Copy link
Member Author

@tunetheweb same question as the other PR, could you merge this if I'm unavailable this week?

@tunetheweb
Copy link
Member

OK, but bit confused where this is. Are we merging @mel-ada 's changes into this one? And then good to merge if no further feedback? Or something else?

@rviscomi
Copy link
Member Author

Yeah the plan is to finish the review on #39 and merge into this. Then we can resolve the open feedback and merge this.

* [WIP] add soft gaming metrics detections: doesElementCoverPercentageOfViewport, findHighestZ. Default isLcpStaticallyDiscoverable to true. Update lcpOverlay metric to target LCP metric only.

* Update dist/performance.js

Co-authored-by: Rick Viscomi <rviscomi@users.noreply.github.com>

* remove IntersectionObserver, remove findHighestZ, gather all styles instead of only computed or only inline, improve soft metric logic

* resolve linting errors

Co-authored-by: Rick Viscomi <rviscomi@users.noreply.github.com>
@mel-ada
Copy link
Contributor

mel-ada commented May 31, 2022

@tunetheweb @rviscomi @25prathamesh
It looks like all metrics are working as expected - here is how I'm testing. If you agree please merge. Thanks!

List of all Custom Metrics Added in PR:

"gaming_metrics":{
  "detectUA-ChromeLH":true,
  "detectUA-GTmetrix":true,
  "detectUA-PageSpeed":true,
  "imgAnimationStrict":true,
  "imgAnimationSoft":true,
  "fidIframeOverlayStrict":true,
  "fidIframeOverlaySoft":true,
  "lcpOverlayStrict":true,
  "lcpOverlaySoft":true
}

Tests for PR:

Inline JS UA Detection Passing

Expected Result:

"gaming_metrics":{
  "detectUA-ChromeLH":true,
  "detectUA-GTmetrix":true,
  "detectUA-PageSpeed":true,
  "lcpOverlaySoft":false
}

CodePen:
https://cdpn.io/pen/debug/rNJpEQd?authentication_hash=NQMzYoKdVqLk (Note the debug in the URL, this removes a CodePen iframe which was preventing inline UA detection from being picked up in @25prathamesh's comment here)

WPT Result: https://www.webpagetest.org/result/220531_AiDc2M_B2N/1/details/#waterfall_view_step1

External JS UA Detection Passing

Expected Result:

"gaming_metrics":{
  "detectUA-ChromeLH":true,
  "fidIframeOverlaySoft":false,
  "lcpOverlaySoft":false
}

WPT Result: (See Example url in test, not a CodePen)
https://www.webpagetest.org/result/220531_AiDcSX_91R/1/details/#waterfall_view_step1

Strict & Soft CWV Metric Hacks Passing

Expected Result:

"gaming_metrics":{
  "imgAnimationStrict":true,
  "imgAnimationSoft":true,
  "fidIframeOverlayStrict":true,
  "fidIframeOverlaySoft":true,
  "lcpOverlayStrict":true,
  "lcpOverlaySoft":true
}

Test Codepen:
https://cdpn.io/pen/debug/KKQQqwp?authentication_hash=dGkXWNjPjVdA

WPT result:
https://webpagetest.httparchive.org/result/220531_1Y_2/1/details/#waterfall_view_step1

@tunetheweb
Copy link
Member

@25prathamesh will wait for your OK since you were going to check some other things. If I don’t hear back by this evening, then will go ahead and merge.

@25prathamesh
Copy link
Contributor

HI @tunetheweb
I have made a small changes to prevent lcpOverlaySoft:false passed on every test
commit ID - cca817a

I did another round of testing with new JS, and everything working properly, you can merge

Strict & Soft CWV Metric Hacks Passing
CodePen Test URL -
https://cdpn.io/pen/debug/KKQQqwp?authentication_hash=dGkXWNjPjVdA

WPT Test URL -
https://www.webpagetest.org/result/220531_BiDcXB_BRX/

Inline JS UA Detection Passing
CodePen Test URL -
https://cdpn.io/pen/debug/eYVGjzr?authentication_hash=NjrYzpGOZPZA
WPT Test URL -
https://www.webpagetest.org/result/220531_BiDcFE_BS6/1/details/#waterfall_view_step1

External JS UA Detection Passing
Test URL -
https://lakmeindia.com/

WPT Test URL -
https://www.webpagetest.org/result/220531_BiDcAE_BT9/1/details/#waterfall_view_step1

@tunetheweb
Copy link
Member

Looks like you made that commit on your own branch. Can you open a PR to this branch?

@25prathamesh
Copy link
Contributor

Looks like you made that commit on your own branch. Can you open a PR to this branch?
just pushed to perf-hacks branch, refer below link
89e4309

@tunetheweb tunetheweb merged commit 961e5f4 into main May 31, 2022
@tunetheweb tunetheweb deleted the perf-hacks branch May 31, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants