Problem/Motivation

We want to support custom anchor link smooth scrolling. It should work with direct links and also in-page links. Support for position fixed top elements should be taken into consideration (eg toolbar admin, sticky navigation, etc). It should be smooth/animated.

Proposed resolution

Remaining tasks

  • Test in IE11 - it is important JS is not throwing JS exception. If it can work without smooth animation that is good enough. If IE11 smooth scrolling support is important consider using jQuery animate scroll API to support old browsers. Make it an option which can be enabled if needed?
  • Figure what to do with window.scrollTo({top: 50}); hack

User interface changes

We are adding module settings page.

API changes

None.

Data model changes

We are adding bs_lib.settings configuration and schema for it.

Resources

CommentFileSizeAuthor
#22 custom-anchor-scrolling-3130742-21.patch15.78 KBpivica
#22 interdiff-3130742-21-22.txt904 bytespivica
#20 interdiff-3130742-19-20.txt3.62 KBpivica
#20 custom-anchor-scrolling-3130742-20.patch15.66 KBpivica
#19 custom-anchor-scrolling-3130742-19-17-interdiff.txt696 bytesmbovan
#19 custom-anchor-scrolling-3130742-19.patch15.11 KBmbovan
#17 custom-anchor-scrolling-3130742-17-15-interdiff.txt635 bytesmbovan
#17 custom-anchor-scrolling-3130742-17.patch15.15 KBmbovan
#15 custom-anchor-scrolling-3130742-15.patch15.05 KBpivica
#15 interdiff-3130742-14-15.txt3.16 KBpivica
#14 custom-anchor-scrolling-3130742-14.patch14.01 KBpivica
#13 interdiff-3130742-12-13.txt1.27 KBpivica
#13 custom-anchor-scrolling-3130742-13.patch14.01 KBpivica
#12 interdiff-3130742-10-12.txt5.29 KBpivica
#12 custom-anchor-scrolling-3130742-12.patch13.98 KBpivica
#10 interdiff-3130742-5-6.txt13.2 KBpivica
#10 custom-anchor-scrolling-3130742-6.patch13.71 KBpivica
#5 interdiff-3130742-4-5.txt8.97 KBpivica
#5 custom-anchor-scrolling-3130742-5.patch11.04 KBpivica
#4 custom-anchor-scrolling-3130742-4.patch6.13 KBpivica
#4 interdiff-3130742-3-4.txt2.06 KBpivica
#3 custom-anchor-scrolling-3130742-3.patch5.99 KBpivica
#3 interdiff-3130742-2-3.txt1.91 KBpivica
#2 custom-anchor-scrolling-3130742-2.patch5.53 KBpivica

Comments

pivica created an issue. See original summary.

pivica’s picture

pivica commented
Status: Active » Needs review
StatusFileSize
new custom-anchor-scrolling-3130742-2.patch5.53 KB

Here is a first patch. More testing is needed including IE11 and Safari.

pivica’s picture

pivica commented
StatusFileSize
new interdiff-3130742-2-3.txt1.91 KB
new custom-anchor-scrolling-3130742-3.patch5.99 KB

Added focus element on scroll and skipping of links with just a # sign.

pivica’s picture

pivica commented
StatusFileSize
new interdiff-3130742-3-4.txt2.06 KB
new custom-anchor-scrolling-3130742-4.patch6.13 KB

Need to reorder scrollTo and focus calls. Commented initial 50px scroll hack for now. Some minor refactoring.

pivica’s picture

pivica commented
StatusFileSize
new custom-anchor-scrolling-3130742-5.patch11.04 KB
new interdiff-3130742-4-5.txt8.97 KB

Added exclude_elements options, this also eliminated a need for custom 50px initial scroll hack :)
Added settings form.
Various refactorings.

Quite flexible now with supported options, hopefully, it can be adapted for most use cases.

This should be ready for real tests now.

pivica’s picture

pivica commented
Issue summary: View changes
pivica’s picture

pivica commented
Issue summary: View changes
pivica’s picture

pivica commented
Issue summary: View changes
pivica’s picture

pivica commented
Issue summary: View changes
pivica’s picture

pivica commented
StatusFileSize
new custom-anchor-scrolling-3130742-6.patch13.71 KB
new interdiff-3130742-5-6.txt13.2 KB

New patch, couple of bigger changes:

  • JS code is refactored, scrolling is split in two parts. This allow us to drop exclude_elements option.
  • We are correctly handling prevention of browser scrolling while loading of link with a hash.
  • We are detecting IE browsers and not continuing with initializations of behaviour - IE will work as before.
  • New option exclude_links is added to exclude some links hash on a page.
  • Handling of /#something hash links - case of hash links that are comming from Drupal menu.
  • Some small optimizations, fixes and comments improvements.

Tested this on a couple of browsers:

  • Chrome Linux works fine
  • Firefox Linux works fine
  • Chrome Android works fine
  • Safari Mac, works except smooth scrolling - Safari is missing smooth option for now https://caniuse.com/#search=scrollIntoView
  • IE11 - we are not loading JS code and falling back to default browser anchor link handling
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

For the list-like settings, why not store it as a list on config? Then the update function and initialization logic could work with a PHP array and you don't have to implode it again on form submit. Just set type: sequence and then nested type string in the config schema, something like this:

   foo:
      type: sequence
      label: 'Foo'
      sequence:
        type: 'string'
pivica’s picture

pivica commented
StatusFileSize
new custom-anchor-scrolling-3130742-12.patch13.98 KB
new interdiff-3130742-10-12.txt5.29 KB

> For the list-like settings, why not store it as a list on config?

Yeah, i forgot that one, good idea. Implemented.

Also added menu link definition.

pivica’s picture

pivica commented
StatusFileSize
new custom-anchor-scrolling-3130742-13.patch14.01 KB
new interdiff-3130742-12-13.txt1.27 KB

Fixed schema indentation.

pivica’s picture

pivica commented
StatusFileSize
new custom-anchor-scrolling-3130742-14.patch14.01 KB

Messed up indentation fix in previous patch, this one should be good.

pivica’s picture

pivica commented
StatusFileSize
new interdiff-3130742-14-15.txt3.16 KB
new custom-anchor-scrolling-3130742-15.patch15.05 KB

New patch is doing next things:

- More accurate calculation of fixed elements offset. Now we can consider overlapped fixed elements.
- Adding triggering of custom events bslib.anchorscroll.fastrscrollended and bslib.anchorscroll.smoothscrollended.
- Expose API window.BSLib.getFixedElementsOffset() method.

Last two we needed for some custom TOC functionality.

pivica’s picture

pivica commented
Status: Needs review » Needs work

Idea of two phase scrolling (fast and then smooth) is nice but I can imagine that not everybody will like it. We should introduce option for this so people can choose. The option can be named type and it can have 3 values:

- smooth
- fast
- fast then smooth (this is current default behavior)

Option 'smooth' means the whole scrolling is done smooth.

Option 'fast' means the whole scrolling is done fast (jump). This is the same as default browser anchor scroll behavior, except we are doing all calculations for fixed elements, etc.

We can default this new option to 'fast then smooth' or 'smooth'.

mbovan’s picture

mbovan commented
StatusFileSize
new custom-anchor-scrolling-3130742-17.patch15.15 KB
new custom-anchor-scrolling-3130742-17-15-interdiff.txt635 bytes

Added a check to exclude the library in the admin theme.

I am leaving the issue status to "Needs work" as per #16.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/bs_lib.module
@@ -21,7 +21,7 @@ function bs_lib_theme() {
   $anchor_scroll = \Drupal::config('bs_lib.settings')->get('anchor_scroll');
-  if ($anchor_scroll['enable']) {
+  if ($anchor_scroll['enable'] && \Drupal::theme()->getActiveTheme()->getName() !== \Drupal::config('system.theme')->get('admin')) {
     $attachments['#attached']['drupalSettings']['bs_lib']['anchor_scroll'] = $anchor_scroll;

why not use \Drupal::service('router.admin_context')->isAdminRoute(), that's much easier. and in theory sites don't need to have a different backend and frontend theme.

mbovan’s picture

mbovan commented
StatusFileSize
new custom-anchor-scrolling-3130742-19.patch15.11 KB
new custom-anchor-scrolling-3130742-19-17-interdiff.txt696 bytes

As discussed, I've had issues with \Drupal::service('router.admin_context')->isAdminRoute() returning FALSE on admin ajax requests and that was the reasoning for #17.

Since hook_page_attachments() is not invoked on ajax requests, updated the fix to use \Drupal::service('router.admin_context')->isAdminRoute() instead.

pivica’s picture

pivica commented
Status: Needs work » Needs review
StatusFileSize
new custom-anchor-scrolling-3130742-20.patch15.66 KB
new interdiff-3130742-19-20.txt3.62 KB

New patch is fixing some edge cases in getFixedElementsOffset() method, renames some functions and improve comments.

pivica’s picture

pivica commented

> We should introduce option for this so people can choose. The option can be named type and it can have 3 values

Tried to do this but it is not that easy, discovered some additional problems, moving this to #3167782: Scroll behaviors option.

pivica’s picture

pivica commented
StatusFileSize
new interdiff-3130742-21-22.txt904 bytes
new custom-anchor-scrolling-3130742-21.patch15.78 KB

We should check for empty fixed selectors because they produce bugs in the code.

  • pivica committed 8be751b on 8.x-1.x 
    Issue #3130742 by pivica, mbovan, Berdir: Custom anchor link scrolling...
pivica’s picture

pivica commented
Status: Needs review » Fixed

Merged finally, thank you all for your help.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.