Bug 40641 - [Qt] Clicking button input does not give it focus
Summary: [Qt] Clicking button input does not give it focus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-06-15 15:34 PDT by Viatcheslav Ostapenko
Modified: 2010-11-09 07:20 PST (History)
7 users (show)

See Also:


Attachments
Fix for the html elements not focused by mouse. (4.80 KB, patch)
2010-06-15 15:42 PDT, Viatcheslav Ostapenko
tonikitoo: review-
Details | Formatted Diff | Diff
Updated layout test. (6.24 KB, patch)
2010-10-13 13:20 PDT, Viatcheslav Ostapenko
tonikitoo: review-
Details | Formatted Diff | Diff
mac specific expected result (262 bytes, text/plain)
2010-10-14 06:24 PDT, Antonio Gomes
no flags Details
Added mac specific test result. (6.42 KB, patch)
2010-10-14 08:23 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Remove fast/events/click-focus-control.html from windows Skipped list (1.19 KB, patch)
2010-11-01 11:07 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2010-06-15 15:34:01 PDT
This bug https://bugs.webkit.org/show_bug.cgi?id=22261 for Qt only.
Comment 1 Viatcheslav Ostapenko 2010-06-15 15:42:32 PDT
Created attachment 58831 [details]
Fix for the html elements not focused by mouse.
Comment 2 Antonio Gomes 2010-07-01 13:58:24 PDT
Usually when you fork a bug like this, you use the CLONE BUG function of bugzilla, so other people in the original bug get notified.

Lgtm, anyways.
Comment 3 Andreas Kling 2010-07-11 15:27:19 PDT
(In reply to comment #2)
> Lgtm, anyways.

Mind r+ing it then with your new powers? :-)
Comment 4 Antonio Gomes 2010-07-11 21:13:10 PDT
Comment on attachment 58831 [details]
Fix for the html elements not focused by mouse.

Hi Viatcheslav. As I said before, patch looks Ok to me. It is a shame that it had to be spun off from bug 22261, where a more general solution was about to take place, but it does not sound as a big deal to me. I look forward for a morecross-port change involving this behavior, but we can make it happen for Qt earlier, yes.

About the patch I'll r- is mainly for two reasons:

- you are adding your own 'log' function. Please use debug function available at LayoutTests/fast/js/resources/js-test-pre.js

- you are adding expected results for Qt only, but not skipping the added test for any other platform (e.g. mac, gtk, chromium). My advice is:

1)  the results should be the same for Qt and Gtk, so please add expected results for Gtk as well, equal to Qt's one;

2) Also make your output such that it is possible to add a general failing result for all other ports where the behavior will remain as is today.
At least for Mac, it seems like the behavior they like to have according to ap: https://bugs.webkit.org/show_bug.cgi?id=22261#c17

The rest of the patch looks fine.
Comment 5 Antonio Gomes 2010-10-13 06:19:30 PDT
I will work on the tests for this, and of course keep Viatcheslav's name on ChangeLog.
Comment 6 Viatcheslav Ostapenko 2010-10-13 13:20:42 PDT
Created attachment 70651 [details]
Updated layout test.
Comment 7 Antonio Gomes 2010-10-14 06:23:21 PDT
Comment on attachment 70651 [details]
Updated layout test.

Patch looks almost perfect to go, but we will need one more round.

Apple guys do not usually skip tests unless their DRT can not run the test. Instead the run the test and check-in the expected result, even if it contains "expected failures". r- due to this.

To make it easier for you, I ran your test case on Mac's DRT and got the failing output. I will attach it to this bug. Please amend it as part of our patch, put it in LayoutTests/platform/mac/fast/events/click-focus-control-expected.txt. On windows we could possibly use the same strategy, but I am not sure if it will produce the exactly same expected result file. We can maybe keep it skipped for now.
Comment 8 Antonio Gomes 2010-10-14 06:24:01 PDT
Created attachment 70733 [details]
mac specific expected result
Comment 9 Viatcheslav Ostapenko 2010-10-14 08:23:33 PDT
Created attachment 70738 [details]
Added mac specific test result.
Comment 10 Antonio Gomes 2010-10-14 08:25:33 PDT
You probably want it to block bug 44677, so it get cherry-picked to branch 2.1
Comment 11 Eric Seidel 2010-10-14 11:44:03 PDT
Why the win file skip?
Comment 12 Antonio Gomes 2010-10-14 11:55:47 PDT
(In reply to comment #11)
> Why the win file skip?

If we have the output produced by a windows box we could just use it (of course we can always just grab it from the windows bot).
Comment 13 Antonio Gomes 2010-10-17 05:19:08 PDT
Viatcheslav , you want the commit-queue to land it automatically for you?

If so, set cq?
Comment 14 Viatcheslav Ostapenko 2010-10-17 06:30:59 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Why the win file skip?
> 
> If we have the output produced by a windows box we could just use it (of course we can always just grab it from the windows bot).

I think somebody, who cares about windows builds should decide the correct behavior and modify focusing as GTK and QT does, or choose Mac variant of test result.
Comment 15 WebKit Commit Bot 2010-10-17 11:19:15 PDT
Comment on attachment 70738 [details]
Added mac specific test result.

Clearing flags on attachment: 70738

Committed r69925: <http://trac.webkit.org/changeset/69925>
Comment 16 WebKit Commit Bot 2010-10-17 11:19:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Ademar Reis 2010-10-22 13:40:17 PDT
Revision r69925 cherry-picked into qtwebkit-2.1 with commit baa0e2d <http://gitorious.org/webkit/qtwebkit/commit/baa0e2d>
Comment 18 Antonio Gomes 2010-10-29 22:32:44 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Why the win file skip?
> > 
> > If we have the output produced by a windows box we could just use it (of course we can always just grab it from the windows bot).
> 
> I think somebody, who cares about windows builds should decide the correct behavior and modify focusing as GTK and QT does, or choose Mac variant of test result.

@aroben, what do you think? Should we unskip it and check in known/expected failing results?
Comment 19 Adam Roben (:aroben) 2010-11-01 06:29:33 PDT
(In reply to comment #18)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > Why the win file skip?
> > > 
> > > If we have the output produced by a windows box we could just use it (of course we can always just grab it from the windows bot).
> > 
> > I think somebody, who cares about windows builds should decide the correct behavior and modify focusing as GTK and QT does, or choose Mac variant of test result.
> 
> @aroben, what do you think? Should we unskip it and check in known/expected failing results?

You can just remove it from the Skipped list without doing anything else. The test will pass because Windows matches Mac in this case. (On Windows, we fall back to Mac-specific test results if no Windows-specific results can be found.)
Comment 20 Antonio Gomes 2010-11-01 07:24:36 PDT
> > @aroben, what do you think? Should we unskip it and check in known/expected failing results?
> 
> You can just remove it from the Skipped list without doing anything else. The test will pass because Windows matches Mac in this case. (On Windows, we fall back to Mac-specific test results if no Windows-specific results can be found.)

Thanks Adam.

Viatcheslav feel like doing that? You can prefill "Rubber-stamped by Antonio Gomes" in the ChangeLog and just set "cq?" .
Comment 21 Viatcheslav Ostapenko 2010-11-01 11:07:39 PDT
Created attachment 72523 [details]
Remove fast/events/click-focus-control.html from windows Skipped list
Comment 22 Antonio Gomes 2010-11-01 11:09:48 PDT
Maybe we need to reopen the bug for CQ to process it, and when it gets re-open set cq? and cq+ again.

Not sure ... lets see.
Comment 23 Viatcheslav Ostapenko 2010-11-09 07:03:47 PST
Reopen the bug in order for CQ to process last patch.
Comment 24 WebKit Commit Bot 2010-11-09 07:20:24 PST
Comment on attachment 72523 [details]
Remove fast/events/click-focus-control.html from windows Skipped list

Clearing flags on attachment: 72523

Committed r71628: <http://trac.webkit.org/changeset/71628>
Comment 25 WebKit Commit Bot 2010-11-09 07:20:31 PST
All reviewed patches have been landed.  Closing bug.