Fix/no shadow dom scrolling

[alifeee]

closes #82

I do not really like the implementation of this fix. It is not very simply written.

However, I could not think how to better do it.

Perhaps, we could use this.hasAttribute('no-shadow') to check for shadow DOM, then use either this.root or document based on result. This would be more code, but perhaps more understandable of why it is happening.

[zerodevx]

Thanks for your PR! The test looks good. As for the solution, I think we can just no-op the goto() function if the no-shadow attribute is set. This basically just falls back to native browser scrolling behaviour which should work in the light-dom context. Specifically here:

https://github.com/zerodevx/zero-md/blob/5a627f2a8f605efbc108ba7f3866eb910de9cf20/src/index.js#L151-L158

We can just no-op it:

if (id && !this.hasAttribute('no-shadow') { ... }

Which I think should resolve the problem.

[zerodevx]

Let me know if you're ok with making this change - happy to merge it in if the tests pass!

[alifeee]

Thanks for the suggestion, I will try it.

I think this change is okay. It is annoying that html works such that <zero-md> element exposes getElementById only if it is a shadow DOM root node. However, I don't think this will change anytime soon.

I would be happy for it to be merged. I verified the tests on my local machine. I was not sure how if they were verified via CI or something here?

Thanks for the review ♥

[alifeee]

sorry, I don't quite understand your suggested change. I understood it to be:

goto(id) {
    if (id && !this.hasAttribute('no-shadow')) {
      const el = this.root.getElementById(id.substring(1))
      if (el) {
        el.scrollIntoView()
      }
    }
  }

However, with this code: if the element is in light-DOM, there is simply no scroll. Thus, the tests fail. I may have misunderstood your request.

For now, I changed the equality a little so it's more obvious what's going on. Let me know if you have any further suggestions. Otherwise, this passes tests.

[zerodevx]

sorry, I don't quite understand your suggested change. I understood it to be:

Hey you understood it right - my bad, just realised my suggestion won't work. Your original fix works, but I thought perhaps it'll be better to refactor the goto() function instead:

goto(id) {
  let el
  try {
    el = this.root.querySelector(id)
  } catch {}
  if (el) el.scrollIntoView()
}

WDYT? If you feel it's ok, if you could commit the change I'll then merge this in.

[alifeee]

Hi, thanks for the update.

One thing I was missing is that: I did not realise that it was default browser behaviour to scroll to hashlinks, i.e., that <a href="#header">go!</a> would scroll to that header by default. However, this does not work with content loaded after the page load, as zero-md is.

Thanks for your code suggestion. I have changed the fix to be very simple: simply replace getElementById with querySelector. The latter is available on all DOM elements, so it works for light- and shadow-DOM.

One final remark is that: if you click a link outside the shadow-DOM, it does not navigate to that header. But, that makes sense why. e.g.,

    <a href="#basic-usage">go to basic usage</a>
    <zero-md src="https://raw.githubusercontent.com/zerodevx/zero-md/master/README.md"></zero-md>

does not work.

The tests pass.

[zerodevx]

LGTM - querySelector() needs to be wrapped in a try...catch else it might throw if the selector is invalid. I'll prob add it in after merge.

Thanks for your contribution! 🙏