Bug 105463 – Implement pointer events

• Component: UI Events
• Status: RESOLVED WONTFIX

[Scott Blomquist]

Created attachment 180221 [details]
Early prototype of pointer events

Master bug for any future pointer events work. Will open dependencies for specific sub-items.

[Ryosuke Niwa]

Comment on attachment 180221 [details]
Early prototype of pointer events

Thanks for contributing a patch.

If you wanted reviewers to take a look at your patch, please set r?. If you don’t want them to review your patch, then don’t set any flag on r (like I just removed it now). r- indicates that a reviewer had already taken a look at your patch and decided that it should be rejected, which I believe isn’t the case.

[Rick Byers]

Sorry, that was my suggestion since they know this patch is nowhere near ready to be reviewed (lots of work left, lots of testing to be done, etc.).  I know there's been debate over explicitly marking review- on webkit-dev, maybe I misinterpreted the consensus.

[Darin Fisher (:fishd, Google)]

Comment on attachment 180221 [details]
Early prototype of pointer events

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

> WebKit/chromium/public/WebInputEvent.h:135
> +        // WebPointerEvent

It's not clear to me that the pointer abstraction should be pushed to the
platform.  It seems like Chromium is already feeding WebKit information
about mouse and touch input, and that could just be used to generate
pointer events within WebKit.  Is there an advantage to synthesizing
pointer events within Chromium instead?

I'm not sure I understand the {Got,Lost}PointerCapture events.

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> Sorry, that was my suggestion since they know this patch is nowhere near ready to be reviewed (lots of work left, lots of testing to be done, etc.).  I know there's been debate over explicitly marking review- on webkit-dev, maybe I misinterpreted the consensus.

I think the problem is that we don't have a flag for saying "Please give me initial comments on my work-in-progress patch"

[Rick Byers]

(In reply to comment #3)
> (From update of attachment 180221 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180221&action=review
> 
> > WebKit/chromium/public/WebInputEvent.h:135
> > +        // WebPointerEvent
> 
> It's not clear to me that the pointer abstraction should be pushed to the
> platform.  It seems like Chromium is already feeding WebKit information
> about mouse and touch input, and that could just be used to generate
> pointer events within WebKit.  Is there an advantage to synthesizing
> pointer events within Chromium instead?

Yes, talking offline with Scott I suggested he start by trying to synthesize all the pointer events entirely in WebCore.  I think eventually we'd probably want to push it down to the platform, at least on some ports (eg. I think it's the only way to ever add stylus support on Windows - since neither PlatformMouseEvent nor PlatformTouchEvent is really appropriate for that), but I don't think they should worry about that yet.  Scott is in the process of removing all the code in the platform layer now.

Some of the other big outstanding issues in my mind (other than the obvious outstanding debate about whether pointer events should be implemented in WebKit at all) are:

1) How best to avoid duplicating logic that exists already for mouse/touch handling?  I see there's quite a bit of copy/paste of existing code here (mainly in EventHandler.cpp).  In addition to the software engineering issues, you're also going to have to figure out how to get good test coverage of all the added code.   But at this point the patch is just a proof of concept, so not something to worry too much about yet.

2) How will mouse events be suppressed when pointer events are swallowed?  In particular, how do we make sure that all the expected peripheral behavior associated with mouse events occur (and occurs consistently) regardless of whether or not mouse events are generated (and again, ideally without duplicating this code in two places).  I think there's a ton of complexity and opportunity for regressions in subtle but important behavior here.  I'm not sure where on the spectrum to aim between a  major refactoring and cleanup of EventHandler and a lower-risk approach that may duplicate some logic.  One way to shed some light on this is to look through the entire existing set of mouse and touch tests and decide how much of the behavior (and so also tests) needs to exist for pure pointer events.

3) What is the right strategy in general for testing?  Again there's going to be a lot of overlap with the existing tests.  Should we be trying to share some test code (eg. parameterizing some tests so they could run in both mouse and pointer event variants?).

4) What is the right model for touchevents and pointerevents co-existing.  This is primarily a spec issue that will be addressed by the WebEvents working group.  I think we need to allow both types of events to co-exist on the same page (eg. we wouldn't want to restrict librarys / embeddable sites from opting into consuming pointer events out of fear of breaking sites).  The common case is probably easy (along the lines of the mouse event compatibility in the pointer event spec), but it will get tricky when it comes to how mouse events are generated (TE and PE specify a different mapping to mouse events and it' spossible code designed for the TE model could be broken by the PE model in the presence of TouchEvents).

There's also a number of obvious issues of mechanics (probably want a run-time flag to enable this, -webkit prefixes, etc.) but I think we're far from having anything to land so it's not worth worrying about this yet.

[Maciej Stachowiak]

Besides the issues of mechanics, it seems to me there was not consensus to add this feature to WebKit when it was proposed on webkit-dev. Out of courtesy I won't mark the bug as WONTFIX (as perhaps a patch or spec revisions would change people's minds). But please make sure not to r+ or land anything until/unless the state of the feature proposal discussion changes.

[Ryosuke Niwa]

(In reply to comment #6)
> But please make sure not to r+ or land anything until/unless the state of the feature proposal discussion changes.

Maybe we need a keyword of some sort like PendingWebKitDevResolution?

[Scott Blomquist]

Created attachment 189248 [details]
Updated Pointer Events implementation to live entirely in WebCore (as opposed to creeping into platform layer)

[WebKit Review Bot]

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

Source/WebCore/html/HTMLElement.cpp:341:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.idl:32:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/platform/PlatformPointerEvent.h:36:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/PlatformPointerEvent.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/PlatformPointerEvent.h:53:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:54:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:55:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:56:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:57:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:59:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:71:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:77:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:82:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.h:114:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/PlatformPointerEvent.h:128:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/dom/PointerEvent.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/PointerEvent.h:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.h:104:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:105:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:106:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:107:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:108:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.h:109:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:110:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:111:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:112:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:115:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:118:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:119:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:120:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:121:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:122:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.h:123:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:124:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:123:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/PointerEvent.h:125:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.h:126:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.h:153:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/PlatformPointerEvent.cpp:33:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:56:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:58:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/PlatformPointerEvent.cpp:60:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/PlatformPointerEvent.cpp:80:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/PlatformPointerEvent.cpp:111:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:141:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:151:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/WebCore/platform/PlatformPointerEvent.cpp:175:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/WebCore/platform/PlatformPointerEvent.cpp:194:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:195:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:196:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:197:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/PlatformPointerEvent.cpp:198:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/PointerEvent.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/dom/PointerEvent.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/dom/PointerEvent.cpp:58:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/dom/PointerEvent.cpp:59:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/dom/PointerEvent.cpp:68:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:69:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:70:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:71:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:72:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:73:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:74:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:75:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:76:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:79:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:82:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:83:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:84:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:85:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:86:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:87:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:88:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:89:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:90:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:91:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:92:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:93:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:94:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:95:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:96:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:97:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:98:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:99:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:100:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:103:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:106:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:107:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:108:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:109:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:111:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:112:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:113:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/dom/PointerEvent.cpp:130:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:143:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/PointerEvent.cpp:161:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/PointerEvent.cpp:164:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/PointerEvent.cpp:167:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/dom/PointerEvent.cpp:174:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/WebCore/dom/PointerEvent.cpp:175:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/dom/PointerEvent.cpp:223:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/dom/PointerEvent.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:4050:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/page/EventHandler.cpp:4057:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/page/EventHandler.cpp:4121:  Declaration has space between type name and * in Node *nodeUnderPointer  [whitespace/declaration] [3]
Source/WebCore/page/EventHandler.cpp:4122:  Declaration has space between type name and * in Node *lastNodFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Document.idl', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/Element.idl', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/PointerEvent.cpp', u'Source/WebCore/dom/PointerEvent.h', u'Source/WebCore/dom/PointerEvent.idl', u'Source/WebCore/html/HTMLAttributeNames.in', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/page/DOMWindow.h', u'Source/WebCore/page/DOMWindow.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/platform/PlatformEvent.h', u'Source/WebCore/platform/PlatformPointerEvent.cpp', u'Source/WebCore/platform/PlatformPointerEvent.h', u'Source/WebKit/chromium/features.gypi']" exit_code: 1
eUnderPointer  [whitespace/declaration] [3]
Source/WebCore/page/EventHandler.cpp:4126:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/page/EventHandler.cpp:4177:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 111 in 25 files


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

[Levi Weintraub]

Comment on attachment 189248 [details]
Updated Pointer Events implementation to live entirely in WebCore (as opposed to creeping into platform layer)

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

You can avoid a lot of these messages by running the check-webkit-style script before uploading.

> Source/WebCore/dom/PointerEvent.h:131
> +        int ConvertPointerTypeNameToInt(const String &);
> +        const AtomicString ConvertPointerTypeIntToString(int) const;

More style: functions should begin with a lowercase character.

[Early Warning System Bot]

Comment on attachment 189248 [details]
Updated Pointer Events implementation to live entirely in WebCore (as opposed to creeping into platform layer)

Attachment 189248 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16650096

[Adam Barth]

Comment on attachment 189248 [details]
Updated Pointer Events implementation to live entirely in WebCore (as opposed to creeping into platform layer)

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

Below are a large number of minor style comments.  This patch needs a bit of work before we can land it.  I haven't reviewed much beyond style and structure.  Also, we'll need LayoutTests for this feature.

> Source/WebCore/dom/Document.h:10
> + * Portions Copyright Microsoft Open Technologies, Inc. All rights reserved.

Is it possible to use a copyright notice similar to the ones above (e.g., with a year)?

> Source/WebCore/dom/Document.h:294
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitpointerdown);

We'll probably want to implement pointer events without a vendor prefix given the current state of its standardization.

> Source/WebCore/dom/Node.cpp:2398
> +bool Node::dispatchPointerEvent(const PlatformPointerEvent& e, const AtomicString& eventType,
> +    int detail, Node* relatedTarget)

This should be on one line.

> Source/WebCore/dom/Node.cpp:2400
> +    return EventDispatcher::dispatchEvent(this, PointerEventDispatchMediator::create(PointerEvent::create(eventType, document()->defaultView(), e, detail, relatedTarget)));

e -> event

> Source/WebCore/dom/PointerEvent.cpp:29
> +#include "config.h"
> +#include "MouseEvent.h"
> +#include "PointerEvent.h"

Please move MouseEvent.h into the #include group below (alphabetized).

> Source/WebCore/dom/PointerEvent.cpp:38
> +

Please remove this extra blank line.

> Source/WebCore/dom/PointerEvent.cpp:42
> +// PointerEvent is based on MouseEvent
> +//

Please remove these comments.

> Source/WebCore/dom/PointerEvent.cpp:68
> +PointerEvent::PointerEvent(
> +			const AtomicString& type, 

Please use spaces instead of tabs.

> Source/WebCore/dom/PointerEvent.cpp:114
> +    m_pointerId(eventInfo.m_pointerId),

WebKit style is to begin these lines with ", " and then the name of the member variable.

> Source/WebCore/dom/PointerEvent.cpp:157
> +int PointerEvent::ConvertPointerTypeNameToInt(const String& name)

Should this function use AtomicStrings for faster comparison?  Also, the "C" should be lower-case.  WebKit uses camelCase names for functions.

> Source/WebCore/dom/PointerEvent.cpp:159
> +    if (name == "mouse") {

No need for { } with one-line conditionals.

> Source/WebCore/dom/PointerEvent.cpp:172
> +const AtomicString PointerEvent::ConvertPointerTypeIntToString(int pointerType) const

Why does this return a "const" string?  That doesn't make much sense.

> Source/WebCore/dom/PointerEvent.cpp:175
> +    switch(pointerType)
> +    {

These lines should be combined.

> Source/WebCore/dom/PointerEvent.cpp:190
> +inline static int adjustedClientX(int innerClientX, HTMLIFrameElement* iframe, FrameView* frameView)

Please move static functions to the top of the file (just inside the "namespace WebCore" declaration).  Also, the "inline" keyword is redundant with the "static" keyword here.

> Source/WebCore/dom/PointerEvent.cpp:217
> +    if (dispatcher->node()->disabled()) // Don't even send DOM events for disabled controls..

Please remove this comment.  WebKit enjoys comments that explain the "why" behind the code.  Comments like this that just say "what" the code does are redundant with the code itself.

> Source/WebCore/dom/PointerEvent.cpp:225
> +    if ((event()->type() != eventNames().webkitpointeroverEvent)  &&
> +       (event()->type() != eventNames().webkitpointeroutEvent)) 
> +    {

These lines do not conform to the WebKit style guide.

> Source/WebCore/dom/PointerEvent.h:40
> +    class PointerEvent : public MouseEvent {

Class declarations should not be indented.

> Source/WebCore/dom/PointerEvent.h:92
> +        // provide interface name for pointer object

I'm not sure what you're trying to say with this comment.  Perhaps we should remove it?

> Source/WebCore/dom/PointerEvent.h:95
> +        // Overload to separate from MouseEvent

This comment is redundant with the code.

> Source/WebCore/dom/PointerEvent.h:100
> +        const AtomicString pointerType() const { return ConvertPointerTypeIntToString(m_pointerType); }

The "const" here doesn't really make sense.

> Source/WebCore/dom/PointerEvent.h:127
> +    protected:

Why protected and not private?

> Source/WebCore/dom/PointerEvent.idl:12
> +[
> +    Conditional=POINTER_EVENTS
> +] interface PointerEvent : MouseEvent {

We should use ConstructorTemplate=Event to allow for DOM4-style construction of PointerEvents.

> Source/WebCore/dom/PointerEvent.idl:16
> +    [InitializedByEventConstructor] readonly attribute boolean isPrimary;
> +    [InitializedByEventConstructor] readonly attribute long pointerId;
> +    [InitializedByEventConstructor] readonly attribute DOMString pointerType;

InitializedByEventConstructor <-- These attributes are good, but they require the ConstructorTemplate attribute on the interface in order to work.

> Source/WebCore/dom/PointerEvent.idl:18
> +    [ObjCLegacyUnnamedParameters] void initPointerEvent(in [Optional=DefaultIsUndefined] DOMString type, 

Please remove initPointerEvent.  Instead, script should use a DOM4-style event constructor.

If the Pointer Events spec is still using this old style of event construction, we should provide feedback to the working group that it should instead using DOM4-style event constructors.

> Source/WebCore/html/HTMLAttributeNames.in:228
> +onwebkitpointerdown
> +onwebkitpointerup
> +onwebkitpointermove
> +onwebkitpointerover
> +onwebkitpointerout
> +onwebkitpointercancel

Again, we should remove the vendor prefixes.  The spec is already in LC.

> Source/WebCore/page/EventHandler.cpp:4053
> +bool EventHandler::handlePointerEvents(PlatformPointerEventCollection & coll)

No space needed between PlatformPointerEventCollection and &.  Should this be a const reference?

Also, "coll" isn't a great name.  WebKit prefers that we use complete words in variable names.

> Source/WebCore/page/EventHandler.cpp:4056
> +    for (Vector<PlatformPointerEvent>::iterator iter = coll.events.begin(); iter != coll.events.end(); iter++)
> +    {

Please combine these lines.

Is it safe to iterate the vector from the parameter?  Should we make a local copy of the vector first and iterate that?  Given that we're dispatching events below, we might worry that JavaScript will destroy the parameter.  (I haven't analyzed the codepath to see if that's a real problem.)

> Source/WebCore/page/EventHandler.cpp:4061
> +        if (e.PointerProcessed()) {

No need for { } for one-line conditionals.

Also, please replace "e" with a complete word.

> Source/WebCore/page/EventHandler.cpp:4082
> +        default:

Generally we leave off the default case for this sort of switch so that the compiler will tell us if we forget a case.

> Source/WebCore/page/EventHandler.cpp:4089
> +bool EventHandler::dispatchPointerEvent(const AtomicString& eventType, const PlatformPointerEvent& e)

This function is too long.  Can we factor out various pieces into smaller functions?

> Source/WebCore/page/EventHandler.cpp:4093
> +    ASSERT(m_frame);
> +    if (!m_frame)
> +        return false;

It isn't correct to both ASSERT a condition and to handle its negation.  If the case can occur, then we should handle it and not have an ASSERT.  If the case cannot occur, then we should have the ASSERT and not handle the impossible case.

> Source/WebCore/page/EventHandler.cpp:4128
> +    Node *nodeUnderPointer = result;
> +    Node *lastNodeUnderPointer = m_lastNodeUnderMouse.get();

"Node *"  ->  "Node* "

> Source/WebCore/platform/PlatformPointerEvent.cpp:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Why is there an Apple copyright in this new file?

> Source/WebCore/platform/PlatformPointerEvent.cpp:31
> +/*
> +
> +This implements Pointer Events as defined in Pointer Events specification submission to W3C.
> +
> + */

This comment is useless and should be removed.

> Source/WebCore/platform/PlatformPointerEvent.cpp:38
> +static long convertButtonToChordedValue(PointerButton);
> +static long convertMouseButtonToChordedValue(MouseButton);

Usually we implement static functions at the same time we declare them.

> Source/WebCore/platform/PlatformPointerEvent.cpp:48
> +//----------------------------------

This comment is useless and should be removed.

> Source/WebCore/platform/PlatformPointerEvent.cpp:109
> +PlatformEvent::Type PlatformPointerEvent::convertToPointerType(PlatformEvent::Type t)

"t" -> please use complete words in variable names.

> Source/WebCore/platform/PlatformPointerEvent.cpp:149
> +static long convertButtonToChordedValue(PointerButton btn)

btn -> please use complete words in variable names.

> Source/WebCore/platform/PlatformPointerEvent.cpp:198
> +//-----------------------------------
> +
> +//#ifdef __clang__
> +//#pragma clang diagnostic ignored "-Wglobal-constructors"
> +//#pragma clang diagnostic ignored "-Wexit-time-destructors"
> +//#endif
> +//PlatformPointerEventTracker PlatformPointerEvent::m_trackedPointerEvents;

Please remove commented out code.

[Adam Barth]

> Maybe we need a keyword of some sort like PendingWebKitDevResolution?

I don't think we need such a keyword.

[Adam Barth]

Looks like the spec has already been updated to using a DOM4-style constructor:

https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#pointerevent-interface

[Scott Blomquist]

Created attachment 194399 [details]
Updated pointer events implementation

With this patch, check-webkit-style runs clean for me and I've incorporated most of Adam's and Levi's review feedback. I'll incorporate the rest of the feedback in the next update.

[Scott Blomquist]

Additional revisions to the Pointer Events implementation. It's still not quite ready for an intensive review, but I appreciate all of the feedback that I've received from all of you so far.

My next major steps include:
  1) Finish implementing the remaining features of the Pointer Events spec draft. (At which point, I'll ask for a much deeper review.)
  2) Add layout tests.
  3) Incorporate remaining feedback items from Adam, Levi, et al.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Document.idl', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/Element.idl', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/PointerEvent.cpp', u'Source/WebCore/dom/PointerEvent.h', u'Source/WebCore/dom/PointerEvent.idl', u'Source/WebCore/html/HTMLAttributeNames.in', u'Source/WebCore/html/HTMLElement.cpp', u'Source/WebCore/page/DOMWindow.h', u'Source/WebCore/page/DOMWindow.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/platform/PlatformEvent.h', u'Source/WebCore/platform/PlatformPointerEvent.cpp', u'Source/WebCore/platform/PlatformPointerEvent.h', u'Source/WebKit/chromium/features.gypi']" exit_code: 1
Source/WebCore/page/EventHandler.cpp:4175:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 25 files


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

[Early Warning System Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17226423

[WebKit Review Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17139747

[Build Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17155927

[Early Warning System Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17138920

[kov's GTK+ EWS bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17212894

[WebKit Review Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17147670

[Peter Beverloo (cr-android ews)]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17193611

[Peter Beverloo (cr-android ews)]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17136902

[EFL EWS Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17239360

[EFL EWS Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17207584

[Build Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17209738

[Build Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17207647

[Build Bot]

Comment on attachment 194399 [details]
Updated pointer events implementation

Attachment 194399 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17183856

[Scott Blomquist]

Created attachment 199314 [details]
Updated pointer events implementation

A few more bug and style fixes. Still TODO: test coverage, implement more of the spec.

[Jason G.]

Is it possible to get an update on this? It looks like Microsoft's attention has been on Blink and Gecko for the past several months, with the latest patch here bitrotting away. I was going to play around at the platform layer and investigate writing patches to properly pass through pen events, but so far I haven't been able to rebase this basic implementation patch to master.

[Benjamin Poulain]

Short term, I don't think we should add this.

I don't think the spec even addressed the compatibility with touch events(?).

[Matt Brubeck]

(In reply to comment #33)
> I don't think the spec even addressed the compatibility with touch events(?).

Work on documenting Touch Event / Pointer Event interaction has started in the Touch Event CG:
http://www.w3.org/community/touchevents/

Overall however we cannot make progress on Touch Event standardization in the W3C if Apple continues to refuse to participate, and continues trying to patent various parts of the API.

[Jorik Tangelder]

What about supporting the touch-action property of the pointer events specification?
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property

Even when pointer events are not implemented, this css property would really improve the support for touch gestures in webpages. Now we're stuck with only calling preventDefault, and often makes elements blocking the scrolling of a page when not needed...

[Benjamin Poulain]

(In reply to comment #35)
> What about supporting the touch-action property of the pointer events specification?
> https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property
> 
> Even when pointer events are not implemented, this css property would really improve the support for touch gestures in webpages. Now we're stuck with only calling preventDefault, and often makes elements blocking the scrolling of a page when not needed...

Yes, I think this is a good idea.

The definition of touch-action is very odd, but the general concepts are interesting.

Can you please open a separate bug for that? It is worth experimenting with touch-action outside pointer events.

[Patrick H. Lauke]

Separate bug for touch-action:manipulation https://bugs.webkit.org/show_bug.cgi?id=133114

[Benjamin Poulain]

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

[Lucas Forschler]

Mass move bugs into the DOM component.

[Ryosuke Niwa]

Pointer events are UI events. Also, pointer events are currently in development.