Update the URL when a fragment link to that navigates the current doc is clicked #1461
Conversation
This file contains 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
cramforce
force-pushed
the
fragment-scrolling
branch
from
09b92d2
to
f327bab
Compare
| @@ -645,9 +645,6 @@ export class ViewportBindingNatural_ { | |||
| if (doc./*OK*/scrollingElement) { | |||
| return doc./*OK*/scrollingElement; | |||
| } | |||
| if (doc.body) { | |||
There was a problem hiding this comment.
Are you sure this is correct? There are still lots of chromes out there with the old API...
There was a problem hiding this comment.
We may need to check the platform here - I looked around. This will definitely cause many problems.
…ument is clicked. Also fixes fragment navigation in Firefox. It does that by deleting the code path that chose `document.body`. At least with Safari 9 all other major browsers appear to be supporting `document.scrollingElement`, to that it should be safe to pick the object here that is correct for FF. I might be missing something, though.
cramforce
force-pushed
the
fragment-scrolling
branch
from
f327bab
to
c12ce46
Compare
|
LGTM |
cramforce
added a commit
that referenced
this pull request
Jan 19, 2016
Update the URL when a fragment link to that navigates the current doc is clicked
| // Firefox does not support scrollTop on doc.body for default | ||
| // scrolling. | ||
| // See https://github.com/ampproject/amphtml/issues/1398 | ||
| // Unfortunately there is no way to feature detect this. |
There was a problem hiding this comment.
Can't you scroll, and see if the scrolled?
const scrollTop = doc.body.scrollTop;
doc.body.scrollTop = 1;
const scrolled = doc.body.scrollTop === 1;
doc.body.scrollTop = scrollTop;There was a problem hiding this comment.
@jridgewell we don't do feature testing if the feature testing would force layout or style recalc.
I'm also not sure that would actually work :)
There was a problem hiding this comment.
It'd certainly not work if the document is not presently scrollable (i.e. too short).
This was referenced Apr 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Also fixes fragment navigation in Firefox. It does that by deleting the code path that chose
document.body. At least with Safari 9 all other major browsers appear to be supportingdocument.scrollingElement, to that it should be safe to pick the object here that is correct for FF. I might be missing something, though.Fixes #1398
Fixes #1397
Fixes webcompat/web-bugs#1793