Bug 98370 – [GTK] accessibility/loading-iframe-sends-notification.html is failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

accessibility/loading-iframe-sends-notification.html is failing on all GTK platforms.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Floading-iframe-sends-notification.html

[Denis Nomiyama (dnomi)]

Created attachment 211203 [details]
Patch

This patch requires bug 70606

[chris fleizach]

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

[Mario Sanchez Prada]

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.

[Denis Nomiyama (dnomi)]

Created attachment 211209 [details]
Patch

Many thanks for the review. Modified the patch according to the comments.

[chris fleizach]

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

[Denis Nomiyama (dnomi)]

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.

[Denis Nomiyama (dnomi)]

Created attachment 211210 [details]
Patch

Modified the patch according to the review.

[WebKit Commit Bot]

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

[Denis Nomiyama (dnomi)]

Created attachment 211213 [details]
Patch

Mistakenly the line with the reviewer name was missing in the ChangeLog. Fixed it.

[WebKit Commit Bot]

Comment on attachment 211213 [details]
Patch

Clearing flags on attachment: 211213

Committed r155456: <http://trac.webkit.org/changeset/155456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joanmarie Diggs]

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

[Joanmarie Diggs]

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

[Mario Sanchez Prada]

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

[Mario Sanchez Prada]

(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.