Prevent infinite loop in revealing algorithms

[josepharhar]

The ancestor details revealing algorithm and the hidden until found revealing algorithm may both get stuck in infinite loops because they iterate the flat tree while firing synchronous events. If the page were to listen to these events and change the flat tree in response, then the algorithm could theoretically never end.

Fixes #11436

• At least two implementers are interested (and none opposed):
  • Chromium
  • WebKit
• Tests are written and can be reviewed and commented upon at:
  • Add beforematch infinite loop test web-platform-tests/wpt#53789
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/433545121
  • Gecko: Not sure if already implemented, will wait for WPT results
  • WebKit: Already implemented?
• Corresponding HTML AAM & ARIA in HTML issues & PRs: Not needed
• MDN issue is filed: Not needed
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/browsing-the-web.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )

[domenic]

Editorially LGTM, although some implementer review from @nt1m and/or @jnjaeschke would be appreciated.

[nt1m]

I think going through every single ancestor twice is very unefficient. The second loop should only go through relevant elements, rather than all of the ancestors again. I know it may result in some behavior difference in some very specific edge cases, but I'd rather have an efficient algorithm here.

[annevk]

Yeah, this seems like the right time to combine these algorithms so we only do a single tree walk. That's an observable change in behavior due to the events, but I think it's very much an acceptable one. And if we ever add more cases we care about we don't end up with ever more ancestor walks.

[nt1m]

Just to be clear, I was pointing out that the current PR collects all the ancestors into a list for both algorithms, instead of only the relevant ones for each algorithm, which is different from what Anne mentioned in the previous comment.

(But having 1 reveal algorithm instead of 2 separate ones as Anne suggests wouldn't be something I'm opposed to, if the change in order of events/revealing makes sense for web developers)

[annevk]

@nt1m and I discussed this some more and while in theory you could do a single ancestor walk and collect the nodes to dispatch events on in separate lists depending on the feature, my suspicion is that it would be more useful for web developers if the events are also collapsed into a single reverse-tree-ordered list.

[josepharhar]

Thanks, I combined the algorithms and only stored relevant nodes for the second traversal. How does it look?

[annevk]

After we fire the first event, do we want to check for each node we go through that it's still connected?

And maybe check that the pre-conditions are still valid?

And maybe that if any of that is no longer true, we return early as we're no longer sure what we're dealing with.

[josepharhar]

After we fire the first event, do we want to check for each node we go through that it's still connected?

And maybe check that the pre-conditions are still valid?

And maybe that if any of that is no longer true, we return early as we're no longer sure what we're dealing with.

I added checks to make sure that each ancestor to reveal is still connected and still has the open attribute if its a details or still has hidden=until-found otherwise, and stop the algorithm if any of those are not true

[annevk]

Looks pretty good. Couple of nits.

[nt1m]

I think you mean "If ancestorToReveal has the open attribute", though I don't think you want to early return either way here.

[annevk]

Why would we not want to return early if the circumstances have changed since we started collecting the nodes?

[nt1m]

Returning early means bailing out of the whole algorithm, including other details & hidden=until-found ancestors (e.g. not just the current details element).

[annevk]

Yeah I know. I suggested that above. I don't see a reason to continue if web developers have started mutating the tree to this extent.

[nt1m]

Not sure if Otherwise is right here, What if the details element also has the hidden=until-found attribute?

[josepharhar]

Ok, I changed the otherwise and some other conditions to account for this. How does it look?