Page MenuHomePhabricator

MMV breaks table of contents and other anchor links
Closed, ResolvedPublic


As of a few days I notice that quite often when clicking on sections in the table of contents, the viewport does not jump to the section in question.

The url in the browser addressbar is updated, and the page flickers very quickly, but ultimately (a fraction of a second later) ends up right back where it was.

I haven't been able to trace the issue to any specific cause, other than that the click ends up being hijacked by MMV code through a popstate handler which then wrongly decides to open the MMV black lightbox, and then immediately closes it again once it realises it isn't relevant. But in doing so it somehow prevents the event from going through to the native handling of jumping to the target section. on the page.

Steps to reproduce:

  • Open page that contains TOC and a thumbnail (one that doesn't opt-out of MMV.) E.g.
  • Click on an image. (e.g. the WikiWorld cartoon on the top right; note that the address now contains #/media/File:).
  • Close the MMV lightbox. (observe that the address has been restored to no longer contain #/media/File:).
  • Click on a section in the table of contents.

Actual result:

Native click handling first fires popstate (because of hash change), which is handled by MMV. This handler wrongly decides to open a lightbox, which is briefly visible but immediately closed by its internal promise handling when no relevant data can be displayed. After this, the the scroll offset remains unchanged, the section was not scrolled to.

Before MMV is interacted with (by clicking the image) the section links in the TOC work fine. But once an image has been clicked, all anchor links on the page are effectively broken.

Event Timeline

Krinkle raised the priority of this task from to High.
Krinkle updated the task description. (Show Details)
Krinkle added a subscriber: Krinkle.

The promise used by MultimediaViewerBootstrap.loadViewer() should be cached. I wonder how this ever worked...

I can reproduce this in Chrome but not Firefox.

Change 256644 had a related patch set uploaded (by Gergő Tisza):
Cache JS loading logic

Change 256644 abandoned by Gergő Tisza:
Cache JS loading logic

Wrong approach; loadViewer() sets up the overlay and that's sometimes needed (but sometimes harmful). Sigh.

Change 256841 had a related patch set uploaded (by Gergő Tisza):
Do not set up the overlay on irrelevant hash changes

Change 256841 merged by jenkins-bot:
Do not set up the overlay on irrelevant hash changes

Jdlrobson claimed this task.