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

docs: Fix documentation and browser back button issue #5716

Closed
wants to merge 2 commits into from

Conversation

pittyplatsch
Copy link

I couldn't find any other issue or discussion thread but I'm new to Node and therefore spending a lot of time with the docs. It's quite annoying that the browser back button is not working when one has jumped to an anchor within a page and then wanting to go back to the index with the browser back functionality. Removing the suggested line fixes the issue. I don't really know what it's good for, so if anyone could tell me and propose a different fix I'd be happy too.

@evanlucas
Copy link
Contributor

Thanks for the contribution! This actually breaks scrolling completely for me in Chrome on OS X, so I think we will have to find a different solution.

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Mar 15, 2016
@pittyplatsch
Copy link
Author

@evanlucas Indeed, for me (FF on Ubuntu) too. Didn't even notice that, was just happy that the browser behaviour was as expected.

@pittyplatsch
Copy link
Author

Some more changes. Scrolling now functional again, sorry for my earlier ignorance.

@Fishrock123
Copy link
Contributor

cc @nodejs/documentation

@silverwind
Copy link
Contributor

Looks to be working in Firefox, Chrome and Safari on OS X. Will test on Windows later.

@jasnell
Copy link
Member

jasnell commented Mar 16, 2016

LGTM

@silverwind
Copy link
Contributor

@pittyplatsch tested in IE10, Chrome, Firefox on Windows and noticed one regression: scrolling the sidebar all the way to the bottom in Chrome (all platforms) and IE "bubbles" the scroll to the main section.

@pittyplatsch
Copy link
Author

@silverwind I see what you mean but don't think it's unnatural or unexpected behaviour that the scrolling continues in the main div of the page if an end is reached in the navigation div. Having the back button not working however is unexpected.

@silverwind
Copy link
Contributor

@pittyplatsch FWIW, the scrolling issue was reported as a bug here, I don't really want to introduce it again. Is it not possible to fix both issues?

@silverwind
Copy link
Contributor

Maybe something like this is in order to prevent the scroll "bubbling". The DOMMouseScroll isn't strictly necessary as only Firefox uses that event and it isn't affected right now, but I guess it won't hurt.

function scroll(e) {
  var delta = (e.type === 'mousewheel') ? e.wheelDelta : e.detail * -40;
  if (delta < 0 && (this.scrollHeight - this.offsetHeight - this.scrollTop) <= 0) {
    this.scrollTop = this.scrollHeight;
    e.preventDefault();
  } else if (delta > 0 && delta > this.scrollTop) {
    this.scrollTop = 0;
    e.preventDefault();
  }
}
document.getElementById('column2').addEventListener('mousewheel', scroll);
document.getElementById('column2').addEventListener('DOMMouseScroll', scroll);

@stevemao
Copy link
Contributor

cc @nodejs/website

@stevemao
Copy link
Contributor

@pittyplatsch interesting. I didn't know css position would break history buttons with anchor links. Any references on this?

@pittyplatsch
Copy link
Author

@silverwind Thanks for linking nodejs/nodejs.org#513 – interesting insights. I personally have no problem with the bubbling, on FF there's a delay which seems like a nice compromise. I too found people discussing using JS to influence the browsers default behaviour (http://stackoverflow.com/questions/5802467/prevent-scrolling-of-parent-element) and also browser developers are discussing it: https://bugs.chromium.org/p/chromium/issues/detail?id=162179. Personally I don't think it's necessary.

@stevemao Only came across it in this particular case. It drives me crazy and I was surprised it hasn't been mentioned before by someone else.

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

What's the status on this one?

@silverwind
Copy link
Contributor

silverwind commented Apr 21, 2016

I'd sign it off if a solution to the scroll bubble issue is provided, be it #5716 (comment) or (preferably) a CSS only solution.

@pittyplatsch
Copy link
Author

Sorry, I won't fix the bubbling. Take it as it is or feel free to close it...

@silverwind
Copy link
Contributor

In that case, I'll have to close it as it fixes one issue, but regresses another. The issue is not forgotten, thought. I have a few minor tweaks to docs CSS in mind, and I'll try to work out something regarding this problem.

@silverwind silverwind closed this Apr 22, 2016
silverwind added a commit to silverwind/node that referenced this pull request May 11, 2016
Moved the sidebar to a fixed position and moved the positioning of the
main content to the page's body, which results in back/forward
navigation through hash links and search highlight working again.

Fixes: nodejs#6637
Based on: nodejs#5716
silverwind added a commit that referenced this pull request May 15, 2016
Moved the sidebar to a fixed position and moved the main column to the
page's body, which results in back/forward navigation through hash
links and search highlight working again.

Fixes: #6637
Fixes: #6751
Based on: #5716
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
silverwind added a commit to silverwind/node that referenced this pull request May 15, 2016
Moved the sidebar to a fixed position and moved the main column to the
page's body, which results in back/forward navigation through hash
links and search highlight working again.

Fixes: nodejs#6637
Fixes: nodejs#6751
Based on: nodejs#5716
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2016
Moved the sidebar to a fixed position and moved the main column to the
page's body, which results in back/forward navigation through hash
links and search highlight working again.

Fixes: #6637
Fixes: #6751
Based on: #5716
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
evanlucas pushed a commit that referenced this pull request May 17, 2016
Moved the sidebar to a fixed position and moved the main column to the
page's body, which results in back/forward navigation through hash
links and search highlight working again.

Fixes: #6637
Fixes: #6751
Based on: #5716
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Moved the sidebar to a fixed position and moved the main column to the
page's body, which results in back/forward navigation through hash
links and search highlight working again.

Fixes: #6637
Fixes: #6751
Based on: #5716
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants