Page MenuHomePhabricator

LivePreview fires wikipage.content hook before attaching content, which breaks some scripts
Closed, ResolvedPublic

Description

I have seen this problem with MathJax and TMH player. Both seem to do things that require the content to be attached. Both work the First time you use LivePreview (probably because of the delay of loading their modules first), but don't work the second time.

I see three solutions.
1: Move the firing of the hook to happen AFTER the content is added
2: add an additional hook
3: Rewrite 3rd party libraries to be able to deal with it. (though for mathjax which does lots of view measurements, that's probably never going to work).

Event Timeline

TheDJ raised the priority of this task from to Needs Triage.
TheDJ updated the task description. (Show Details)
TheDJ added a project: JavaScript.
TheDJ added a subscriber: TheDJ.

Or perhaps change their loader scripts to use mutation observers or something ? Not really used those before, perhaps VE team can advise on this, they probably have seen a lot of similar problems.

Mutation observers are not widely supported, and VE problems were always caused by them and never solved.

I think it would be sensible to attach before firing the hook.

For the record, there's a fairly simple workaround – just wrap the body of the hook handler function in setTimeout( function () { /* actual code here */ }, 0 ). This will run the code asynchronously as soon as the current execution (which is going to attach the elements) finishes. This is in use in mediawiki.page.gallery in core, for example.

Aklapper triaged this task as Medium priority.Feb 26 2015, 1:42 PM

I would lean towards solution 1) - attach before firing the hook.

wikipage.content has always been fired before attachment by all handlers other than the initial one (LivePreview and VisualEditor being the main ones; it seems VE was changed though I suspect unintentionally).

It's been documented as allowing to be fired both ways, but listeners must only rely on the content tree, not on position or attachment.

Relying on attachment was never supported so I'd recommend updating callers violating this contract. And in general, while in theory there are some use cases, in my experience most code that "depends" on content being attached doesn't conceptually depend on it but is just not written with it in mind. Rewriting it differently encourages taking performance into consideration and ensuring the first paint of the new content is correct from the start (instead of having every plugin cause a re-paint of the content area, which would have considerable performance and visual/UX impact).

Both TMH and MathJax have patches that fix this using solution 2 now.

Assuming that solution 2 has become dominant, anybody planning to work on this?

TheDJ claimed this task.

My patch for TMH is still awaiting review. The behavior described in this ticket is documented in the api, so I think we should just close this.