Bug 25831 - [GTK] events missing when a document is (re)loaded
Summary: [GTK] events missing when a document is (re)loaded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on: 30964 50215 54198 54295
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-15 21:08 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2011-02-16 09:13 PST (History)
8 users (show)

See Also:


Attachments
AtkDocument events implementation (5.09 KB, patch)
2010-04-05 15:52 PDT, José Millán Soto
no flags Details | Formatted Diff | Diff
AtkDocument events implementation (5.10 KB, patch)
2010-04-05 17:44 PDT, José Millán Soto
xan.lopez: review-
Details | Formatted Diff | Diff
AtkDocument events implementation (6.96 KB, patch)
2010-11-12 01:46 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
AtkDocument events implementation (7.12 KB, patch)
2010-11-15 03:41 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
AtkDocument events implementation + New unit test (13.82 KB, patch)
2010-11-19 09:22 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
AtkDocument events implementation + New unit test (11.99 KB, patch)
2010-11-26 04:31 PST, Mario Sanchez Prada
xan.lopez: review+
Details | Formatted Diff | Diff
Patch proposal + Layout Tests (11.59 KB, patch)
2011-02-11 03:56 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal + Layout test (11.93 KB, patch)
2011-02-16 08:47 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-05-15 21:08:53 PDT
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.
Comment 1 José Millán Soto 2010-04-05 15:52:29 PDT
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.
Comment 2 WebKit Review Bot 2010-04-05 15:56:19 PDT
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.
Comment 3 José Millán Soto 2010-04-05 17:44:05 PDT
Created attachment 52590 [details]
AtkDocument events implementation

New version of the patch.
Sorry for the error in the Changelog file of the previous one.
Comment 4 Joanmarie Diggs (irc: joanie) 2010-05-10 21:05:41 PDT
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!
Comment 5 Xan Lopez 2010-05-13 01:10:19 PDT
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.
Comment 6 Mario Sanchez Prada 2010-11-12 01:46:07 PST
Created attachment 73716 [details]
AtkDocument events implementation

Attaching a new patch addressing the pending issues, based on Jose Millan's patch
Comment 7 chris fleizach 2010-11-12 14:01:14 PST
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
Comment 8 Mario Sanchez Prada 2010-11-15 02:28:51 PST
(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.
Comment 9 Mario Sanchez Prada 2010-11-15 02:29:47 PST
Comment on attachment 73716 [details]
AtkDocument events implementation

Removing r? flag since there are still some changes that need to be done
Comment 10 Mario Sanchez Prada 2010-11-15 03:41:10 PST
Created attachment 73881 [details]
AtkDocument events implementation

New patch, addressing the issues pointed out by Chris.
Comment 11 Mario Sanchez Prada 2010-11-19 09:22:16 PST
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...
Comment 12 Xan Lopez 2010-11-19 18:28:30 PST
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.
Comment 13 Mario Sanchez Prada 2010-11-20 06:31:23 PST
(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 :-)
Comment 14 Xan Lopez 2010-11-23 15:20:54 PST
(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).
Comment 15 Mario Sanchez Prada 2010-11-26 04:31:32 PST
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
Comment 16 Xan Lopez 2010-11-26 06:02:59 PST
Comment on attachment 74920 [details]
AtkDocument events implementation + New unit test

Great stuff!
Comment 17 Mario Sanchez Prada 2010-11-26 08:19:03 PST
Committed r72764: <http://trac.webkit.org/changeset/72764>
Comment 18 Mario Sanchez Prada 2010-11-30 04:47:14 PST
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
Comment 19 Mario Sanchez Prada 2010-12-01 02:01:22 PST
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 ?
Comment 20 Mario Sanchez Prada 2010-12-01 13:11:33 PST
Depending on bug 50331, reported to track the issue about the failing ASSERT
Comment 21 Eric Seidel (no email) 2010-12-14 15:12:15 PST
Attachment 74920 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.
Comment 22 Mario Sanchez Prada 2010-12-15 01:12:35 PST
(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.
Comment 23 Mario Sanchez Prada 2011-02-10 06:05:29 PST
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)
Comment 24 Mario Sanchez Prada 2011-02-11 03:56:57 PST
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.
Comment 25 Xan Lopez 2011-02-11 04:46:57 PST
Comment on attachment 82118 [details]
Patch proposal + Layout Tests

Looks pretty good to me, great that we can use layout tests for this now.
Comment 26 Mario Sanchez Prada 2011-02-11 04:52:00 PST
(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
Comment 27 Mario Sanchez Prada 2011-02-11 04:54:06 PST
Committed r78331: <http://trac.webkit.org/changeset/78331>
Comment 28 WebKit Review Bot 2011-02-11 05:58:21 PST
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
Comment 29 Mario Sanchez Prada 2011-02-11 11:13:59 PST
Reopening as per rollout done through bug 54295
Comment 30 Mario Sanchez Prada 2011-02-11 11:15:06 PST
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)
Comment 31 Mario Sanchez Prada 2011-02-16 08:47:25 PST
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 :-)
Comment 32 Martin Robinson 2011-02-16 08:51:15 PST
Comment on attachment 82645 [details]
Patch proposal + Layout test

Let's give this another shot!
Comment 33 Mario Sanchez Prada 2011-02-16 09:13:00 PST
Committed r78715: <http://trac.webkit.org/changeset/78715>