whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.04k stars 2.64k forks source link

Add option to Element.focus() to specify async application #6077

Open chrishtr opened 3 years ago

chrishtr commented 3 years ago

The current spec for Element.focus specifies (like other HTML APIs) that focus applies immediately, so that for example a call to document.activeElement reflects the new focused element.

While this is easy to understand and program to, it has the downside that if a developer calls focus(), the browser has no choice but to force-compute styles for all elements, or at the very least the elements that might be affected by the focus() call. This is bad because it can lead to style thrashing in code like this:

myElement.focus();
...
changeStyleState();

without an intervening update-the-rendering event loop task, because styles will in general have to be computed twice. (Sites these days have complex rendering machinery using frameworks, and it's often not feasible for them to make sure to change styles at just the right times to avoid this style thrashing.)

I propose to add an async option to FocusOptions. The behavior would be to run the focus algorithm steps at step 13 of update-the-rendering (which allows it to be placed after style recalc is done in a browser implementation).

focus() is commonly called on sites in order to improve accessibility during and after single-page-app transitions; facebook.com for example currently does this. @bgirard.

rniwa commented 3 years ago

Why can't the page simply schedule a callback with requestAnimationFrame to set the focus instead of calling focus()?

bgirard commented 3 years ago

1) If the code is already within requestAnimationFrame then you'll be doing your focus on the next (wrong) frame. This wont be frame perfect, may lead to an additional paint and may clobber other focus() in the next frame. 2) When your focus runs within requestAnimationFrame, there may be other requestAnimationFrame callbacks that will modify the DOM. If the focus rAF callback runs first it will recompute style for the focus call and once again after the other callbacks re-dirty the DOM.

emilio commented 3 years ago

Focus kinda needs two style updates somehow doesn't it? You need to know the styles pre-setting focus to know if the focus target is indeed focusable, and if it is, then focus can change styles itself (or styles of arbitrary elements) via the :focus / :focus-visible / :focus-within pseudo-classes. So what am I missing?

emilio commented 3 years ago

Not to say that applying focus async is not desirable of course, just that some kind of extra work seems mostly unavoidable. Gecko and I think WebKit too have fast paths to try to avoid updating the style too much for the "determine whether something is focusable" case.

chrishtr commented 3 years ago

Not to say that applying focus async is not desirable of course, just that some kind of extra work seems mostly unavoidable. Gecko and I think WebKit too have fast paths to try to avoid updating the style too much for the "determine whether something is focusable" case.

I agree that two style updates are needed: one to clean before running the algorithm to determine which element receives focus, and one to apply the focus. And as you say, browsers can and do optimize for this second style update, so a double render is indeed avoided. Note also that in the problematic situation leading to me filing this issue, it went like this:

  1. Script task dirties style
  2. Script task calls focus(). This computes styles from #1 and sets focus on an element
  3. Script task dirties style
  4. Update-the-rendering steps run. This computes styles, then re-runs a partial style recalc to apply focus styles to the focused element

If focus were run async, it'd be:

  1. Script task dirties style
  2. Script task calls focusAsync(). No style is computed. Focus is not yet applied to any element
  3. Script task dirties style
  4. Update-the-rendering steps run. This computes style, sets the focus on an element, and then re-runs a partial style recalc to apply focus styles to the focused element

This runs one full style recalc and one partial style recalc, whereas the sync version runs two full style recalcs.

bgirard commented 3 years ago

You need to know the styles pre-setting focus to know if the focus target is indeed focusable

That's an interesting point. Is it sensible to specify that the async focus call is ignored if the target is not focusable when resolved?

This would lead to the following problem: If you async focus A..N and N is not focusable, what do you do? Do you drop prior focus requests A..M? Do you keep a list of focus requests A..N? Do you walk through that list forward or backwards until you find a valid target?

emilio commented 3 years ago

This runs one full style recalc and one partial style recalc, whereas the sync version runs two full style recalcs.

I'm not sure I follow. I don't think the sync version would be particularly more expensive that the async one, for two reasons:

I guess in the case where the script tasks in (1) and (3) dirty intersecting set of styles, iff you don't have something like the WebKit optimization above, you might compute styles for a couple more elements than needed, but that's the only case that comes to mind... Maybe that's what you meant though?

That's an interesting point. Is it sensible to specify that the async focus call is ignored if the target is not focusable when resolved?

What does "when resolved" mean? You mean when the browser actually goes through the async focus requests? If so, yeah, I imagine that's a way it could work (I think that's how .focus() works too). But how does that solve the issue? If it is focusable, then you need to actually do all the work that changing focus entails, including doing the style recalc and so on.

Unless you mean that you check whether the element is focusable without updating style/layout? If so, I think that'd be bad, as that'd make stuff like:

let textarea = document.createElement("textarea");
document.body.appendChild(textarea);
textarea.asyncFocus();

Work or not depending on whether something updates the style of the page before asyncFocus is processed.

chrishtr commented 3 years ago

This runs one full style recalc and one partial style recalc, whereas the sync version runs two full style recalcs.

I'm not sure I follow. I don't think the sync version would be particularly more expensive that the async one, for two reasons:

  • Browsers already have pretty decent style invalidation to avoid recalculating styles of all elements, and per my comment above even that might not be needed all the time, see https://bugs.webkit.org/show_bug.cgi?id=208504 for example.

Yes, browsers do have such optimizations. The cost of the extra style recalc will depend on how much was invalidated. In general it is true that browsers can and do optimize to reduce the extra costs, but there is still a cost.

  • Even in the async focus case, you can't just restrict the recalc to the focused element sub-tree (which I think it's what you're suggesting in (4)), because we have pseudo-classes that propagate up the tree all the way to the root (like :focus-within), so you can technically change a lot of styles pretty much everywhere with a focus change. (Plus sibling combinators etc but those are less problematic).

I agree that the extra style recalc to apply the style of the focused element and other elements that depend on focus state might be expensive. But that won't be made any worse with this async proposal, no?

I guess in the case where the script tasks in (1) and (3) dirty intersecting set of styles, iff you don't have something like the WebKit optimization above, you might compute styles for a couple more elements than needed, but that's the only case that comes to mind... Maybe that's what you meant though?

Are you referring here to the style recalc done to apply styles affected by presence of focus? I.e. the "partial style recalc" I mentioned in step 4.

emilio commented 3 years ago

I agree that the extra style recalc to apply the style of the focused element and other elements that depend on focus state might be expensive. But that won't be made any worse with this async proposal, no?

No, not particularly.

Are you referring here to the style recalc done to apply styles affected by presence of focus? I.e. the "partial style recalc" I mentioned in step 4.

No, I mean something a bit different. So, if the two script tasks don't touch intersecting parts of the DOM, there shouldn't be much perf difference from recalculating style in between via sync focus(), right? At least I know Gecko and Blink track the closest common ancestor of everything that has changed in the DOM, to avoid doing lots of tree walking, so the amount of work you do if you have that optimization seems essentially the same.

The case where you'd waste work (and thus where asyncFocus() helps) is if they touch the same parts of the DOM, right? Where you'd style them twice, once as the style recalc .focus() triggers to see if the target is focusable, and once afterwards.

However, you could also build the opposite example, where the script task in (3) touches the DOM that .focus() changes the style of. In that case, the current ordering saves work, and asyncFocus would cause that part of the page to be restyled twice (once before asyncFocus() runs, due to the changes in (3), and once after, due to the application of the various focus pseudo-classes).

That is, I'm still having a bit of a hard time to find good examples where this helps, due to .focus() being fundamentally a read+write of the style tree. The cases where it helps that I can see are:

So whether this is worth depends on whether the cases this improves are more common than the cases it regresses IMO (with the usual cost of adding a new APIs etc factored in of course).

chrishtr commented 3 years ago

No, I mean something a bit different. So, if the two script tasks don't touch intersecting parts of the DOM, there shouldn't be much perf difference from recalculating style in between via sync focus(), right?

Correct. In that scenario, most of the time there would not be much difference in performance.

The case where you'd waste work (and thus where asyncFocus() helps) is if they touch the same parts of the DOM, right? Where you'd style them twice, once as the style recalc .focus() triggers to see if the target is focusable, and once afterwards.

Right.

However, you could also build the opposite example, where the script task in (3) touches the DOM that .focus() changes the style of. In that case, the current ordering saves work, and asyncFocus would cause that part of the page to be restyled twice (once before asyncFocus() runs, due to the changes in (3), and once after, due to the application of the various focus pseudo-classes).

Why would there be any restyling before asyncFocus is called? In your example, the performance would be about the same with asyncFocus as with focus. I think this is a special case of the case you mentioned above.

So whether this is worth depends on whether the cases this improves are more common than the cases it regresses IMO (with the usual cost of adding a new APIs etc factored in of course).

If you're thinking about aggregate improvement in average page interaction speed, or even expected improvements for many sites, I would agree with you. However, consider it from the page author's perspective: if you want to guarantee that your site has no performance problems of this kind, and asyncFocus was not available, then you have to do a bunch of special and fragile work to polyfill it.

Stepping back to the larger picture though: on a case-by-case basis your arguments can make some sense. After all, asyncFocus will only fix a tiny fraction of the total source of jank on the web, and adding it does have a cost. But architecturally, it makes sense for us to make progress towards a platform that has no forced style recalcs or layouts at all if used correctly, and one that is not hard to use correctly. This goal will allow sites to achieve reliable and understandable performance for their script tasks (*), and allow user agents maximum ability to optimize rendering speed on their behalf.

For these reasons, we should add asyncFocus.

(*) Otherwise they will somehow have to account for script task timing measurements that sometimes have significant browser work inside them.