Bug 25831 – [GTK] events missing when a document is (re)loaded

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

When a document is (re)loaded, the following events should be emitted:

1) object:state-changed:busy with detail1 == 1

2) document:reload (Only if the existing document is being reloaded.)

.....page contents change.....

3) document:load-complete or document:load-stopped

4) object:state-changed:busy with detail1 == 0

See http://library.gnome.org/devel/atk/unstable/AtkDocument.html

In addition, the current implementation of WebKit replaces the top level object (ROLE_DOCUMENT_FRAME) with a new object when a page is loaded. When this occurs, it would be helpful if the following events were emitted:

1) object:state-changed:defunct emitted by the top level object/document frame only (i.e. This event should NOT be emitted for all of the AtkObjects from the old page as that will potentially bombard ATs with too many events.)

2) object:children-changed:remove emitted by the parent (ROLE_SCROLL_PANE) of the top level object.

[José Millán Soto]

Created attachment 52585 [details]
AtkDocument events implementation

This patch implements the emission of the signals object:state-changed:busy, document:reload, document:load-complete, document:load-stopped and object:state-changed:defunct.

[WebKit Review Bot]

Attachment 52585 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[José Millán Soto]

Created attachment 52590 [details]
AtkDocument events implementation

New version of the patch.
Sorry for the error in the Changelog file of the previous one.

[Joanmarie Diggs]

I just gave this a spin. Thanks for doing it!

Unfortunately, here's what I'm seeing in the terminal window from which I launched GtkLauncher:

(GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x968c768'

(GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x96776a0'
** Message: console message: http://googlevideo.blogspot.com/ @813: ReferenceError: Can't find variable: OnLoad


(GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x967ef40'

(GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0xb4b14968'

(GtkLauncher:22151): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.24.0/gobject/gsignal.c:3079: signal name `state-changed:defunct' is invalid for instance `0x9674a00'

==============================

Here's what I'm seeing in Accerciser's event monitor:

[document frame | ]
	application: [application | GtkLauncher]
[document frame | ]
	application: [application | GtkLauncher]
document:load-complete(0, 0, )
	source: [document frame | ]
	application: [application | GtkLauncher]
[document frame | ]
	application: [application | GtkLauncher]
[document frame | ]
	application: [application | GtkLauncher]

==============================

Here's what I see in Accerciser's event monitor when using Firefox:

document:reload(0, 0, Ubuntu Start Page)
	source: [document frame | Ubuntu Start Page]
	application: [application | Firefox]
document:load-complete(0, 0, Ubuntu Start Page)
	source: [document frame | Ubuntu Start Page]
	application: [application | Firefox]
document:load-complete(0, 0, foo - Google Search)
	source: [document frame | foo - Google Search]
	application: [application | Firefox]
document:load-complete(0, 0, Foobar - Wikipedia, the free encyclopedia)
	source: [document frame | Foobar - Wikipedia, the free encyclopedia]
	application: [application | Firefox]

The Accerciser output for WebKitGtk isn't just incomplete, something funky is taking place for events which are not document:load-complete. That's the biggest issue. It would be good to have the page title as the event.any_data, but if that's a pain it can be treated as a separate bug IMHO.

(Sorry for the bad news!)

BTW, José, if you're not familiar with Accerciser, I encourage you to get it. Best. Tool. EVER. Seriously. http://live.gnome.org/Accerciser

If Alejandro (API) has the time, he should be able to give you a crash course in its use. Otherwise, I will do my best to be around on IRC. And if you don't see me in #webkit-gtk, I might be hiding but still online. My nick on freenode is joanie.

Thanks again for your work!

[Xan Lopez]

Comment on attachment 52590 [details]
AtkDocument events implementation

Removing from the queue since the patch does not seem to work, need to work on it.

[Mario Sanchez Prada]

Created attachment 73716 [details]
AtkDocument events implementation

Attaching a new patch addressing the pending issues, based on Jose Millan's patch

[chris fleizach]

Comment on attachment 73716 [details]
AtkDocument events implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=73716&action=review

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:165
> +        return returnString(coreObject->document()->title());

only returning the title here limits some future flexibility, so as using aria-label, name or something else that might also be available

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:215
> +

don't know how you would get into this method to turn on AX, since the only caller first checks if AX is on before calling this method... see notifyStatus() change

[Mario Sanchez Prada]

(In reply to comment #7)
> (From update of attachment 73716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73716&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:165
> > +        return returnString(coreObject->document()->title());
> 
> only returning the title here limits some future flexibility, so as using aria-label, name or something 
> else that might also be available

My (probably poor) reasoning here was that no string was being returned for the object with the webArea role so that returning something like this would be at least an improvement (and very useful for emitting the signals along with this patch in the right way).

But most likely you're right and I should instead re-write this in a way that all those other possible situations were taken into account as well (perhaps moving this lines to the end of the function, as some kind of fallback in case no other string was found, or by defining a dedicated static function).

I'll come up with a new patch soon, just willing to share these thoughts now

> > WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:215
> > +
> 
> don't know how you would get into this method to turn on AX, since the only caller first checks if 
> AX is on before calling this method... see notifyStatus() change

My bad, sure. Thanks for pointing it out.

[Mario Sanchez Prada]

Comment on attachment 73716 [details]
AtkDocument events implementation

Removing r? flag since there are still some changes that need to be done

[Mario Sanchez Prada]

Created attachment 73881 [details]
AtkDocument events implementation

New patch, addressing the issues pointed out by Chris.

[Mario Sanchez Prada]

Created attachment 74398 [details]
AtkDocument events implementation + New unit test

(In reply to comment #10)
> Created an attachment (id=73881) [details]
> AtkDocument events implementation
> 
> New patch, addressing the issues pointed out by Chris.

Following Martin's suggestion via IRC, now attaching a new version of the patch, featuring a new unit test to carefully check that the related events are being emitted, and in the right order.

Asking for review over this one then...

[Xan Lopez]

Comment on attachment 74398 [details]
AtkDocument events implementation + New unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=74398&action=review

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:196
> +

Mmm, where is this used in the patch?

> WebKit/gtk/tests/testatk.c:316
> +    g_timeout_add(10000, (GSourceFunc)bailOutFromMainLoop, loop);

Uh, 10000? In general I don't like this thing of having a timeout "in case the test hangs". It should either pass or ASSERT somewhere IMHO. That being said, 10s is surely way too long.

[Mario Sanchez Prada]

(In reply to comment #12)
> (From update of attachment 74398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74398&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:196
> > +
> 
> Mmm, where is this used in the patch?

It's not explicitly used anywhere but this code allows that we get more complete information along with the signals issued by the webArea. 

In other words, without that code, we'd get things like this:

document:load-complete(0, 0, )
    source: [document frame | ]
    application: [application | GtkLauncher]


...but with the patch we'd get this:

document:load-complete(0, 0, Ubuntu Start Page)
    source: [document frame | Ubuntu Start Page]
    application: [application | GtkLauncher]

...as that information comes right from the atk_object_get_name() method executed over the AtkObject for the webArea object.

> > WebKit/gtk/tests/testatk.c:316
> > +    g_timeout_add(10000, (GSourceFunc)bailOutFromMainLoop, loop);
> 
> Uh, 10000? In general I don't like this thing of having a timeout "in case the test hangs". It should either pass or ASSERT somewhere IMHO. That being said, 10s is surely way too long.

Well, I don't really see the problem because the test would normally (actually all the times I tried it) run as quick as the reload process lasts (that is, way less than 10 secs, not even 1sec I'd say). The timeout is just a way to ensure the test won't hang, and I couldn't find any other better option for that.

Perhaps 10 secs is too much so we could lower that to 2-3 secs, that would look fine to me. But I do not see many other options here since we're dealing with asinchronous stuff... I'm open to suggestions, though :-)

[Xan Lopez]

(In reply to comment #13)
> Well, I don't really see the problem because the test would normally (actually all the times I tried it) run as quick as the reload process lasts (that is, way less than 10 secs, not even 1sec I'd say). The timeout is just a way to ensure the test won't hang, and I couldn't find any other better option for that.
> 
> Perhaps 10 secs is too much so we could lower that to 2-3 secs, that would look fine to me. But I do not see many other options here since we're dealing with asinchronous stuff... I'm open to suggestions, though :-)

My point is, either you know the test just has to finish, probably very fast, or not. If you don't, then there's something wrong and we should fix it, or make the test ASSERT in case it hits the problem. Otherwise we should be adding 10s timeouts everywhere just in case some test hangs... (FWIW the layout tests have this I believe, but it's done generally and not in each test separately).

[Mario Sanchez Prada]

Created attachment 74920 [details]
AtkDocument events implementation + New unit test

(In reply to comment #14)
> 
> My point is, either you know the test just has to finish, probably very fast, or not.
> If you don't, then there's something wrong and we should fix it, or make the test
> ASSERT in case it hits the problem. Otherwise we should be adding 10s timeouts
> everywhere just in case some test hangs... (FWIW the layout tests have this I
> believe, but it's done generally and not in each test separately).

After the discussion about this in the IRC, I just rewrote the unit test code in testatk.c not to use a new main loop nor a call to g_add_timeout(), using the g_test_trap_* API instead, where I set a 2secs timeout as guard for the unlikely case where the unit test would fail.

Attaching a new patch then

[Xan Lopez]

Comment on attachment 74920 [details]
AtkDocument events implementation + New unit test

Great stuff!

[Mario Sanchez Prada]

Committed r72764: <http://trac.webkit.org/changeset/72764>

[Mario Sanchez Prada]

Reopening as there seems to be a problem with the current patch, which was causing API crashes in the bots. The patch has already been rolled out through bug 50215.

My wild guessing up to this point is that could be something related to the fork done trhough the g_test_trap_fork() function, so perhaps it would be better to avoid that usage and replace it with some good-enough equivalent code to test this case (something like the previous version of th epatch).

But won't be able to tell for sure until I find a way to reproduce the problem locally

[Mario Sanchez Prada]

Looks like the problem is related to another (still unresolved) bug, which is currently being worked around through an "ASSERT(m_currentItem); if (!m_current_item) return;" combo:

    void HistoryController::restoreScrollPositionAndViewState()
    {
        [...]

        ASSERT(m_currentItem);
    
        // FIXME: As the ASSERT attests, it seems we should always have a currentItem here.
        // One counterexample is <rdar://problem/4917290>
        // For now, to cover this issue in release builds, there is no technical harm to returning
        // early and from a user standpoint - as in the above radar - the previous page load failed 
        // so there *is* no scroll or view state to restore!
        if (!m_currentItem)
            return;
    
        [...]
    }

Not sure how to proceed here then... I'll try to investigate the root issue to see if I can help on that FIXME, or just find another way to implement the unit test avoiding this code (whenever it's possible), if I can't find a better fix for the whole problem.

Btw, is there any way I can check rdar://problem/4917290 ?

[Mario Sanchez Prada]

Depending on bug 50331, reported to track the issue about the failing ASSERT

[Eric Seidel (no email)]

Attachment 74920 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.

[Mario Sanchez Prada]

(In reply to comment #21)
> Attachment 74920 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.

Yes, I'm just waiting for bug 50331 to be resolved before committing this one, since it's depending on that.

[Mario Sanchez Prada]

Removing dependency from bug 50331 since it's very likely that, as soon as bug 54198 gets fixed, we could write a Layout test for this patch instead of an unit test (which was what was actually being blocked by bug 50331)

[Mario Sanchez Prada]

Created attachment 82118 [details]
Patch proposal + Layout Tests

Attaching a new patch that replaces the old unit test with a layout test, now that we have bug 54198 fixed and no longer depend on bug 50331

Asking for review again then. It should be pretty straightforward, as the fix itself hasn't changed a single line.

[Xan Lopez]

Comment on attachment 82118 [details]
Patch proposal + Layout Tests

Looks pretty good to me, great that we can use layout tests for this now.

[Mario Sanchez Prada]

(In reply to comment #25)
> (From update of attachment 82118 [details])
> Looks pretty good to me, great that we can use layout tests for this now.

Yeah, I definitely agree with this as well.

Thanks for the quick review

[Mario Sanchez Prada]

Committed r78331: <http://trac.webkit.org/changeset/78331>

[WebKit Review Bot]

http://trac.webkit.org/changeset/78331 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/dynamic/view-overflow.html
fast/dynamic/window-scrollbars-test.html
fast/frames/flattening/frameset-flattening-simple.html
fast/overflow/clip-rects-fixed-ancestor.html
fast/repaint/fixed-child-move-after-scroll.html
fast/repaint/fixed-child-of-fixed-move-after-scroll.html
fast/repaint/fixed-move-after-scroll.html
fast/repaint/fixed-tranformed.html
fast/repaint/fixed.html
scrollbars/custom-scrollbar-with-incomplete-style.html
transforms/2d/transform-fixed-container.html

[Mario Sanchez Prada]

Reopening as per rollout done through bug 54295

[Mario Sanchez Prada]

Comment on attachment 82118 [details]
Patch proposal + Layout Tests

Clearing up flags as this patch caused some problems and needed to be rolled out (see bug 54295)

[Mario Sanchez Prada]

Created attachment 82645 [details]
Patch proposal + Layout test

(In reply to comment #29)
> Reopening as per rollout done through bug 54295

Attaching new patch, which does not interfere with other tests as it was pointed out in comment 28.

Seems like we can't call to gtk_widget_get_accessible() from notifyAccessibilityStatus() in FrameLoaderClientGtk.cpp, as that will call AXObjectCache::rootObject() which will call getOrCreate() over m_document->view, which by that time might be not ready.

I worked around this by directly calling to getOrCreate over the contentRenderer for the frame currently in use, which returned the valid AtkObject as well (and proof for this is that the returned object implements the AtkDocument interface which only happens for that single object of interest).

Hence, eagerly asking for review :-)

[Martin Robinson]

Comment on attachment 82645 [details]
Patch proposal + Layout test

Let's give this another shot!

[Mario Sanchez Prada]

Committed r78715: <http://trac.webkit.org/changeset/78715>