Page MenuHomePhabricator

Speed up the time to page preview hovers by using a delegate event handler
Closed, ResolvedPublic2 Estimated Story Points

Description

Had a really interesting chat with @Krinkle just now.

He pointed out that in PagePreviews we wait for the document to be loaded, query the page to get all the links that can be hovered and then add a event handler to all those links.

We already do this, but only for the article content container.

The downsides of current approach:

  • The first-possible activation of page previews is artificially postponed until the entire page is ready (including below-the-fold images, and other scripts)
  • High cost to querying the DOM, creating and iterating a large array, and attaching many event listeners (one for each link).

Using jQuery event delegation on the /document/, we can avoid the need to add many event listeners by using only one (on the document object), and this approach also doesn't require waiting till the DOM is ready.

$(() => $('a').on('click', showHoverCard));
// wait for dom ready, find all <a>, implicit iterate, add event listener to each

becomes

$(document).on('click', '#mw-content-text a', showHoverCard);

This will make use of event bubbling to add a single event handler to the body element. When the event fires, it will look at ev.target and fire the event only if the element matches the selector.

This means a link that has been rendered before the document is ready will show a page preview right away.

More documentation here: http://api.jquery.com/on/

This seems like a no-brainer.

Acceptance criteria

  • Remove the mw.hook subscription
  • Set the .on listeners on document
  • Modify validLinkSelector to include the selector for the wikipage.content container

Event Timeline

^ @Krinkle hope I covered that well. Anything to add?

Jdlrobson added a project: Page-Previews.

@Jdlrobson Yep, just made a tiny change from $('body') to $(document) which avoids a few browser bugs and runs with even less overhead (avoiding jQuery.find, Sizzle, Sizzle.setDocument, regexes, getElementByTagName, toArray etc).

This means a link that has been rendered before the document is ready will show a page preview right away.

Only from the point in time where the page previews JS code is run, this won't help with links that might be rendered before that.

We discussed and I think we are already using event delegation as we bind the events on the content div, see https://github.com/wikimedia/mediawiki-extensions-Popups/blob/8721acbc77d940462739640685b8663e03da05fe/src/index.js#L237-L260

Jon will reach out to clarify if there is something else we've missed.

@Krinkle @Jhernandez pointed to me that we've already done this (albeit not on body):
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/index.js#L237

Do we need the mw.hook( 'wikipage.content' ) hook though?

The document (not the body) is available a lot sooner than a specific element, though. Scanning for $container elements forces you to wait for the whole DOM to be available.

AFAIK we use the containers because we don't want page previews to show out of the content area. We could bind to document and check ancestors on hover but that seems a bit wasteful.

This works:

$(document).on('click', '.myContainerClass a', showHoverCard);

Right, didn't think of that! 👍

So basically:

  • Remove the mw.hook subscription
  • Set the .on listeners on document
  • Modify validLinkSelector to include the selector for the wikipage.content container

So basically:

  • Remove the mw.hook subscription
  • Set the .on listeners on document
  • Modify validLinkSelector to include the selector for the wikipage.content container

Looks good to me.

Just to clarify, the mw.hook().add( $c => $.on( .. call is practically identical to $(function () { $( '#mw-content-text' ).on( ... This means that, while Popups did have the optimisation of not requiring an expensive selector query and not requiring a large O(N) for iterating and creating event listeners in memory, it did still wait for dom-ready before doing so (in fact, the hook actually fires slightly later than dom-ready).

The waiting part is the most important in terms of it starting to listen sooner. The other aspect instead reduce memory usage and reduce risk of temporarily freezing the browser, both of which Popups was already avoiding doing right.

Jdlrobson set the point value for this task to 2.Apr 10 2018, 4:17 PM

Change 425563 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Speed up the time to page preview hovers by using a delegate event handler

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

Change 425875 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Drop second requestIdleCallback call

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

pmiazga removed a project: Patch-For-Review.
pmiazga added a subscriber: pmiazga.

I tested this feature on Edge16, latest Firefox, latest Safari and latest Chrome - works as expected. But It's good to do one more sanity check for major browsers to prove that Page Previews work as expected.

Change 425563 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Speed up the time to page preview hovers

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

Change 425875 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Drop second requestIdleCallback call

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

Jhernandez claimed this task.

Everything seems ok.