Fix accessibility for dialogs

[ASalavei]

Add delay to wait until all elements in semantic tree are finalised

Fixes https://youtrack.jetbrains.com/issue/CMP-6938/Accessibility-does-not-work-for-dialogs-with-platformLayers-enabled

Release Notes

Fixes - iOS

• Fix Accessibility Items availability inside dialogs

[MatkovIvan]

Since it's in the infinity loop looks probably ok-ish, but if we need a completion, it's probably better to wait for the next main thread iteration instead of "delay". @igordmn wdyt?

[igordmn]

Usually any delay without waiting for some specific thing is a hack. Only in a case we can't get access to something, we could use it.

Is it the case here? We don't have access to some completion information, because the system doesn't provide it? If so, can you describe it more?

If not, and we wait for some state inside Compose itself, we should replace this delay by an explicit wait, or "flush" needed changes manually and synchronously.

[igordmn]

I see that we wait for invalidations that come from Compose, so it seems that we have access to this information.

probably better to wait for the next main thread iteration instead of "delay"

invalidationChannel.receive() is suppose to already wait for the next main thread iteration. Quick check:

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.channels.BufferOverflow
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import org.jetbrains.skiko.MainUIDispatcher

fun main() {
    val invalidationChannel = Channel<Unit>(1, onBufferOverflow = BufferOverflow.DROP_LATEST)
    CoroutineScope(MainUIDispatcher).launch {
        while (true) {
            println("Wait receive")
            invalidationChannel.receive()
            println("Receive")
        }
    }
    CoroutineScope(MainUIDispatcher).launch {
        delay(1000)
        println("Send[")
        invalidationChannel.trySend(Unit)
        println("]Send")
    }
}

Wait receive
Send[
]Send
Receive
Wait receive

Also, even if we don't add a delay or additional suspend points like yield(), isn't that code should just handle all invalidations on the next iteration? If that is not the case with dialogs, it seems some updates are lost, and delay doesn't fix that, it just hides the issue.

Generally, all semantic updates should happen synchronously and fully in UI thread during ComposeScene methods: setContent, render, sendPointerEvent, sendKeyEvent, without any suspend points in between

[igordmn]

I looked at the code, and it seems we have this structure with Dialogs:

RootView
  MainView
    ComposeScene
      coroutineContext = FlushCoroutineContext(Dispatchers.Main)
  DialogView
    ComposeScene
      coroutineContext = FlushCoroutineContext(mainView.coroutineContext)

In this structure, when we render() MainView, we flush all effects for dialogs (see performScheduledEffects). But we also do that when we render() the DialogView itself.

It is an unexpected side effect of 2 our decisions:

• to manually flush effects during frame render()
• use the MainView coroutineContext as the parent for all created dialogs/popups

Not sure yet if this causes the bug. Perhaps we shouldn't use the parent coroutineContext, but use the root one. But only further investigation will help to answer that.

[ASalavei]

@igordmn , @MatkovIvan , it looks like Android recalculates semantics also on every onLayoutChange, which we're skipping now. Also it goes with some delay between recalculations. It does not mean we have to copy this behaviour because iOS accessibility engine may work in a different way.

About current situation - If we'd like to add the fix to the current release, I would go with some non-destructive solution that requires minimum testing. I would rather go with two things:

• Forcing tree recalculation if onLayoutChange called for unknown node - that means the new node may appear.
• Keeping minimal delay to mitigate number of tree reloads.
  PTAL.

[ASalavei]

Can we call invalidation on layout change?

@igordmn , I already did it. Please check the change.

[igordmn]

Thanks! Didn't notice it.

Then, can we remove delay? Batching itself should already work because of the suspend point in channel.receive. delay won't improve things.

A physical real delay also probably not the right optimisation if updates are heavy, because they still can cause frame losses, no matter the delay value.

[ASalavei]

@igordmn , I do agree that it may work without delay here - will remove it.
Since runloop triggers much faster than the frame rate, tree reload can occur several times per frame, but considering the capabilities of the iOS VoiceOver engine, it looks like the optimal reload rate is around 0.1-0.5 seconds. I still need to investigate this, so I'm happy to remove the delay for now.
But I still think that delay (basically throttling) is a very effective tweak, as it allows for significantly decrease unnecessary tree reloads, which is especially helpful during animations. So I would try to find optimal intervals to properly synchronise iOS without harming performance and VoiceOver responciveness.

[igordmn]

Since runloop triggers much faster than the frame rate

It shouldn't trigger much faster because channel.receive() is suspend, rendering is in the UI thread, and accessibility in the UI thread.

The only reason it may trigger faster because of the wrong usage of mainView.coroutineContext I mentioned above.

But I still think that delay (basically throttling) is a very effective tweak, as it allows for significantly decrease unnecessary tree reloads

If the updating is more than 16ms, then delay isn't the right option here (there will be visible frame drops)

If the updating is significantly less than 16ms and doesn't affect frame rate, then there is no point in delay, because it should only happen once per frame.

[igordmn]

delay isn't the right option here (there will be visible frame drops)

If updating is really heavy, then we should add more suspend points yield() in the iteration itself - it will fix the frame drops.

[igordmn]

I see that we wait for invalidations that come from Compose, so it seems that we have access to this information.

probably better to wait for the next main thread iteration instead of "delay"

invalidationChannel.receive() is suppose to already wait for the next main thread iteration. Quick check:

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.channels.BufferOverflow
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import org.jetbrains.skiko.MainUIDispatcher

fun main() {
    val invalidationChannel = Channel<Unit>(1, onBufferOverflow = BufferOverflow.DROP_LATEST)
    CoroutineScope(MainUIDispatcher).launch {
        while (true) {
            println("Wait receive")
            invalidationChannel.receive()
            println("Receive")
        }
    }
    CoroutineScope(MainUIDispatcher).launch {
        delay(1000)
        println("Send[")
        invalidationChannel.trySend(Unit)
        println("]Send")
    }
}

Wait receive
Send[
]Send
Receive
Wait receive

Also, even if we don't add a delay or additional suspend points like yield(), isn't that code should just handle all invalidations on the next iteration? If that is not the case with dialogs, it seems some updates are lost, and delay doesn't fix that, it just hides the issue.

Generally, all semantic updates should happen synchronously and fully in UI thread during ComposeScene methods: setContent, render, sendPointerEvent, sendKeyEvent, without any suspend points in between

[MatkovIvan]

Not blocking, but I'd love to see a test for that fix

[ASalavei]

@MatkovIvan , sure it will be done, we have this in plans