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

Update the URL when a fragment link to that navigates the current doc is clicked #1461

Merged
merged 1 commit into from
Jan 19, 2016

Conversation

cramforce
Copy link
Member

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.

Fixes #1398
Fixes #1397
Fixes webcompat/web-bugs#1793

@@ -645,9 +645,6 @@ export class ViewportBindingNatural_ {
if (doc./*OK*/scrollingElement) {
return doc./*OK*/scrollingElement;
}
if (doc.body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is correct? There are still lots of chromes out there with the old API...

Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to check the platform here - I looked around. This will definitely cause many problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

…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.
@dvoytenko
Copy link
Contributor

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
@cramforce cramforce merged commit 00840a3 into ampproject:master Jan 19, 2016
@cramforce cramforce deleted the fragment-scrolling branch January 19, 2016 18:06
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd certainly not work if the document is not presently scrollable (i.e. too short).

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