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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃悶] Weird location.hash / window.history behaviour #6184

Open
ptzar80 opened this issue Apr 30, 2024 · 5 comments
Open

[馃悶] Weird location.hash / window.history behaviour #6184

ptzar80 opened this issue Apr 30, 2024 · 5 comments
Labels
COMP: runtime STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working

Comments

@ptzar80
Copy link

ptzar80 commented Apr 30, 2024

Which component is affected?

Qwik Runtime

Describe the bug

Issue Overview

This report outlines an observed inconsistency in the behavior of the <a> and Qwik Link elements when used for scrolling to a target element in a Qwik application. The behavior varies between local and Stackblitz environments, and also changes after a routeAction$ is invoked upon form submission.

<a> Element Behavior

The <a> element initially performs as expected, scrolling to the target element and opening the details element that contains the target when clicked. However, after a routeAction$ is triggered upon form submission, subsequent clicks on the <a> element have no effect. This behavior is consistent in both local and Stackblitz environments.

Qwik Link Element Behavior

The behavior of the Qwik Link element differs significantly between local and Stackblitz environments. In a local environment, the Link element, despite having the same href as the <a> element, does not respond to clicks even before form submission. Conversely, in the Stackblitz environment, the Link element functions as expected both before and after form submission.

Reproduction Steps

Both the local and Stackblitz applications share the same codebase, including the package.json. Detailed instructions for reproducing the issue are provided on-screen in both applications.

Reproduction

https://github.com/Sport-Thieme/qwik-scroll-issue

Steps to reproduce

Qwik Scroll Issue Demo

This repository contains a demo for the Qwik scroll issue. Follow the instructions below to clone and start the repository:

Cloning the Repository

  1. Open a terminal.

  2. Navigate to the directory where you want to clone the repository.

  3. Run the following command:

    git clone https://github.com/Sport-Thieme/qwik-scroll-issue

Starting the Repository

  1. Navigate into the cloned repository's directory:

    cd qwik-scroll-issue
  2. Install the dependencies:

    npm install
  3. Start the application:

    npm start

The application should now be running at http://localhost:5173.

Issue Description

This repository demonstrates an unexpected behavior in Qwik related to the <a> element and the Qwik Link element when used for scrolling to an element with a specific id.

<a> Element Behavior

The <a> element behaves as expected, scrolling to the target element and opening the details element that contains the target when clicked, until a routeAction$ is called upon form submission. After the form submission, clicking on the <a> element has no effect. This behavior is consistent both when running the application locally and on Stackblitz.

Qwik Link Element Behavior

The behavior of the Qwik Link element differs between running the application locally and on Stackblitz.

Local Environment

In a local environment, the Link element neither scrolls to the target element nor opens the details element, even before form submission.

Stackblitz Environment

On Stackblitz, the Link element behaves as expected, both before and after form submission. It scrolls to the target element and opens the details element when clicked.

Testing the Issue

Detailed instructions on how to test this issue will be displayed on the screen when you run the application.

System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 138.08 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.1 - ~/.nvm/versions/node/v18.17.1/bin/node
    Yarn: 1.22.21 - ~/.nvm/versions/node/v18.17.1/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.1/bin/npm
    pnpm: 9.0.5 - ~/.nvm/versions/node/v18.17.1/bin/pnpm
  Browsers:
    Chrome: 124.0.6367.92
    Safari: 17.4.1
  npmPackages:
    @builder.io/qwik: ^1.5.2 => 1.5.2 
    @builder.io/qwik-city: ^1.5.2 => 1.5.2 
    undici: * => 6.15.0 
    vite: ^5.1.6 => 5.2.10

Additional Information

Please, make sure to check the behaviour both on Stackblitz and by running the app locally after cloning it.

@ptzar80 ptzar80 added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Apr 30, 2024
@jordanw66
Copy link
Contributor

jordanw66 commented May 1, 2024

Interesting issue, when upgrading to SPA, QwikCity will hijack all same-page anchor tags because Firefox destroys history state when clicking hash anchors. It also only ever fires a hashchange event the first time and not again (Chrome does), so we can't detect this to fix it. So Qwik forces the anchors to go through the same pipeline as Link, which uses element.scrollIntoView().

Looks like this will only scroll to an element if it's already visible, it won't expand the details element.

The reason Link works on stackblitz is because stackblitz breaks the QwikCity router/useNavigate location and Link thinks it's navigating to a different website so it doesn't scroll, it uses location.href instead. This is why the favicon is requested again in the network tab when clicking it.

The only real solution to this issue I think is to use location.href = ... and let the browser perform its extra scrolling magic then fix the history state after. This works without reloading with Chromium-based browsers but would have to test to make sure it's the same elsewhere.

@tzdesign
Copy link
Contributor

tzdesign commented May 2, 2024

@jordanw66 thanks a lot. I was pretty sure stackblitz is reloading, but wasn't that smart to check network tab 馃樁鈥嶐煂笍

we will try with location.href even if this is a downgrade.

Can you point me in the right direction in qwik? Not sure if we could check if the element is visible and if not, use location.href in qwik instead?

@ptzar80
Copy link
Author

ptzar80 commented May 2, 2024

@jordanw66 it seems that anchor tag works before the first Form submission and it does scroll to the element before it is visible (opens the details element) - it's only Link that will not open the details element (and works if it's already open).

However, we tried setting

location.url.href = `${location.url.href}#content`

both in the onClick$ of the <a> and the Link and when logging the location.url.href after that, the href is really set, but it doesn't help the situation. Link still scrolls only if the details are open and the <a> still only opens and scrolls until the Form is submitted. If we're doing something wrong, could you please drop a few more details how to try this out so that we proceed in the right direction?

In the meantime we'll do a workaround with a custom context.

@jordanw66
Copy link
Contributor

jordanw66 commented May 2, 2024

So what's going on specifically, so there's no confusion, is that location.href = ... will scroll to an element and even expand any number of nested <details> as needed.

Regular <a> tags pretty much work the same way, in fact probably exactly because they also trigger a favicon load in network tab when clicked.

Before submitting the form, <a> works like normal. But <Link> doesn't, it uses element.scrollIntoView(). This fails when the details element is collapsed and the nested element is not visible.

When you submit the form, that forces an SPA reload, which upgrades the current page/history entry to SPA, as a result this now hijacks the same-page (hash/fragment) <a> and forces them into using pushState() and element.scrollIntoView(). (because of Firefox's functional shortcomings)

At this point <a> now no longer works to expand the details element. This happens even if you reload because the page has been upgraded to SPA, so Qwik reboots the SPA and re-hijacks these anchors immediately after a refresh.

The reason <Link> works on stackblitz is because it never manually scrolls, it's broken and always thinks it's navigating to a different website since it's comparing two cross-domain urls ("stackblitz..." vs "localhost"), so it always aborts early and calls location.href = ....

I think the issue you're having with onClick$ is because it's interacting with other handlers of <a> and <Link>. You can verify simply setting the location works because this is what <Link> is actually doing on stackblitz, but also you can just call it yourself in the console and it'll scroll correctly.

Lastly, I'd say real quick be careful with location.url.href = "${location.url.href}#content", if the location already has a fragment then this will just keep adding: ...#content#content#content..., etc.

If the page is already on the fragment, simply calling location.href = location.href will re-trigger the scrolling behavior.

Two solutions to this problem:

  • Replace element.scrollIntoView() with location.href = ... and fix the history state after. (the browser will insert its own history entry)
  • Check if the target element is not visible, and if not then use a selector to get every nested parent <details> element and open them.

First option will probably replicate natural browser behavior and quirks better, second is kind of hacky and probably has unforeseen edge cases. Just have to make sure whichever plays nicely with all browsers.

I'm on a phone so I'll post the relevant sections of code in a following comment.

@jordanw66
Copy link
Contributor

jordanw66 commented May 2, 2024

This is the code that scrolls for <Link> and hijacked same-page <a> tags:

https://github.com/QwikDev/qwik/blob/main/packages%2Fqwik-city%2Fruntime%2Fsrc%2Fscroll-restoration.ts#L4-L34

Because of the way Qwik/QwikCity works, there were some subtleties and also time constraints for me that required duplicating some code on refresh/re-initialization. I haven't had time to dedupe yet, so the following code scrolls for hijacked same-page <a> tags on an SPA page but only after a refresh:

https://github.com/QwikDev/qwik/blob/main/packages%2Fqwik-city%2Fruntime%2Fsrc%2Fspa-init.ts#L140-L182

Whatever solution needs to be copied to both locations for now.

(Look for getElementById and scrollIntoView to see the exact lines in the code blocks above that actually trigger what we're talking about)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: runtime STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants