Page MenuHomePhabricator

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

Description

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. https://en.wikipedia.org/wiki/Talk:Godwin%27s_Law
  • 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.

Details

Related Gerrit Patches:
mediawiki/extensions/MultimediaViewer : masterDo not set up the overlay on irrelevant hash changes
mediawiki/extensions/MultimediaViewer : masterCache JS loading logic

Event Timeline

Krinkle created this task.Nov 30 2015, 2:49 AM
Krinkle raised the priority of this task from to High.
Krinkle updated the task description. (Show Details)
Krinkle added a subscriber: Krinkle.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 30 2015, 2:49 AM
Tgr added a subscriber: Tgr.Nov 30 2015, 3:16 AM

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

https://gerrit.wikimedia.org/r/256644

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

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

https://gerrit.wikimedia.org/r/256644

Jdlrobson set Security to None.

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

https://gerrit.wikimedia.org/r/256841

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

https://gerrit.wikimedia.org/r/256841

Jdlrobson closed this task as Resolved.Dec 8 2015, 7:29 PM
Jdlrobson claimed this task.