Bug 40102 – WebCore EventHandler needs to take account of onLoad events fired before layout() complete

• Component: WebCore Misc.
• Status: CLOSED FIXED

[Kent Tamura]

editing/input/page-up-down-scrolls.html, which was introduced by r60591, fails on Qt Linux.

[Nico Weber]

Does webkit/qt/linux have sendEvent in its DRT? Are there instructions on how to build webkit/qt/linux somewhere? How is it normally handled when a new test fails only on a subset of ports?

[Csaba Osztrogonác]

pretty diff can be found here:
http://build.webkit.org/results/Qt%20Linux%20Release/r60591%20%2812729%29/editing/input/page-up-down-scrolls-pretty-diff.html

Yes, Qt-DRT has sendEvent, pageUp and pageDown should be handled.

This test fails only on Qt port, so it was a correct solution.
We usually file a bug and put the test into the skipped list 
as you did. And then somebody will try to fix the bug.

(If you would like to build QtWebKit on Linux, try to follow http://trac.webkit.org/wiki/BuildingQtOnLinux . But I'm not
sure if it is an absolutely perfect and up-to-date documentation.)

[Csaba Osztrogonác]

Simon, what do you think, is it a release blocker?

Or should we simple cherry-pick http://trac.webkit.org/changeset/60600
which add editing/input/page-up-down-scrolls.html to the Skipped list?

[Laszlo Gombos]

I do not think this is a release blocker - keeping it with P2 priority.

[Csaba Osztrogonác]

(In reply to comment #4)
> I do not think this is a release blocker - keeping it with P2 priority.
OK, but if it isn't a release blocker, we should cherry-pick 
http://trac.webkit.org/changeset/60600 to make buildbot happier.

Simon, could you do it?

[Robert Hogan]

This is failing because WebCore 'cheats' by firing the onLoad event before the FrameView has performed its final layout(). This is deliberate on the part of WebCore, from implicitClose() in Document.cpp (1862):

    // Make sure both the initial layout and reflow happen after the onload
    // fires. This will improve onload scores, and other browsers do it.
    // If they wanna cheat, we can too. -dwh

So at first glance you'd think Qt needs to ensure that it forces the layout before acting on onLoad events that need to know the correct layout, which is presumably what other ports do.

In the case of this test, QWebPagePrivate::handleScrolling() could always check that the layout is final before doing anything - but that's just one possible situation.

Qt doesn't know it's acting on an onLoad event so there doesn't appear to be a way of doing this in a generic that way will cover all cases.

This also seems to me like something WebCore should be looking after anyway - since it's the one that's 'cheating'. 

That said, I've no idea why this only fails for Qt - maybe it's something to do the way it queues events.

[Robert Hogan]

Created attachment 58591 [details]
Patch

[Robert Hogan]

(In reply to comment #7)
> Created an attachment (id=58591) [details]
> Patch

This patch makes WebCore take account of it's own 'cheating' on the firing of eventLoad handlers. I think this is the correct thing to do, it seems unfair to make ports worry about whether the layout is ready when processing onLoad events.

[Kenneth Rohde Christiansen]

It is a bit confusing that the patch says [Qt] and only touches WebCore code.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> It is a bit confusing that the patch says [Qt] and only touches WebCore code.

I was commenting on the WebCore changelog:

 5         [Qt] editing/input/page-up-down-scrolls.html failure
 6 
 7         https://bugs.webkit.org/show_bug.cgi?id=40102

Shouldn't this be

WebCore EventHandler needs to take account of onLoad events fired before layout() complete
https://bugs.webkit.org/show_bug.cgi?id=40102

instead?

[Robert Hogan]

(In reply to comment #10)
> (In reply to comment #9)
> > It is a bit confusing that the patch says [Qt] and only touches WebCore code.
> 
> I was commenting on the WebCore changelog:
> 
>  5         [Qt] editing/input/page-up-down-scrolls.html failure
>  6 
>  7         https://bugs.webkit.org/show_bug.cgi?id=40102
> 
> Shouldn't this be
> 
> WebCore EventHandler needs to take account of onLoad events fired before layout() complete
> https://bugs.webkit.org/show_bug.cgi?id=40102
> 
> instead?

I changed the bug title after uploading the patch!

[Robert Hogan]

Created attachment 58704 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 58704 [details]
Patch

Seems OK with me.

[Simon Hausmann]

This should be included in the release branch, as r60591 is also included and the test indeed fails on the release branch at the moment.

[Robert Hogan]

Committed r61353: <http://trac.webkit.org/changeset/61353>

[Alexey Proskuryakov]

>        WebCore 'cheats' by firing onLoad events before the frame's layout

Why are you saying so? Do we violate any specification?

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> >        WebCore 'cheats' by firing onLoad events before the frame's layout
> 
> Why are you saying so? Do we violate any specification?

Please read comment #6, it explains it

[Alexey Proskuryakov]

Thanks, that explains why ChangeLog said so. But it's not clear to me if this ancient comment is accurate.

[Simon Hausmann]

Revision r61353 cherry-picked into qtwebkit-2.0 with commit e32cb21d4f1787147bcb681883b96a95f867749a