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

core(tap-targets): drop custom node rect creation #12005

Merged
merged 3 commits into from
Feb 11, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

Fixes #11948

@connorjclark connorjclark requested a review from a team as a code owner January 26, 2021 20:52
@google-cla google-cla bot added the cla: yes label Jan 26, 2021
@@ -4827,12 +4827,12 @@
"path": "3,HTML,1,BODY,17,BUTTON",
"selector": "body > button.small-button",
"boundingRect": {
"top": 484,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just noticed these (huge) differences :o need to investigate

Copy link
Collaborator Author

@connorjclark connorjclark Jan 26, 2021

Choose a reason for hiding this comment

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

ok I had to remind myself that this bounding rect is only used for full page screenshot positioning, and the value will have no effect on tap target results

I reckon numbers are so different because we changed deviceScaleFactor

Running LH against dbw_tester and viewing report shows that the screenshot lines up just fine.

I updated the sample artifact for TapTargets, it was a little old.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Sorry, I saw the "need to investigate" comment and missed the follow up comment.

LGTM! The new rects look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use normal node rect for tap targets
3 participants