Bug 98370

Created attachment 211203 [details] Patch This patch requires bug 70606

Comment on attachment 211203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211203&action=review > Source/WebCore/ChangeLog:10 > + No new tests are required because this feature will use an existing a11y I'm not a fan of using a11y to describe accessibility, since it's a colloquialism that may not be familiar to the casual observer > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:169 > + else if (notification == AXInvalidStatusChanged) we should think about changing this if/else chain to a switch statement in the future

Comment on attachment 211203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211203&action=review > Source/WebCore/dom/Document.cpp:2463 > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) You probably want to comment on bug 112003 to let them that EFL platform might be just a single step away from fixing that bug (that is, adding here PLATFORM(EFL)). Not saying that you add it now, though. > Source/WebCore/page/FrameView.cpp:1315 > +#if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) Ditto. > Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:109 > + if (!g_strcmp0(g_value_get_string(&paramValues[1]), "layout-complete")) I agree with Chris we might eventually switch into a "switch" statement, but in the meantime I would use here an "else if" for consistency with the rest of the code.

Created attachment 211209 [details] Patch Many thanks for the review. Modified the patch according to the comments.

Comment on attachment 211209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211209&action=review > Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:109 > + if (!g_strcmp0(g_value_get_string(&paramValues[1]), "layout-complete")) this should also be else if

Comment on attachment 211209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211209&action=review >> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:109 >> + if (!g_strcmp0(g_value_get_string(&paramValues[1]), "layout-complete")) > > this should also be else if Ops. I will fix it.

Created attachment 211210 [details] Patch Modified the patch according to the review.

Comment on attachment 211210 [details] Patch Rejecting attachment 211210 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 211210, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/1735567

Created attachment 211213 [details] Patch Mistakenly the line with the reviewer name was missing in the ChangeLog. Fixed it.

Comment on attachment 211213 [details] Patch Clearing flags on attachment: 211213 Committed r155456: <http://trac.webkit.org/changeset/155456>

All reviewed patches have been landed. Closing bug.

Comment on attachment 211213 [details] Patch Sorry for just now catching this, but.... + } else if (notification == AXLayoutComplete) + g_signal_emit_by_name(axObject, "state-change", "layout-complete", true); There's no such "state-change" event called "layout-complete." As a result, AT-SPI2 is now complaining, and Orca is becoming non-responsive. I will see if there's a way to detect and not get hung up over bogus events. In the meantime, could you please fix this? Assuming that layout complete is not the same thing as load complete, the immediate fix would be to not emit this signal at all -- at least not until the signal is added to ATK. But if layout complete is the same thing as load complete, then you could emit that signal on the document interface. See https://developer.gnome.org/atk/unstable/AtkDocument.html#AtkDocument.signals

Reopening because this "fix" just breaks stuff for ATs.

I'm "resolving" this bug again, as this issue will be tracked down by bug 122970 instead.

(In reply to comment #12) > [...] > Assuming that layout complete is not the same thing as load complete, the > immediate fix would be to not emit this signal at all -- at least not until > the signal is added to ATK. At the moment, the only AtkObject that implements AtkDocument is the one wrapping the webArea AccessibilityObject: // Document if (role == WebAreaRole) interfaceMask |= 1 << WAI_DOCUMENT; So, even though I agree AXLayoutComplete is not the same than AXLoadComplete (in WebCore's jargon), the signal AtkDocument::load-complete will serve well anyway to implement the needed stuff in DRT/WKTR that we need to pass this test. After all, the test is called "loading-iframe-sends-notification", doesn't have nothing to do with completing the layout process. > But if layout complete is the same thing as load complete, then you could > emit that signal on the document interface. See > https://developer.gnome.org/atk/unstable/AtkDocument.html#AtkDocument.signals It is not, but as I said above, the AtkDocument::load-complete signal will serve us well in this case. Denis is currently working in a patch now to do the right thing here, and will hopefully be ready today.