Bug 39493 – Touch events do not affect the :active CSS state

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

When using touch events, clicking an element with an active state defined in CSS does not cause a transition into that state.

I'm working on a patch for this.

[Ben Murdoch]

Created attachment 56884 [details]
Proposed patch and test.

[WebKit Review Bot]

Attachment 56884 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/EventHandler.cpp:2771:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Murdoch]

Created attachment 56886 [details]
Patch and layout test changes.

Correct style on switch statement.

[Ben Murdoch]

ping .... thoughts anyone? :)

[Kim Grönholm]

Hi Ben and sorry for late reply.

I was thinking about the definition of the :active pseudo-class:
"•The :active pseudo-class applies while an element is being activated by the user. For example, between the times the user presses the mouse button and releases it. On systems with more than one mouse button, :active applies only to the primary or primary activation button (typically the "left" mouse button), and any aliases thereof." (http://www.w3.org/TR/css3-selectors/#the-user-action-pseudo-classes-hover-act)

Should we only do transition to the active state from left mouse button events since those are the ones that actually activate e.g. a link? And then mapping touch events to mouse events on system/whatever level makes the links work and also enable the active state transitions (since eventhandler thinks that left mouse button was used).

This is just a thought, what do you think?

[Ben Murdoch]

(In reply to comment #5)

Hey Kim,

Thanks for the comments. We don't convert the touches on android to  mouse events until the touch sequence is complete - i.e. the finger is released again, by which time the active class wouldn't apply anymore. Although they are similar, I don't think mouse and touch events are the same enough to do conversion sooner than this. We don't know the intent of the user's touch until later - were they just meaning to pan the page or perform a long press for the context menu for example (in which case we wouldn't want to trigger links/send mouse events to webcore ... or set the active state).

There's a good example of using active on divs with touch handlers at this page: http://xdev.org/anim/browser/ the divs should turn blue while you are touching them. The motivation behind trying to come up with a fix for this is to bring WebKit touch support in line with that of the iPhone :)

Cheers, Ben

[Kim Grönholm]

Ah.. Right, that's a good point, I see now :) And it is common not to send the left mouse button press immediately.. This is fine on my behalf.

[Ben Murdoch]

Thanks for taking a look Kim. I suspect that I'll need to rebase this patch now but rather than just uploading a new rebased one I could also make any changes that reviewers suggest at the same time. Any thoughts guys? :)

Thanks, Ben

[Ben Murdoch]

Created attachment 62058 [details]
Rebased patch / layout test.

[WebKit Review Bot]

Attachment 62058 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/page/EventHandler.cpp:2857:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Murdoch]

Created attachment 62063 [details]
Rebased patch / layout test, fixed style error.

[Ben Murdoch]

Hmm, strange that the Mac EWS isn't happy - built fine for me locally.

I'm trying a clean build locally to try and see what the problem is.

[Ben Murdoch]

Hmm, built completely fine on my local machine. Perhaps some flakiness in the bot... doesn't seem to provide any extra information other than "fail"...

[Steve Block]

Comment on attachment 62063 [details]
Rebased patch / layout test, fixed style error.

Kim's the expert here, but given his previous review, r=me

[Ben Murdoch]

Comment on attachment 62063 [details]
Rebased patch / layout test, fixed style error.

I can't repro the Mac build problem, so going to try landing. Will watch the build pages carefully.

[Ben Murdoch]

Patch landed as r63807; there was a break on Mac (not sure why it didn't repro on my local MacBook build though) and the fix for that break (simple xcodeproj update) landed in r63808.

[Simon Fraser (smfr)]

Using an int for hitType is ugly. This should be a typedef.

[Ben Murdoch]

Created attachment 62215 [details]
Use a typedef as suggested.

[Ben Murdoch]

Reopening based on SImon's comments.

[Simon Fraser (smfr)]

Comment on attachment 62215 [details]
Use a typedef as suggested.

You should use the same type in HitTestRequest, like this:

class HitTestRequest {
public:
    enum RequestTypeFlags {
        ReadOnly = 1 << 1,
        ...
    };

    typedef unsigned HitTestRequestType;

    HitTestRequest(HitTestRequestType requestType) ...

private:
    HitTestRequestType m_requestType;
}
etc.

[Ben Murdoch]

Created attachment 62222 [details]
Patch with typedef.

I couldn't find anywhere else in WebCore that is using an int/unsigned for this purpose that needed updating. WDYT?

Thanks, Ben

[Simon Fraser (smfr)]

Comment on attachment 62222 [details]
Patch with typedef.

r=me. Thanks for the cleanup!

[WebKit Commit Bot]

Comment on attachment 62222 [details]
Patch with typedef.

Clearing flags on attachment: 62222

Committed r63888: <http://trac.webkit.org/changeset/63888>

[Adam Barth]

This patch claims to have been landed.