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

x/pkgsite: jump to "x" close button broken #52502

Closed
tallclair opened this issue Apr 22, 2022 · 5 comments
Closed

x/pkgsite: jump to "x" close button broken #52502

tallclair opened this issue Apr 22, 2022 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/frontend Issues related to pkgsite HTML/CSS/JavaScript and frontend development pkgsite

Comments

@tallclair
Copy link

What is the URL of the page with the issue?

https://pkg.go.dev/bytes@go1.18.1, but I think all packages have this problem.

What is your user agent?

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.127 Safari/537.36

Screenshot

Screenshot from 2022-04-22 10-45-26

What did you do?

Click the "Jump To" input in the left panel, click the X button in the top-right of the popup

What did you expect to see?

The popup closes.

What did you see instead?

Nothing happens. No errors captured in the js console.

The close button (bottom right) works.

@gopherbot gopherbot added this to the pkgsite/unplanned milestone Apr 22, 2022
@jamalc
Copy link

jamalc commented Apr 22, 2022

We've seen this report before (#50895), I'm finding this hard to reproduce. Does it happen consistently for you, or are you able to provide detailed steps?

It might be less work to reimplement these components than track this down. They were created before widespread browser support for the HTML dialog element and make use of dialog polyfill that is no longer necessary.

@jamalc jamalc self-assigned this Apr 22, 2022
@jamalc jamalc added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 22, 2022
@tallclair
Copy link
Author

Hmm, it seems to be hit-or-miss when navigating from another page, and also seems to work when I close a tab and reopen it (ctrl-shift-T), but refreshing the page seems to break it pretty reliably.

In a tab where it works, a breakpoint at line 43 of modal.ts is hit:

    for (const btn of this.el.querySelectorAll<HTMLButtonElement>('[data-modal-close]')) {
      btn.addEventListener('click', () => {
        if (this.el.close) {      // <----- breakpoint here is hit when clicking close
          this.el.close();
        } else {
          this.el.removeAttribute('opened');
        }
      });
    }

But on a tab that dosen't work that breakpoint isn't hit.

I also put a breakpoint in the constructor that registers that event handler. When refreshing the page, the constructor isn't called, but when doing a hard refresh (ctrl-shift-R) it is (and subsequently the button works).

Seems like maybe a race condition, but I haven't been able to narrow it down any more.

ZekeLu added a commit to ZekeLu/pkgsite that referenced this issue Apr 23, 2022
When the js file is cached in the browser, it's very likely that
it will be executed before the DOM is ready. In this case, the
returned elements of a query is not the expected ones most of the
time. So the js code that accesses elements should be delayed
until the page is fully loaded.

Fixes golang/go#52502
@gopherbot
Copy link

Change https://go.dev/cl/401914 mentions this issue: static/frontend: do not access elements until the page is loaded

@ZekeLu
Copy link
Contributor

ZekeLu commented Apr 23, 2022

I can reproduce the issue like this:

  1. Access the page https://pkg.go.dev/bytes@go1.18.1 once so that https://pkg.go.dev/static/frontend/frontend.js is cached.

  2. Simulate a slow network with the DevTools (make sure the cache is not disabled) and refresh the page. Most likely that the following code will be executed before the elements .js-modal exist. So that the event handler for the close button is not added.

    for (const el of document.querySelectorAll<HTMLDialogElement>('.js-modal')) {
      new ModalController(el);
    }

I have sent a PR to fix this issue by delaying the execution until the page is fully loaded.

@jamalc
Copy link

jamalc commented Apr 25, 2022

Thank you for looking into this!

@hyangah hyangah added the pkgsite/frontend Issues related to pkgsite HTML/CSS/JavaScript and frontend development label May 20, 2022
@rsc rsc unassigned jamalc Jun 22, 2022
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/frontend Issues related to pkgsite HTML/CSS/JavaScript and frontend development pkgsite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants