Make accessibility/aria-hidden-false-works-in-subtrees.html async to pass in isolated tree mode.
<rdar://problem/89312156>
Created attachment 452898 [details] Patch
Comment on attachment 452898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452898&action=review > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:117 > + NSString *attributeName = [NSString stringWithJSStringRef:idAttribute]; > Convert the JSString to NSString on the main thread and capture the NSString to be used on the AX secondary thread. Why is it better to do it this way? > LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:98 > + setTimeout(async () => { Does this entire block need to be wrapped in async in order for the test to pass? It seems like this portion is testing static content, right? Though maybe there's enough static content such that this part of the test does need to be async. What do you think?
Created attachment 452912 [details] Patch
(In reply to Tyler Wilcock from comment #3) > Comment on attachment 452898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452898&action=review > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:117 > > + NSString *attributeName = [NSString stringWithJSStringRef:idAttribute]; > > > Convert the JSString to NSString on the main thread and capture the NSString to be used on the AX secondary thread. > Why is it better to do it this way? I believe JSStringRefs are conceived to be used on the main thread, so I think it may be safer to convert it to NSString on the main thread and then use the NSString off the main thread. Somebody with deeper knowledge of JSStringRef may say that they are just as good off the main thread as NSStrings, in which case this is a mute point, but I feel more comfortable doing it this way without knowing more intimately the threading behavior of JSStringRefs. Hopefully Darin Adler or somebody else may shed more light here. > > > LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:98 > > + setTimeout(async () => { > > Does this entire block need to be wrapped in async in order for the test to > pass? It seems like this portion is testing static content, right? > > Though maybe there's enough static content such that this part of the test > does need to be async. What do you think? It is necessary to have the whole thing async because that's the only way we keep the order in the outputs. In other words, to keep the same sequence of outputs between the main body and the unload function, they both need to be sync or async, but as soon as any part of them doesn't match the rest, then the outputs get intermingled. I think setTimeout dispatches to some other queue or thread, and if you don't queue everything that produces an output into the expected.txt in that same queue, the order of the output is undefined.
(In reply to Andres Gonzalez from comment #5) > (In reply to Tyler Wilcock from comment #3) > > Comment on attachment 452898 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=452898&action=review > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:117 > > > + NSString *attributeName = [NSString stringWithJSStringRef:idAttribute]; > > > > > Convert the JSString to NSString on the main thread and capture the NSString to be used on the AX secondary thread. > > Why is it better to do it this way? > > I believe JSStringRefs are conceived to be used on the main thread, so I > think it may be safer to convert it to NSString on the main thread and then > use the NSString off the main thread. Somebody with deeper knowledge of > JSStringRef may say that they are just as good off the main thread as > NSStrings, in which case this is a mute point, but I feel more comfortable > doing it this way without knowing more intimately the threading behavior of > JSStringRefs. Hopefully Darin Adler or somebody else may shed more light > here. > > > > > > LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:98 > > > + setTimeout(async () => { > > > > Does this entire block need to be wrapped in async in order for the test to > > pass? It seems like this portion is testing static content, right? > > > > Though maybe there's enough static content such that this part of the test > > does need to be async. What do you think? > > It is necessary to have the whole thing async because that's the only way we > keep the order in the outputs. In other words, to keep the same sequence of > outputs between the main body and the unload function, they both need to be > sync or async, but as soon as any part of them doesn't match the rest, then > the outputs get intermingled. I think setTimeout dispatches to some other > queue or thread, and if you don't queue everything that produces an output > into the expected.txt in that same queue, the order of the output is > undefined. Meant "onload function", not "unload function", spell checker playing tricks.
(In reply to Andres Gonzalez from comment #5) > (In reply to Tyler Wilcock from comment #3) > > Comment on attachment 452898 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=452898&action=review > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:117 > > > + NSString *attributeName = [NSString stringWithJSStringRef:idAttribute]; > > > > > Convert the JSString to NSString on the main thread and capture the NSString to be used on the AX secondary thread. > > Why is it better to do it this way? > > I believe JSStringRefs are conceived to be used on the main thread, so I > think it may be safer to convert it to NSString on the main thread and then > use the NSString off the main thread. Somebody with deeper knowledge of > JSStringRef may say that they are just as good off the main thread as > NSStrings, in which case this is a mute point, but I feel more comfortable > doing it this way without knowing more intimately the threading behavior of > JSStringRefs. Hopefully Darin Adler or somebody else may shed more light > here. > > > > > > LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:98 > > > + setTimeout(async () => { > > > > Does this entire block need to be wrapped in async in order for the test to > > pass? It seems like this portion is testing static content, right? > > > > Though maybe there's enough static content such that this part of the test > > does need to be async. What do you think? > > It is necessary to have the whole thing async because that's the only way we > keep the order in the outputs. In other words, to keep the same sequence of > outputs between the main body and the unload function, they both need to be > sync or async, but as soon as any part of them doesn't match the rest, then > the outputs get intermingled. I think setTimeout dispatches to some other > queue or thread, and if you don't queue everything that produces an output > into the expected.txt in that same queue, the order of the output is > undefined. Thanks for the explanation!
Created attachment 453454 [details] Patch
Committed r290673 (247944@main): <https://commits.webkit.org/247944@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453454 [details].