-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Perform Chrome version check to enable highlighting using text fragments #35633
Conversation
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.
Left a small comment but otherwise looks great!
// `'fragmentDirective' in document = true` which breaks feature detection. | ||
// Chrome 93 supports the proposal that works across iframes, hence this | ||
// version check. | ||
if (platform.isChrome() && platform.getMajorVersion() >= 93) { |
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.
Can we also include the feature detection? After a bit we can remove the Chromium 93 hard coding and rely solely on feature detection.
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.
Done
this.ampdoc_.win | ||
); | ||
|
||
// Chrome 81 added support for text fragment proposal. However, 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.
Great comment that explains the scenario well. Thanks!
} | ||
); | ||
|
||
// TODO(dmanek): remove `ifChrome` once other major browsers support text fragments |
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.
If we retain the feature detection, would we need to run this test against Chromium only?
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.
Correct. If we were not doing Chrome version detection, this could be removed. I've updated the TODO.
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.
1
Partial for #32139.