Bug 40641 – [Qt] Clicking button input does not give it focus

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Viatcheslav Ostapenko]

This bug https://bugs.webkit.org/show_bug.cgi?id=22261 for Qt only.

[Viatcheslav Ostapenko]

Created attachment 58831 [details]
Fix for the html elements not focused by mouse.

[Antonio Gomes]

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.

[Andreas Kling]

(In reply to comment #2)
> Lgtm, anyways.

Mind r+ing it then with your new powers? :-)

[Antonio Gomes]

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.

[Antonio Gomes]

I will work on the tests for this, and of course keep Viatcheslav's name on ChangeLog.

[Viatcheslav Ostapenko]

Created attachment 70651 [details]
Updated layout test.

[Antonio Gomes]

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.

[Antonio Gomes]

Created attachment 70733 [details]
mac specific expected result

[Viatcheslav Ostapenko]

Created attachment 70738 [details]
Added mac specific test result.

[Antonio Gomes]

You probably want it to block bug 44677, so it get cherry-picked to branch 2.1

[Eric Seidel (no email)]

Why the win file skip?

[Antonio Gomes]

(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).

[Antonio Gomes]

Viatcheslav , you want the commit-queue to land it automatically for you?

If so, set cq?

[Viatcheslav Ostapenko]

(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.

[WebKit Commit Bot]

Comment on attachment 70738 [details]
Added mac specific test result.

Clearing flags on attachment: 70738

Committed r69925: <http://trac.webkit.org/changeset/69925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r69925 cherry-picked into qtwebkit-2.1 with commit baa0e2d <http://gitorious.org/webkit/qtwebkit/commit/baa0e2d>

[Antonio Gomes]

(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?

[Adam Roben (:aroben)]

(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.)

[Antonio Gomes]

> > @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?" .

[Viatcheslav Ostapenko]

Created attachment 72523 [details]
Remove fast/events/click-focus-control.html from windows Skipped list

[Antonio Gomes]

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.

[Viatcheslav Ostapenko]

Reopen the bug in order for CQ to process last patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.