Page MenuHomePhabricator

[Spike, 8hrs] Investigate using display locking when expanding/collapsing sections
Closed, ResolvedPublic0 Estimated Story PointsSpike

Description

A currently experiment API may help opening/closing sections feel smoother. We want to investigate it and report back to the API developers so that we ensure that it is something we benefit from when it is made stable.

description

This new browser feature is currently behind a flag in the latest Chrome stable. It allows to do large DOM updates and insertions while retaining control over when it is rendered. Furthermore, when the locked content is requested to be rendered, it can do so in a non-janky way, while still allowing other browser operations.

At the moment the DOM show/hide done by section toggling is blocking, and we have seen that it can cause significant jank when large sections are opened.

This new API is designed to solve specifically this problem. I think it warrants being experimented on right now, in order to give upstream feedback if it doesn't quite work in its current implementation for section toggling.

Note that it brings one major upside, which is that it would allow browser text search to search within collapsed content, automatically opening a collapsed section when a match is found inside of it.

Finally, here's the latest explainer for a new declarative version of this API: https://github.com/WICG/display-locking/blob/master/README.md

spike outcomes

  • a demo
  • a write up on performance impact
  • Is there anything about this api that needs improving?

Event Timeline

this looks pretty interesting! I hadn't heard of it before

Jdlrobson renamed this task from Investigate using display locking when expanding/collapsing sections to [Spike, 8hrs] Investigate using display locking when expanding/collapsing sections.Aug 28 2019, 3:24 PM
Jdlrobson assigned this task to ovasileva.
Jdlrobson triaged this task as High priority.
Jdlrobson updated the task description. (Show Details)
Jdlrobson subscribed.

Olga I think this would be a valuable use of our time but requires timely action from our side.

Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptAug 28 2019, 3:26 PM

@ovasileva could you provide context on your move to "triaged but future". Should I read this as meaning you don't think this is a valuable use of our time or to mean something else? https://phabricator.wikimedia.org/T230941#5446117

@ovasileva could you provide context on your move to "triaged but future". Should I read this as meaning you don't think this is a valuable use of our time or to mean something else? https://phabricator.wikimedia.org/T230941#5446117

I think it would be valuable (as noted by the priority), but I'd rather wait until most AMC tasks and icon tasks are completed before beginning further analysis/pulling it onto the board.

Jdlrobson set the point value for this task to 0.Sep 24 2019, 4:30 PM
ovasileva lowered the priority of this task from High to Medium.Sep 24 2019, 4:36 PM

The API has received a major update, to now be completely declarative, which makes it much easier to use. There is also an Origin Trial scheduled for Chrome 79. We should seize that opportunity to check that the implementation is useful for us and have a direct influence on the spec if it doesn't quite do what we need. Here's the full message from Chris Harrelson at Google (chrishtr@google.com):

I'd like to follow up and see if you are able to try out the Display Locking features as a way to improve performance of Wikipedia. We discussed this a while back in an comment thread on your design doc.

Since then, we have actually changed the API to be fully declarative and not require any javascript for your use case. All you would need to do is identify parts of the page that may be below the fold and put the rendersubtree attribute on an element that contains that part. e.g.

<div id=above-the-fold>
...
</div>

<div id=might-be-below-the-fold rendersubtree="invisible activatable">
...
</div>

<div id=might-be-below-the-fold rendersubtree="invisible activatable">
...
</div>

That's it!

When these divs & their subtrees are not actually visible in the viewport, Chrome will not render their subtrees and not do any work to lay them out or style them. If you want them to nevertheless take up placeholder space in the layout (e.g. to preserve scrolling and scrollbar behavior), you can add the content-size CSS property to the divs.

When Chrome loads the page, it will automatically determine which of these divs are on-screen and "activate" them, meaning it will set the rendersubtree attribute on those elements to the empty string. This will cause those subtrees to start rendering. Similarly, if after page load something causes those elements to come on the screen, the same will happen.

This feature is fully implemented behind the --enable-experimental-web-platform-features command-line flag of Chrome. We also plan to run an Origin Trial for this feature in Chrome 79, which will allow you to test impact at scale on Wikipedia.

Hi, Chris Harrelson here. (Thanks Gilles for inviting me to comment.)

As I mentioned, we are going to start an origin trial of this feature in Chrome 79. I think it would work well for the use cases of:

  • optimizing page load time
  • automatically enabling find-in-page, accessibility, tab/link navigation, and other browser functionality

I also think it should be very easy to play with. It's just setting a attributes on hidden/not-rendered DOM subtrees, plus a single CSS property for placeholder sizing.

I'd love to work with a Wikipedia engineer on a demo for these features.

Here is an example of auto-opening of collapsed sections when performing a find-in-page operation:

https://wicg.github.io/display-locking/sample-code/collapsed-sections.html

Try it with chrome://flags/#enable-experimental-web-platform-features turned on.

cc @Jhernandez this might be interesting for your mobile-html endpoint as it seems to be geared towards the progressive loading you are experimenting with.

@Chrishtr thanks for all the shares. if nobody beats me to it I'm hoping at experimenting with this on friday! Exciting stuff!

Jdlrobson added a subscriber: ovasileva.

Change 544236 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] POC: Use display locking

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

Took a look at this. The resulting patch https://gerrit.wikimedia.org/r/544236 was very straightforward and if this became a standard could easily be integrated into mediawiki.

A few notes:

  • It fixes the longstanding bug T216789 \o/
  • When closing a section again I found it a little unintuitive to set rendersubtree attribute to 'invisible skip-viewport-activation' I was setting it to invisible instead as I thought skip-viewport-activation was only an initial load instruction but that wasn't hiding the element
  • How do hash fragments work with this? I notice that if an element <div id="focusable" rendersubtree="invisible activatable"><div id="foo">Text</div>` was not visible when I navigated to #foo in the URL. We have a lot of code relating to this in the mobile site that checks the hash and then finds and reveals the appropriate element. I would love to offload this to the browser.
  • To avoid flash of unstyled content and accessible content in older browsers we would need to continue to ship styles that display: none or display: block these elements during load time. It would have been useful to have a way to avoid these styles for modern browsers via feature query.

e.g.

@supports not (something to do with rendersubtree ) {
 // we hide all section elements with display: none (but don't for modern browsers so that find on page continues to work
}

@Gilles not sure if you want to run any performance tests using https://gerrit.wikimedia.org/r/544236

Hi, Vlad here (working on the rendersubtree implementation).

It fixes the longstanding bug T216789 \o/

\o/

When closing a section again I found it a little unintuitive to set rendersubtree attribute to 'invisible skip-viewport-activation' I was setting it to invisible instead as I thought skip-viewport-activation was only an initial load instruction but that wasn't hiding the element

When we have "full activation", as in, we just specify rendersubtree=invisible, there are a number of ways the browser would make the element render. One of them is intersection with the viewport. This means that as soon as the element is on screen (but its subtree is hidden from view), we will issue a beforeactivate signal and make the subtree visible. That's why it seems that it's not possible to hide the element. skip-viewport-activation prevents this case, and only allows activation on non-viewport related functionality (such as find-in-page).

How do hash fragments work with this? I notice that if an element <div id="focusable" rendersubtree="invisible activatable"><div id="foo">Text</div>` was not visible when I navigated to #foo in the URL. We have a lot of code relating to this in the mobile site that checks the hash and then finds and reveals the appropriate element. I would love to offload this to the browser.

It should work more or less as you want it to. Specifically, again with rendersubtree="invisible skip-viewport-activation", hash fragment navigation would "activate" the element. For instance:

<!doctype HTML>

<div rendersubtree="invisible skip-viewport-activation">
  <div id=foo>Text</div>
</div>

<a href="#foo">link</a>

Clicking the link would render "Text". Also, once that's in the URL bar, refreshing the page would automatically activate and render "Text"

To avoid flash of unstyled content and accessible content in older browsers we would need to continue to ship styles that display: none or display: block these elements during load time. It would have been useful to have a way to avoid these styles for modern browsers via feature query.

I agree that it's a little awkward to do the feature detection right now. We're still looking at ways to improve this, so hopefully we'll have something more elegant in the future.

Based on local performance testing on an average sized article, the only gain I see is upon section expansion, by way of reducing the time spent on forced reflow due to T225946. This reduces the duration of the long task happening on click by 15-20% (but it's still a long task causing dropped frames).

When the forced reflow bug is fixed, though, the performance gains introduced by Display Locking might become negligible.

IMHO the main value at this point is fixing T216789 and mitigating T225946.

Thanks for doing the perf evaluation!

There are a couple of things to also consider:

  • We've also added an Element.updateRendering() method, which when called on a rendersubtree=invisible element causes updates to happen co-operatively, returning a promise. When the promise resolves, there should be a performance benefit expanding the section, since the reflow should be minimized. This would allow you to control when to cause the sections to update (ie, doing it before the user action to expand it). The co-operative part is meant to avoid janking the page while the updates happen, although admittedly our currently algorithm can be improved.
  • Without updateRendering(), the first time the section is expanded, I would imagine you would see similar performance to the current approach (with display: none). However, if the section is collapsed again, and expanded a second time, display: none would cause the same amount of work to happen (modulo resource fetches). With rendersubtree, it should retain as much of the state as it can, so if the section is expanded a second time, you should notice an improvement. This is probably not a very common case, but I thought I'd mention it anyway.

Change 544236 abandoned by Jdlrobson:
POC: Use display locking

Reason:
POC no longer needed.

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

Hi, just wanted to add that the rendersubtree attribute is now available as an Origin Trial in Chrome 79:

https://developers.chrome.com/origintrials/#/view_trial/3677189095748009985

This means you can experiment with it on your live site with real users. We'd love for you to try it and provide feedback!

nray updated the task description. (Show Details)

@nray @ovasileva @Jdlrobson what was the outcome in terms of decision-making and follow-up work on this topic? Is it documented somewhere?

@Gilles T230941#5587731 is essentially the outline. From my perspective if this is implemented and becomes a standard, it's a bit of a no brainer to use it to solve the find in page problem. I don't see any downsides - There is no risks to doing this given the backwards compatible nature of the change (https://gerrit.wikimedia.org/r/#/c/544236/ is a straightforward patch). Was there anything more specific than that you were looking for?

Just the tasks for the follow-up work, so that I can subscribe to them. But it sounds like they haven't been filed yet?

From the Chromium side, we were hoping that you would be willing to participate in the Origin Trial for this feature. This would mean that you would be able to deploy and test this feature with a subset of your users. In turn, this would allow us to verify the feature's usefulness in a "real world" situation. Is this feasible?

@Gilles T216789 would be the task to subscribe to.

@vmpstr I've asked our product manager @ovasileva to answer that specific question!

DannyS712 subscribed.

[batch] remove patch for review tag from resolved tasks

Change 544236 restored by Jdlrobson:
POC: Use display locking

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