Bug 62126 – [Qt] fast/dom/HTMLDocument/hasFocus.html failed

• Component: WebKit Qt
• Status: RESOLVED INVALID

[qi]

Title say everything.

[qi]

Created attachment 96090 [details]
patch

[Chang Shu]

Comment on attachment 96090 [details]
patch

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

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:176
> +    m_webPage->d->page->focusController()->setFocusedFrame(0);    

Since ChromeClientQt::focus() does not call setFocusedFrame(), it's unlikely the best place to reset focused frame is in this unfocus() function.
It seems to me a better place would be QWebPagePrivate::focusOutEvent or somewhere in QWebFrame. You can try if it works. :)

[qi]

In this case, ChromeClientQt::unfocus() was called by "window.blur()", QWebPagePrivate::focusOutEvent is not touched. Currently, the code only clear the focus for qwidget, I think it should also notify the FocusController.

[Chang Shu]

QtTestBrowser works ok, right? You don't have to call setFocusedFrame directly. The event handling mechanism should get it done. I think the problem is in DRT. Probably my patch http://trac.webkit.org/changeset/88461 will provide a clue. It's a fix to WTR but DRT may have the similar issue.

[qi]

I am looking into qt code to find why view->clearFocus() in ChromeClientQt doesn't issue a "FocusOut" event which suppose to trigger focusController to reset focuse frame. 

What I found is: in QWidget::clearFocus(), it found it doesn't have focus! (line 6500 in qwidget.cpp). The reason why the QWidget doesn't have focus is because QWidget will call QApplicationPirvate::setFocusWidget (line 6423 in qwidget.cpp) to set itself as focus widget, in QApplicationPrivate::setFocusWidget, it will find the focus widget is in hidden status then it will save the widget into hidden_focus_widget in QApplication (line 2187 in qapplication.cpp), if the focus widget is visible, it will be saved into focus_widget instead of hidden_focus_widget (line 2197 in qapplication.cpp) . When we call hasFocus() in qwidget.cpp which will call QApplication::focusWidget (line 6331 in qwidget.cpp) to find focus widget, which only return QApplicationPrivate::focus_widget. So, for our case, our view is hidden, even we set it to focus, we will never find it has focus!

Not sure anybody has a idea about it?

[Alexis Menard (darktears)]

Comment on attachment 96090 [details]
patch

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

> Source/WebKit/qt/ChangeLog:8
> +        Clean the FocusController when the windows lose the focus.

Clear is better than clean :D. Typo -> "when the window loses"

>> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:176
>> +    m_webPage->d->page->focusController()->setFocusedFrame(0);    
> 
> Since ChromeClientQt::focus() does not call setFocusedFrame(), it's unlikely the best place to reset focused frame is in this unfocus() function.
> It seems to me a better place would be QWebPagePrivate::focusOutEvent or somewhere in QWebFrame. You can try if it works. :)

Yep, the view get the focus cleared therefore the page will receive a focusOutEvent, sounds like a better place. focusController->setFocusedFrame(QWebFramePrivate::core(mainFrame)); is done in the focusInEvent, perhaps we should clear it in the focusOutEvent.

[Antonio Gomes]

Note that you should do informal reviews :)

"Making unofficial reviews before you become a reviewer is encouraged. This is an excellent way to show your skills. Note that you should not put r+ nor r- on patches in such unofficial reviews."

http://www.webkit.org/coding/commit-review-policy.html

[qi]

Created attachment 97298 [details]
patch2

Because the QWidget will not able to send out FocusIn or FocusOut event when it is hidden (See comment#5), I let ChormeClientQt send the event out. I put the code only for DumpRenderTree because I don't want to change the browser behavior.

[Alexis Menard (darktears)]

Comment on attachment 97298 [details]
patch2

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

otherwise LGTM

> Source/WebKit/qt/ChangeLog:9
> +        manually send out FocusOut event when it is hidden. And same thing for setFocus.

"When QWidget in hidden status" -> When QWidget is in a hidden status. "manually send" is enough, you don't need "out". "And same thing for setFocus" -> could be a bit better  like "The same applies for setFocus". I'm nitpicking.

[Robert Hogan]

Comment on attachment 97298 [details]
patch2

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

As long as you fixed that nit when landing, I would r+ this.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:67
> +#include "qapplication.h"

Better to use <QApplication>

[qi]

Created attachment 103623 [details]
patch3

Under the latest revision, this bug is not reproducible anymore, I remove this test case from skip list.

[qi]

Created attachment 103625 [details]
patch3

Sorry, update wrong patch.

[Chang Shu]

Comment on attachment 103625 [details]
patch3

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

> LayoutTests/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

remove this line before landing.

[qi]

Created attachment 103627 [details]
patch4

Modify based on comments.

[Chang Shu]

Comment on attachment 103627 [details]
patch4

LGTM

[WebKit Review Bot]

Comment on attachment 103627 [details]
patch4

Rejecting attachment 103627 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 1

Last 500 characters of output:
s/API/qt/qdesktopwebview.cpp
	M	Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp
	M	Source/WebKit2/ChangeLog
r92851 = e9115cc6db9aa5f59ab997c94d77b9288193c9e0 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9351233

[qi]

Created attachment 103631 [details]
patch5

Resend patch and put "Chang Shu" as reviewer.

[WebKit Review Bot]

Comment on attachment 103631 [details]
patch5

Clearing flags on attachment: 103631

Committed r92858: <http://trac.webkit.org/changeset/92858>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

I don't know what's going on here. How many patches have you finally landed from here? By the way the hasFocus test failed on the Szeged bots (x86_64-Release, 4.8, WK2) so I skipped it again in http://trac.webkit.org/changeset/92869.

[Balazs Kelemen]

*** Bug 66076 has been marked as a duplicate of this bug. ***

[Chang Shu]

(In reply to comment #20)
> I don't know what's going on here. How many patches have you finally landed from here? By the way the hasFocus test failed on the Szeged bots (x86_64-Release, 4.8, WK2) so I skipped it again in http://trac.webkit.org/changeset/92869.

Balazs, is the test failing on WK2 only? Then we should put it into WK2 skip list. We can reuse this bug for the WK2 failure, but we should change the title.

[qi]

(In reply to comment #20)
> I don't know what's going on here. How many patches have you finally landed from here? By the way the hasFocus test failed on the Szeged bots (x86_64-Release, 4.8, WK2) so I skipped it again in http://trac.webkit.org/changeset/92869.

I just landed one patch, because of the commit failed I repatched it. You said it failed on WK2, why you revert the skip file for WK1?

[Balazs Kelemen]

(In reply to comment #23)
> (In reply to comment #20)
> > I don't know what's going on here. How many patches have you finally landed from here? By the way the hasFocus test failed on the Szeged bots (x86_64-Release, 4.8, WK2) so I skipped it again in http://trac.webkit.org/changeset/92869.
> 
> I just landed one patch, because of the commit failed I repatched it. You said it failed on WK2, why you revert the skip file for WK1?

No. I sad it failed on the x86_64-Release, the Qt-4.8 and the WK2 bots. Sorry if I was not clear. These bots are hosted at http://build.webkit.sed.hu/waterfall. I still don't get the picture. You just landed the patch that removes the test from the skipped list? Or did you also land something that actually fix the issue?

[qi]

I just landed the patch that removes the test from the skipped list.

[qi]

It is weird, my workspace and Chang's workspace still pass this case. Since I can't reproduce this bug, my original patch(patch2) can't be landed.

[Balazs Kelemen]

(In reply to comment #26)
> It is weird, my workspace and Chang's workspace still pass this case. Since I can't reproduce this bug, my original patch(patch2) can't be landed.

Probably you could reproduce the bug in a 64 bit environment.

[Chang Shu]

I don't think we need the workaround in patch2. The original problem is either a DRT issue or Qt issue. Since the bug is showing up in DRT only, I would lower down the priority or even mark this bug as FIX LATER.

[Eric Seidel (no email)]

Comment on attachment 103625 [details]
patch3

Cleared Chang Shu's review+ from obsolete attachment 103625 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 103627 [details]
patch4

Cleared Chang Shu's review+ from obsolete attachment 103627 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.