...
<rdar://problem/115936550>
Created attachment 467835 [details] Patch
Comment on attachment 467835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467835&action=review > LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 > +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) This looks wrong
(In reply to chris fleizach from comment #3) > Comment on attachment 467835 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=467835&action=review > > > LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 > > +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) > > This looks wrong How so? It moves #foo-button to be a child of #treeitem2.
Comment on attachment 467835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467835&action=review >>> LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 >>> +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) >> >> This looks wrong > > How so? It moves #foo-button to be a child of #treeitem2. Shouldn't this JavaScript have been executed? This is the expected file
(In reply to chris fleizach from comment #5) > Comment on attachment 467835 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=467835&action=review > > >>> LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 > >>> +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) > >> > >> This looks wrong > > > > How so? It moves #foo-button to be a child of #treeitem2. > > Shouldn't this JavaScript have been executed? This is the expected file The test uses `evalAndReturn`, which executes the JavaScript and then returns it at as string so that we can include it in the output. I like it because it makes reading the expectation easier (you can see the JS alongside the expected effect to the accessibility tree). But I can remove it if you'd prefer.
Comment on attachment 467835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=467835&action=review >>>>> LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 >>>>> +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) >>>> >>>> This looks wrong >>> >>> How so? It moves #foo-button to be a child of #treeitem2. >> >> Shouldn't this JavaScript have been executed? This is the expected file > > The test uses `evalAndReturn`, which executes the JavaScript and then returns it at as string so that we can include it in the output. I like it because it makes reading the expectation easier (you can see the JS alongside the expected effect to the accessibility tree). But I can remove it if you'd prefer. Is this a pattern other WebKit tests are going in? Personally I think it makes expected output harder to read. I think main concern though is that we don't so this anywhere else, so why is this test case so special?
(In reply to chris fleizach from comment #7) > Comment on attachment 467835 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=467835&action=review > > >>>>> LayoutTests/accessibility/mac/tree-properties-update-after-dynamic-change-expected.txt:22 > >>>>> +document.getElementById('treeitem2').appendChild(document.getElementById('foo-button')) > >>>> > >>>> This looks wrong > >>> > >>> How so? It moves #foo-button to be a child of #treeitem2. > >> > >> Shouldn't this JavaScript have been executed? This is the expected file > > > > The test uses `evalAndReturn`, which executes the JavaScript and then returns it at as string so that we can include it in the output. I like it because it makes reading the expectation easier (you can see the JS alongside the expected effect to the accessibility tree). But I can remove it if you'd prefer. > > Is this a pattern other WebKit tests are going in? Personally I think it > makes expected output harder to read. I think main concern though is that we > don't so this anywhere else, so why is this test case so special? We use it in other accessibility tests, I invented it :). Andres has also expressed he doesn't like it, so I'll remove it from this patch and stop using it in future patches since that seems to be the consensus.
Created attachment 467837 [details] Patch
Committed 268385@main (3afbb0ed02b1): <https://commits.webkit.org/268385@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 467837 [details].