Handle appear events

[peterszerzo]

Also includes some refactoring that fixes a click but related to the once feature.

[jakub-nlx]

For appear on mutation observer, I'd imagine you want the sync version rather than waiting up to a second for the element to appear.

[jakub-nlx]

Suggested change

	mutations.forEach(async (mutation) => {
	const targets = await withElements(appearSteps);
	const targets = await withElements(appearSteps);
	mutations.forEach(async (mutation) => {

Seems like a performance win, no?

[jakub-nlx]

I'm not sure this is required at all. Don't we want basically the same logic like we do for URL events? If the DOM changed at all, and the query didn't succeed previously but now it did, then fire the appear event? Or perhaps more elaborately, we store the count of elements, then if the count is now larger than the old count we trigger the appear event?

[peterszerzo]

I don't see this the same way from a semantic perspective. If, by 'query succeeds', you mean both query and URL condition combined, then it can happen that the step fires even though no 'appear' took place, but purely from the technicality of the combined condition wasn't satisfied before but now is.

Practical example: we want to play something on voice when an error message first appears, as opposed to when the URL is finally compatible but the error had been around on the page for 2 minutes.

[jakub-nlx]

Well then pre-filter the array based on URL condition, then reset it when those change?

The above code as far as I read it just triggers when an element we are targeting has a child inserted into the DOM. That could be for all sorts of reasons and doesn't necessarily mean it "appeared". For one, we are not checking its visibility, so it might not actually be visible to the user at all.

Also, why the contains check? That could trigger an element that simply has changeable children?

[peterszerzo]

I had the condition backwards, just changed to addedNode.contains(element). This way we make sure if the user selected an element, but that is inserted as a result of its parent being inserted, then the event still fires. Otherwise, the user has to target the exact element that has been added to the DOM.

Appear intentionally does not care about visibility. There is a separate 'enters viewport' that takes care of that, and they are intentionally separate because sometimes an error notification is not visible, but you want to react to it immediately anyway.

At this point I'm tempted to say if you have a different implementation in mind, feel free to implement it as a separate PR, and we can compare.

[jakub-nlx]

I had the condition backwards, just changed to addedNode.contains(element).

OK, that I think alleviates most of my concern here.

Appear intentionally does not care about visibility.

I believe that isn't strictly true, since the accessibility queries do care about elements being visible (at least in the they don't have display: none set sense).

[peterszerzo]

I believe that isn't strictly true, since the accessibility queries do care about elements being visible (at least in the they don't have display: none set sense).

I'm ok with this edge case, but good to know for debugging.

[jakub-nlx]

Suggested change

return null;

not necessary

[peterszerzo]

Explicit returns read clearer to me.