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

Fix Bug 1479817, anchor link headings are obscured by toc #4954

Merged
merged 2 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@schalkneethling
Collaborator

schalkneethling commented Aug 28, 2018

This resolves the problem described in bug 1479817 on Bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=1479817

I also extracted the scrollToHeading function into utils and removed the jQuery dependency it had. Also, added scroll-behaviour: smooth; to the html element so, now in supported browsers, the scroll behaviour will be silky smooth. (Try Fx)

@jwhitlock @hobinjk r?

@jwhitlock

Sorry @schalkneethling, I didn't get too far into my review. Here's some preliminary comments.

I don't understand the original issue enough to know if this fixes it. Maybe @hobinjk will have some time next week to take a look.

// collect all headings with an `id` attribute
var headings = Array.from(document.querySelectorAll('h2[id]')).concat(
Array.from(document.querySelectorAll('h3[id]'))
);
var pageUrl = document.location.href;
// if the url contains a hash, strip it away
if (window.location.hash) {

This comment has been minimized.

@jwhitlock

jwhitlock Sep 12, 2018

Member

I'm not familiar with the APIs. What is the difference between document.location and window.location? Why do you switch between the two?

Also, I am sad that a substr is needed to clear the hash. I hoped something like pageURL.hash = "" would do it, but that leaves a naked # at the end of the URL 😠

That being said, would it be better for pageURL to be an URL object, so that later you can set the hash with anchorURL = pageURL; anchorURL.hash = currentHeading.id; localAnchorTag = anchorURL.href, rather than string manipulation?

This comment has been minimized.

@schalkneethling

schalkneethling Sep 13, 2018

Collaborator

I'm not familiar with the APIs. What is the difference between document.location and window.location? Why do you switch between the two?

No difference, let me stick with document for consistency.

var scrollY = headingTop - tocHeight;
// update hash
window.location.hash = '#' + id;

This comment has been minimized.

@jwhitlock

jwhitlock Sep 12, 2018

Member

I think the '#' is unnecessary, this works in FF 63:

window.location.hash = id;
@schalkneethling

r+ This is gonna make a noticeable difference in performance I reckon. Cannot wait for font-smooth to be support everywhere. I makes such a massive difference. r+

Whoops, wrong PR :)

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1479817-anchor-links-heading-obscured-by-toc branch from 09f8757 to 719438b Sep 14, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 14, 2018

Thanks for the review @jwhitlock - updated

@hobinjk

hobinjk approved these changes Oct 1, 2018

Code LGTM. My local development environment is currently out of commission so I can't check it out on a real page.

@jwhitlock

Thanks for taking a look @hobinjk. I ran this again locally, and I found an issue when the URL contains an anchor.

Steps:

  1. Load test page into development environment with ./manage.py scrape_document https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
  2. Go to http://localhost:8000/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#Using_thisArg
  • Expected Result: Scrolls to "Using thisArg" <h3> heading, with heading visible below TOC
  • Actual Result: The heading is under the Table of Contents, and a JS error TypeError: can't assign to property "hash" on "http://localhost:8000/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach": not an object.

screenshot 2018-10-01 16 13 51

A new bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1495230, maybe has some easier steps to reproduce:

  1. Load test page into development environment with ./manage.py scrape_document https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Tag
  2. Go to http://localhost:8000/en-US/docs/MDN/Contribute/Howto/Tag#Tagging_problems_you_can_fix
  3. Click "Tagging problems you can fix" in the TOC (this is OK in production and development)
  4. In the section "No tags", click the "topic" link to go to http://localhost:8000/en-US/docs/MDN/Contribute/Howto/Tag#Topic
  • Expected result: Go to <h3> "Topic" under <h2> "Tag type guide". "Topic" heading is visible below the table of contents.
  • Actual result: "Topic" heading is obscured below the TOC.

screenshot 2018-10-01 16 19 44

This code needs a rebase as well - local-anchor.js has had some of these changes applied already.

// if the url contains a hash, strip it away
if (pageUrl.hash) {
pageUrl = pageUrl.href.substr(0, window.location.href.indexOf('#'));

This comment has been minimized.

@jwhitlock

jwhitlock Oct 1, 2018

Member

If the page has a hash, then pageURL turns from a URL object to a string. This results in a JavaScript error on line 28, when you try to set the .hash attribute of a string.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 2, 2018

Collaborator

Yup, this just needed a rebase.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 2, 2018

Ok, this was badly in need of a rebase. Think it was simmering here for a while ;)

Regarding the new bug > Actual result: "Topic" heading is obscured below the TOC.

This is another case we need to handle for in content links that links to a hash. On it.

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1479817-anchor-links-heading-obscured-by-toc branch from 719438b to 7b19af1 Oct 2, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 2, 2018

Additional issues addressed. Also found an additional problem(and fixed it) where the svg or the nested path would be the target of the click event.

Setting pointer-events to none prevents this from happening.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 2, 2018

Thanks for the review @jwhitlock and @hobinjk

@jwhitlock

This seems to work, but it removes the floating table of contents. Can we have both?

Array.from(document.querySelectorAll('#wikiArticle h3[id]'))
var mainDocument = document.getElementById('document-main');
// collect all headings with an `id` attribute
var headings = Array.from(document.querySelectorAll('h2[id]')).concat(

This comment has been minimized.

@jwhitlock

jwhitlock Oct 2, 2018

Member

Did you intend to drop #wikiArticle from the CSS selector?

This comment has been minimized.

@schalkneethling

schalkneethling Oct 2, 2018

Collaborator

I can definitely scope is to the main document instead. Good catch.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 2, 2018

This seems to work, but it removes the floating table of contents. Can we have both?

@jwhitlock When I have devtools open, the stickiness of the toc does go away but, I have not seen it not be sticky when devtools are closed. Are you seeing it not be sticky even with devtools closed?

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 2, 2018

I wasn't aware that the TOC went away with dev tools open, thanks. Now the TOC is back.

Test #1 is still broken. If I go directly to http://localhost:8000/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#Using_thisArg, then heading is hidden behind the TOC.

Test #2 works. If I go to http://localhost:8000/en-US/docs/MDN/Contribute/Howto/Tag, then click "Tagging problems you can fix", then the "Topic" link, I see the heading. However, if I open a new window/tab with links with anchor tags, the heading is obscured:

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 2, 2018

Test #1 is still broken. If I go directly to http://localhost:8000/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#Using_thisArg, then heading is hidden behind the TOC.

Hmmm, I cannot reproduce this one, let me take another look.

However, if I open a new window/tab with links with anchor tags, the heading is obscured:

Might be another edge case, let me look at this. Thanks @jwhitlock

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 2, 2018

I double-checked that I'm using the new JS by loading it in the browser, since dev tools seem to interfere with the banner:

The JS appears to be the new stuff. I still see issues going directly to the anchor link in FF Dev Edition 63.0b11, if I open in a new tab or window. The content immediately jumps to the heading, hidden behind the TOC.

In Chrome, it appears to work reliably in a tab or a window, starting at the top of the page and then scrolling down to the heading. It seems like the first time I tried in a new window in Chrome it failed (header was behind TOC), but I wasn't able to reproduce it.

I don't know if this is a browser difference, or a cold cache vs a warm one (FF Dev is my daily for development, Chrome is for Hangouts).

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 4, 2018

@jwhitlock Thanks for the review. Just to confirm, the issue below is the only remaining issue?

I still see issues going directly to the anchor link in FF Dev Edition 63.0b11, if > I open in a new tab or window. The content immediately jumps to the heading, >hidden behind the TOC.

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 4, 2018

I think so. Bug 1479817 is specifically about what happens when you share a link with an anchor, such as over email or stack overflow, so I'd like for it to show the header more reliably.

I also don't want other stuff to break:

  • TOC should be shown (minus issues around having dev tools open)
  • Clicking the link icon should load the anchor into the location bar, not hide the header
  • Clicking an anchor link in the content should go to that anchor, show the header
  • Going to a URL without a hash component should still work
@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 8, 2018

Hey @jwhitlock

I have tested this last scenario multiple times in Fx and Chrome and I cannot seem to reproduce the problem. For me it works as expected. See the videos below:

Firefox: https://cl.ly/9e4df462f7d5
Chrome: https://cl.ly/6e542d9b404c (It does a bit of a peekaboo but settles down correctly :))

And then just clicking around using the TOC in Fx
https://cl.ly/8d11bdad66af

@jwhitlock jwhitlock closed this Oct 8, 2018

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 8, 2018

Oops, did not mean to post that last comment, or close the PR.

@jwhitlock jwhitlock reopened this Oct 8, 2018

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 8, 2018

Here's the comment I meant to post, once I discovered that the height of the window mattered

I still get browser differences, and differences between short mode (Table of Contents is non-sticky) and tall mode (Table of Contents is sticky). The browser differences may be due to evaluation order of non-combined JS files, or if analytics.js is blocked. Here are my results:

  • Firefox - https://cl.ly/31154afd856e
  • Chrome - https://cl.ly/7a262e47b86a.
    • / Short mode, #Hash link: First link, smooth scroll from top of page to the correctly positioned header. On second and third links, smooth scroll, but header includes space for missing table of contents .
    • Short mode, Link without hash, then click Table of Contents: Smooth scroll to correctly positioned header.
    • Short mode, click on-page anchor link: Smooth scroll to header that includes space for missing table of contents.
    • Tall mode, #Hash link: Smooth scroll from top of page to the correctly positioned header. Same as @schalkneethling's results - https://cl.ly/6e542d9b404c
    • Tall mode, Link without hash, then click Table of Contents: Smooth scroll from top of page to the correctly positioned header
    • Tall mode, click on-page anchor link: Smooth scroll to the correctly positioned header

I think the issue with Tall mode (with sticky ToC) and direct hash links is going to be hard to reproduce, and is possibly a JS ordering issue. We should retest when it is on staging or production, but not block.

I think more work could be done to only include the Table of Content padding when the browser is tall enough for a sticky ToC, which reproduces for me across browsers. @schalkneethling, if you think that is in scope for this change, please try to reproduce and adjust the code.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 9, 2018

Thanks for the detailed feedback @jwhitlock . I am on it.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 9, 2018

@jwhitlock Whoah ok, so the calculation results by Chrome and Firefox are pretty different. Seeing that Chrome is the one that gets it right, I assume the problem lies with Fx. I am going to hunt down for a known bug, or other properties where the difference is not so extreme:

Firefox:

headingTop 1122.4833374023438 (Difference of ~95)
tocHeight 48
scrollY 1074.4833374023438 (Difference of ~96)

Chrome:

headingTop 1218
tocHeight 48
scrollY 1170

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1479817-anchor-links-heading-obscured-by-toc branch from 7b19af1 to ea6bbe0 Oct 10, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 10, 2018

Ok, phew. I reckon this does it @jwhitlock r?

@jwhitlock

All the tests work for me in Firefox and Chrome. Thanks @schalkneethling!

@jwhitlock jwhitlock merged commit 8ade76f into mozilla:master Oct 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No manifest changes detected

@schalkneethling schalkneethling deleted the schalkneethling:bug1479817-anchor-links-heading-obscured-by-toc branch Oct 11, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 11, 2018

w00t! Thanks for the thorough review @jwhitlock

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