Bug 92093 - There is no way to tell whether an element can be activated or not
Summary: There is no way to tell whether an element can be activated or not
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: 91894
  Show dependency treegraph
 
Reported: 2012-07-24 04:02 PDT by Allan Sandfeld Jensen
Modified: 2012-07-30 11:45 PDT (History)
15 users (show)

See Also:


Attachments
Patch (26.09 KB, patch)
2012-07-24 04:06 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (25.98 KB, patch)
2012-07-24 05:00 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (42.26 KB, patch)
2012-07-24 06:08 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (38.13 KB, patch)
2012-07-27 06:49 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (36.38 KB, patch)
2012-07-27 08:02 PDT, Allan Sandfeld Jensen
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-07-24 04:02:21 PDT
To complement the hasEventListener we should have a corresponding hasDefaultEventHandler that can tell if a node implementation will react in any way to an event passed to it.

This is useful in touch-adjustment which can more reliably tell which nodes might respond to a synthesised click event and which will node.
Comment 1 Allan Sandfeld Jensen 2012-07-24 04:06:52 PDT
Created attachment 154013 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-24 04:23:44 PDT
Comment on attachment 154013 [details]
Patch

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

> Source/WebCore/html/HTMLSummaryElement.cpp:160
> +bool HTMLSummaryElement::hasDefaultEventHandler(const AtomicString& eventType)

Why not call it hasDefaultEventHandlerFor ?
Comment 3 WebKit Review Bot 2012-07-24 04:34:30 PDT
Comment on attachment 154013 [details]
Patch

Attachment 154013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13331341
Comment 4 Allan Sandfeld Jensen 2012-07-24 04:53:18 PDT
(In reply to comment #2)
> (From update of attachment 154013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154013&action=review
> 
> > Source/WebCore/html/HTMLSummaryElement.cpp:160
> > +bool HTMLSummaryElement::hasDefaultEventHandler(const AtomicString& eventType)
> 
> Why not call it hasDefaultEventHandlerFor ?

It follows the argument style of hasEventListeners(), but I have no strong opinion on what we call it.
Comment 5 Allan Sandfeld Jensen 2012-07-24 05:00:59 PDT
Created attachment 154022 [details]
Patch
Comment 6 WebKit Review Bot 2012-07-24 05:28:53 PDT
Comment on attachment 154022 [details]
Patch

Attachment 154022 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13325438
Comment 7 Kevin Ellis 2012-07-24 05:41:33 PDT
cr-linux error:

Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:204: error: 'const class WebCore::HTMLInputElement' has no member named 'isReadOnly'

Should this be isReadOnlyFormControl?
Comment 8 Kevin Ellis 2012-07-24 05:43:13 PDT
Thanks for looking into this Allan.

Addresses one of two issues with bug 91894 (Search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled).
Comment 9 Allan Sandfeld Jensen 2012-07-24 06:01:16 PDT
(In reply to comment #7)
> cr-linux error:
> 
> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:204: error: 'const class WebCore::HTMLInputElement' has no member named 'isReadOnly'
> 
> Should this be isReadOnlyFormControl?

No it should be readOnly(). I will upload a new patch soon.
Comment 10 Allan Sandfeld Jensen 2012-07-24 06:08:01 PDT
Created attachment 154037 [details]
Patch

Fix isReadOnly -> readOnly() and add hasDefaultEventHandler to shadow media controls
Comment 11 Antonio Gomes 2012-07-24 08:35:05 PDT
Comment on attachment 154037 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Touch-adjustment is changed to using hasDefaultEventHandler which improves accuracy in 
> +        shadow-dom, and also means it will now prefer enabled form-elements over disabled ones.

is the difference testable?
Comment 12 Kevin Ellis 2012-07-24 08:55:27 PDT
(In reply to comment #11)
> (From update of attachment 154037 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154037&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        Touch-adjustment is changed to using hasDefaultEventHandler which improves accuracy in 
> > +        shadow-dom, and also means it will now prefer enabled form-elements over disabled ones.
> 
> is the difference testable?

This patch partially fixes bug 91894 (search cancel button is hard to activate with a tap gesture even if touch adjustment is enabled).  Please feel free to borrow from that test case.  Without this fix, any touch that clips the input field next to the cancel button would cause a touch adjustment to the input field as the cancel button would be pruned from the candidate list.  In short, the fix is testable, though several cases in the test attached to 91894 will still fail because of the scoring algorithm.
Comment 13 Alexey Proskuryakov 2012-07-24 09:29:46 PDT
I have very strong reservations about this feature.

This is a whole lot of parallel knowledge about default handlers, which can become a maintenance nightmare. Furthermore, it's hard to get interoperability here, because certain behaviors that are implemented via default handlers in one browser can be implemented otherwise in others (access keys come to mind).
Comment 14 Ryosuke Niwa 2012-07-24 10:22:36 PDT
Comment on attachment 154037 [details]
Patch

This feature should definitely be discussed on webkit-dev first.
Comment 15 Allan Sandfeld Jensen 2012-07-24 11:06:48 PDT
(In reply to comment #13)
> I have very strong reservations about this feature.
> 
> This is a whole lot of parallel knowledge about default handlers, which can become a maintenance nightmare. Furthermore, it's hard to get interoperability here, because certain behaviors that are implemented via default handlers in one browser can be implemented otherwise in others (access keys come to mind).

It is NOT intended as an exported API, I completely agree with you on that. This is only intended for internal use.

It is currently only used for touch-adjustment. The alternative to hasDefaultEventHandler could be an API that only returned whether the Node would respond to the events emitted by a tap gesture. The question here is: Is this something we would want to make generic so you can also tell if an element would respond to keyboard-events, etc? Or would it be better with a simpler touch-adjustment specific API?
Comment 16 Ryosuke Niwa 2012-07-24 12:34:47 PDT
(In reply to comment #15)
> It is currently only used for touch-adjustment.

We already have needTouchEvents. We can't use needTouchEvents here?
Comment 17 Antonio Gomes 2012-07-24 13:04:22 PDT
(In reply to comment #0)
> To complement the hasEventListener we should have a corresponding hasDefaultEventHandler that can tell if a node implementation will react in any way to an event passed to it.
> 
> This is useful in touch-adjustment which can more reliably tell which nodes might respond to a synthesised click event and which will node.

FWIW, in the blackberry implementation we list the nodes we are interested in filtering in, instead of asking webcore if it has a defaulthandle for the events we care about.
Comment 18 Allan Sandfeld Jensen 2012-07-24 15:35:21 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > It is currently only used for touch-adjustment.
> 
> We already have needTouchEvents. We can't use needTouchEvents here?

This is not about native touch events, this is asking if a node responds to click, DOMactivate, mouseup, mousedown or mousemove. The events that are emitted by a tap gesture.

(In reply to comment #17)
> (In reply to comment #0)
> > To complement the hasEventListener we should have a corresponding hasDefaultEventHandler that can tell if a node implementation will react in any way to an event passed to it.
> > 
> > This is useful in touch-adjustment which can more reliably tell which nodes might respond to a synthesised click event and which will node.
> 
> FWIW, in the blackberry implementation we list the nodes we are interested in filtering in, instead of asking webcore if it has a defaulthandle for the events we care about.

I am aware of that, and we had something similar in the browser for N9, but I want to move away from that kind of solution. Not only is such a list likely to be incomplete or get outdated. It is also expensive to match and can not solve the problem from bug 91894 with special shadow dom elements.
Comment 19 Ryosuke Niwa 2012-07-24 15:45:50 PDT
(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > It is currently only used for touch-adjustment.
> > 
> > We already have needTouchEvents. We can't use needTouchEvents here?
> 
> This is not about native touch events, this is asking if a node responds to click, DOMactivate, mouseup, mousedown or mousemove. The events that are emitted by a tap gesture.

My concern for adding this method will be that it'll be get out-of-date very quickly. e.g. how is a contributor supposed to know that he/she needs to update hasDefaultEventHandler when he/she has updated defaultEventHandler? Given that the vast majority of contributors come from Apple and Google these days and they don't use this feature, this is bound to regress.

I highly recommend to revise the approach you're taking here.

> I am aware of that, and we had something similar in the browser for N9, but I want to move away from that kind of solution. Not only is such a list likely to be incomplete or get outdated. It is also expensive to match and can not solve the problem from bug 91894 with special shadow dom elements.

How does moving the code into WebCore help that situation? I'm sorry but nobody is going to remember to update hasDefaultEventHandler in practice.
Comment 20 Allan Sandfeld Jensen 2012-07-25 00:49:13 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > It is currently only used for touch-adjustment.
> > > 
> > > We already have needTouchEvents. We can't use needTouchEvents here?
> > 
> > This is not about native touch events, this is asking if a node responds to click, DOMactivate, mouseup, mousedown or mousemove. The events that are emitted by a tap gesture.
> 
> My concern for adding this method will be that it'll be get out-of-date very quickly. e.g. how is a contributor supposed to know that he/she needs to update hasDefaultEventHandler when he/she has updated defaultEventHandler? Given that the vast majority of contributors come from Apple and Google these days and they don't use this feature, this is bound to regress.

What kind of events a Node reacts to shouldn't change very often, most changes to defaultEventHandlers are how they react. Worst case scenario if it is out of date, is that taps are not adjusted correctly making the node slightly harder to hit for a user. 

Additionally it is possible to make watch-list for changes to defaultEventHandlers if we really want to monitor it.

> 
> I highly recommend to revise the approach you're taking here.
> 
> > I am aware of that, and we had something similar in the browser for N9, but I want to move away from that kind of solution. Not only is such a list likely to be incomplete or get outdated. It is also expensive to match and can not solve the problem from bug 91894 with special shadow dom elements.
> 
> How does moving the code into WebCore help that situation? I'm sorry but nobody is going to remember to update hasDefaultEventHandler in practice.

I am not moving anything into WebCore, touch-adjustment already is in WebCore. This is fixing a bug in WebCore. We can argue if this is the right way to fix it, but it has nothing to do with being in WebCore or not being in WebCore.
Comment 21 Ryosuke Niwa 2012-07-25 01:08:08 PDT
(In reply to comment #20)
> (In reply to comment #19) 
> > How does moving the code into WebCore help that situation? I'm sorry but nobody is going to remember to update hasDefaultEventHandler in practice.
> 
> I am not moving anything into WebCore, touch-adjustment already is in WebCore. This is fixing a bug in WebCore. We can argue if this is the right way to fix it, but it has nothing to do with being in WebCore or not being in WebCore.

Then I'd argue that this is not the right fix. Additionally, you should reference to the bug you're trying to fix.
Comment 22 Antonio Gomes 2012-07-25 05:53:01 PDT
> > I highly recommend to revise the approach you're taking here.
> > 
> > > I am aware of that, and we had something similar in the browser for N9, but I want to move away from that kind of solution. Not only is such a list likely to be incomplete or get outdated. It is also expensive to match and can not solve the problem from bug 91894 with special shadow dom elements.
> > 
> > How does moving the code into WebCore help that situation? I'm sorry but nobody is going to remember to update hasDefaultEventHandler in practice.
> 
> I am not moving anything into WebCore, touch-adjustment already is in WebCore. This is fixing a bug in WebCore. We can argue if this is the right way to fix it, but it has nothing to do with being in WebCore or not being in WebCore.

Sorry Allan, but I am in agreement with the others here. A list "fixed list" of elements won't change either, and can be a single place to keep this, imo.
Comment 23 Allan Sandfeld Jensen 2012-07-25 07:29:25 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19) 
> > > How does moving the code into WebCore help that situation? I'm sorry but nobody is going to remember to update hasDefaultEventHandler in practice.
> > 
> > I am not moving anything into WebCore, touch-adjustment already is in WebCore. This is fixing a bug in WebCore. We can argue if this is the right way to fix it, but it has nothing to do with being in WebCore or not being in WebCore.
> 
> Then I'd argue that this is not the right fix. Additionally, you should reference to the bug you're trying to fix.

I didn't block the originally bug this patch could fix because I am not fully confident in whether this is the right solution or not either, but one way or another, those special nodes that can be tap-activated has to be recognizable, and they can not be matched by CSS queries, and they currently have no useful functions that can tell us if they can be tap-activated.
Comment 24 Ryosuke Niwa 2012-07-25 10:23:10 PDT
(In reply to comment #23)
> I didn't block the originally bug this patch could fix because I am not fully confident in whether this is the right solution or not either, but one way or another, those special nodes that can be tap-activated has to be recognizable, and they can not be matched by CSS queries, and they currently have no useful functions that can tell us if they can be tap-activated.

I'm sorry but we don't normally add new functions to WebCore based on a possibility that it could help us moving forward in other bugs. Otherwise, we'll be adding all sorts of functions and classes that could be useful. Instead, we'll look at the actual problem/bug at hand, and then come up with a right solution for that problem along with a regression test.

As such, I don't think we should proceed with this patch. In fact, we should probably close this bug as WontFix and move onto the actual touch adjustment bug you mentioned.
Comment 25 Allan Sandfeld Jensen 2012-07-25 11:04:19 PDT
(In reply to comment #24)
> I'm sorry but we don't normally add new functions to WebCore based on a possibility that it could help us moving forward in other bugs. Otherwise, we'll be adding all sorts of functions and classes that could be useful. Instead, we'll look at the actual problem/bug at hand, and then come up with a right solution for that problem along with a regression test.
> 
> As such, I don't think we should proceed with this patch. In fact, we should probably close this bug as WontFix and move onto the actual touch adjustment bug you mentioned.

Ey? I am sorry if I have offended you, but there is not reason to get confrontational. 

Bug 91894 can be solved in a more specific but incorrect way, which is the approach currently pursued since this correct solution might draw out with pointless discussions. Also bug 91894 is only one _example_ of the problem, the same problem happens in several instances. It is quite possible to extend this patch with several test-cases which it fixes. 

If we end up using a different approach, the title of the bug should be changed, but since I wanted to use the bug to post a possible solution for discussion, I kept it specific to the solution and not the problem.
Comment 26 Allan Sandfeld Jensen 2012-07-25 11:04:51 PDT
Comment on attachment 154037 [details]
Patch

Lacks test-cases.
Comment 27 Ryosuke Niwa 2012-07-25 11:09:28 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > I'm sorry but we don't normally add new functions to WebCore based on a possibility that it could help us moving forward in other bugs. Otherwise, we'll be adding all sorts of functions and classes that could be useful. Instead, we'll look at the actual problem/bug at hand, and then come up with a right solution for that problem along with a regression test.
> > 
> > As such, I don't think we should proceed with this patch. In fact, we should probably close this bug as WontFix and move onto the actual touch adjustment bug you mentioned.
> 
> Ey? I am sorry if I have offended you, but there is not reason to get confrontational. 

I have no intention to be confrontational here. I'm just giving you information and stating my opinions.

> Bug 91894 can be solved in a more specific but incorrect way, which is the approach currently pursued since this correct solution might draw out with pointless discussions. Also bug 91894 is only one _example_ of the problem, the same problem happens in several instances. It is quite possible to extend this patch with several test-cases which it fixes. 

What are those other problems?
Comment 28 Ryosuke Niwa 2012-07-25 11:28:20 PDT
Looking at the bug 91894, what we need to know is whether an element can be activated or not, which is a different question from whether an element has a default event handler or not.
Comment 29 Allan Sandfeld Jensen 2012-07-25 11:40:30 PDT
(In reply to comment #28)
> Looking at the bug 91894, what we need to know is whether an element can be activated or not, which is a different question from whether an element has a default event handler or not.

True, but the detail is that we want to know if it will be activated (or in some other way react) to the events generated by a gesture tap. A gesture tap generates mousemove, mousedown and mouseup events. These events indirectly also triggers click, DOMActivate and focus events. 

We already have good ways of checking if an element is author programmed to react to these event using hasEventListener(eventType), and we can tell if it can be focused by calling is mouseFocusable(). The only part we do not know, is if something other than focus has an effect in the defaultEventHandler. So this first suggestion was to implement something similar to hasEventListener for the default event handling, that tells us if there is a chance the element will react to that event-type.

If we do not want something as generic as hasDefaultEventHandler(eventType), something else needs to tell us if the node/element will react to a click, DOMActivate, mousemove, mousedown or mouseup event in the defaultEventHandler.

I am open to suggestions.
Comment 30 Ryosuke Niwa 2012-07-25 11:59:31 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > Looking at the bug 91894, what we need to know is whether an element can be activated or not, which is a different question from whether an element has a default event handler or not.
> 
> True, but the detail is that we want to know if it will be activated (or in some other way react) to the events generated by a gesture tap. A gesture tap generates mousemove, mousedown and mouseup events. These events indirectly also triggers click, DOMActivate and focus events. 

I can't think of an element activation that can't be triggered by mousedown & mouse up. Given that, I tend to think that we don't need to differentiate elements that can be activated via a gesture tap and ones that can be activated by anything but a gesture tap.

> We already have good ways of checking if an element is author programmed to react to these event using hasEventListener(eventType), and we can tell if it can be focused by calling is mouseFocusable(). The only part we do not know, is if something other than focus has an effect in the defaultEventHandler. So this first suggestion was to implement something similar to hasEventListener for the default event handling, that tells us if there is a chance the element will react to that event-type.

The maintenance cost of hasEventListener is much no lower than hasDefaultEventHandler you proposed in previous patches because JSC and V8 know how many event listeners there are on a given element for a given event. On the other hand, maintaining that information for WebCore code incurs some cost, and we should try to minimize that.

One approach that might work, if we're going to take the original approach you took, then it's probably better to make defaultEventHandler answer the question when "event" (the argument) is 0.

> If we do not want something as generic as hasDefaultEventHandler(eventType), something else needs to tell us if the node/element will react to a click, DOMActivate, mousemove, mousedown or mouseup event in the defaultEventHandler.

We probably don't want that because there are default event handlers for all sorts of events, and we're only interested in whether an element activates or not.
Comment 31 Ryosuke Niwa 2012-07-25 12:00:14 PDT
>One approach that might work, if we're going to take the original approach you took, then it's probably better to make defaultEventHandler answer the question when "event" (the argument) is 0.

should read

One approach that might work, if we're going to take the original approach you took, is to make defaultEventHandler answer the question when "event" (the argument) is 0.
Comment 32 Benjamin Poulain 2012-07-25 14:13:27 PDT
I understand the problem you are trying to solve here, but in my opinion this patch is not a solution.

What you want is a way for an element to tell if it will, _in its current state_, respond to a click event. You must take into account the state of the element, and you don't need to support any type of events.

Even such code is hard to maintain. To make it more manageable, you should have it extensively tested (and ideally also used by Chromium for the synthetic click implementation).
Comment 33 Allan Sandfeld Jensen 2012-07-26 06:12:55 PDT
(In reply to comment #32)
> I understand the problem you are trying to solve here, but in my opinion this patch is not a solution.
> 
> What you want is a way for an element to tell if it will, _in its current state_, respond to a click event. You must take into account the state of the element, and you don't need to support any type of events.
> 
> Even such code is hard to maintain. To make it more manageable, you should have it extensively tested (and ideally also used by Chromium for the synthetic click implementation).

So something like isMouseActivatable? 

I guess that could make sense, and matches the style of isMouseFocusable virtual functions.
Comment 34 Ryosuke Niwa 2012-07-26 10:59:34 PDT
(In reply to comment #33)
> So something like isMouseActivatable? 

Yeah. That's probably the most sensible solution here.
Comment 35 Benjamin Poulain 2012-07-26 12:45:02 PDT
> So something like isMouseActivatable?

Yep, that is the idea.
If you want to follow the iOS terminology: willRespondToMouseClickEvents().
Comment 36 Allan Sandfeld Jensen 2012-07-27 05:20:50 PDT
(In reply to comment #35)
> > So something like isMouseActivatable?
> 
> Yep, that is the idea.
> If you want to follow the iOS terminology: willRespondToMouseClickEvents().

Well actually they seem to have set of these to cover various events 
willRespondToMouseClickEvents, willRespondToMouseMoveEvents, willRespondToMouseWheelEvents. I am not sure having multiple functions is any better than a generic one.
Comment 37 Allan Sandfeld Jensen 2012-07-27 06:11:15 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > > So something like isMouseActivatable?
> > 
> > Yep, that is the idea.
> > If you want to follow the iOS terminology: willRespondToMouseClickEvents().
> 
> Well actually they seem to have set of these to cover various events 
> willRespondToMouseClickEvents, willRespondToMouseMoveEvents, willRespondToMouseWheelEvents. I am not sure having multiple functions is any better than a generic one.

Okay I have decided to reuse the iOS terminology, but reimplement the functions.

I am implementing willRespondToMouseClickEvents and willRespondToMouseMoveEvents.
Comment 38 Antonio Gomes 2012-07-27 06:23:50 PDT
> Okay I have decided to reuse the iOS terminology, but reimplement the functions.
> 
> I am implementing willRespondToMouseClickEvents and willRespondToMouseMoveEvents.

what elements are you planning to add willRespondeToMouseMoveEvents to? What event respending to mouse move event would not get catch in willRespondeToMouseClick?

Not sure both are needed, but I might be missing something...
Comment 39 Allan Sandfeld Jensen 2012-07-27 06:29:00 PDT
(In reply to comment #38)
> > Okay I have decided to reuse the iOS terminology, but reimplement the functions.
> > 
> > I am implementing willRespondToMouseClickEvents and willRespondToMouseMoveEvents.
> 
> what elements are you planning to add willRespondeToMouseMoveEvents to? What event respending to mouse move event would not get catch in willRespondeToMouseClick?
> 
> Not sure both are needed, but I might be missing something...

The point is that these two functions not only tells of if the defaultEventHandler will react to the event but includes the check of hasEventListener.

The elements that would be missing by only using willRespondToMouseClick are those that react to mousemove events, which in most cases will be those with an eventlistener on mousemove.
Comment 40 Antonio Gomes 2012-07-27 06:31:30 PDT
(In reply to comment #39)
> (In reply to comment #38)
> > > Okay I have decided to reuse the iOS terminology, but reimplement the functions.
> > > 
> > > I am implementing willRespondToMouseClickEvents and willRespondToMouseMoveEvents.
> > 
> > what elements are you planning to add willRespondeToMouseMoveEvents to? What event respending to mouse move event would not get catch in willRespondeToMouseClick?
> > 
> > Not sure both are needed, but I might be missing something...
> 
> The point is that these two functions not only tells of if the defaultEventHandler will react to the event but includes the check of hasEventListener.
> 
> The elements that would be missing by only using willRespondToMouseClick are those that react to mousemove events, which in most cases will be those with an eventlistener on mousemove.

Makes sense.
Comment 41 Allan Sandfeld Jensen 2012-07-27 06:49:07 PDT
Created attachment 154934 [details]
Patch
Comment 42 Kenneth Rohde Christiansen 2012-07-27 07:06:00 PDT
I think this is a pretty okay way of doing it
Comment 43 Antonio Gomes 2012-07-27 07:24:32 PDT
Comment on attachment 154934 [details]
Patch

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

> Source/WebCore/dom/Node.h:577
> +    virtual bool willRespondToMouseMoveEvents();
> +    virtual bool willRespondToMouseClickEvents();

'const' here and throughout?
Comment 44 Allan Sandfeld Jensen 2012-07-27 07:36:30 PDT
(In reply to comment #43)
> (From update of attachment 154934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154934&action=review
> 
> > Source/WebCore/dom/Node.h:577
> > +    virtual bool willRespondToMouseMoveEvents();
> > +    virtual bool willRespondToMouseClickEvents();
> 
> 'const' here and throughout?

Good idea, I will add that with the next version. I am just adding a few test-cases.
Comment 45 Allan Sandfeld Jensen 2012-07-27 08:02:33 PDT
Created attachment 154953 [details]
Patch
Comment 46 Allan Sandfeld Jensen 2012-07-27 08:04:02 PDT
(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 154934 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154934&action=review
> > 
> > > Source/WebCore/dom/Node.h:577
> > > +    virtual bool willRespondToMouseMoveEvents();
> > > +    virtual bool willRespondToMouseClickEvents();
> > 
> > 'const' here and throughout?
> 
> Good idea, I will add that with the next version. I am just adding a few test-cases.

No, sorry. I forgot that hasEventListeners() is non-const for some reason, so these functions will have to be non-const as well.
Comment 47 Allan Sandfeld Jensen 2012-07-30 03:01:06 PDT
This patch willl need to be rebased before landing since the patch for bug #91894 landed, but it should still be suitable for reviewing.
Comment 48 Antonio Gomes 2012-07-30 06:04:23 PDT
Comment on attachment 154953 [details]
Patch

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

nice!

r=me

> Source/WebCore/ChangeLog:11
> +        Touch-adjustment is changed to using the two functions which improves accuracy when

use?
Comment 49 Allan Sandfeld Jensen 2012-07-30 06:33:34 PDT
Committed r124022: <http://trac.webkit.org/changeset/124022>
Comment 50 Adam Barth 2012-07-30 09:30:43 PDT
Comment on attachment 154953 [details]
Patch

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

> LayoutTests/touchadjustment/disabled-formelements.html:48
> +//         adjustedNode = testRoundTouch(16, 8, 5);
> +//         shouldBeEqualToString('adjustedNode.id', 'label1');

Did you intend to land this with commented out test code?
Comment 51 Allan Sandfeld Jensen 2012-07-30 11:42:12 PDT
(In reply to comment #49)
> Committed r124022: <http://trac.webkit.org/changeset/124022>

(In reply to comment #50)
> (From update of attachment 154953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154953&action=review
> 
> > LayoutTests/touchadjustment/disabled-formelements.html:48
> > +//         adjustedNode = testRoundTouch(16, 8, 5);
> > +//         shouldBeEqualToString('adjustedNode.id', 'label1');
> 
> Did you intend to land this with commented out test code?

Yes, I forgot to comment on that. They are to be un-commented once bug #91012 lands. I thought it might land before this one.
Comment 52 Allan Sandfeld Jensen 2012-07-30 11:45:31 PDT
(In reply to comment #51)
> (In reply to comment #49)
> > Committed r124022: <http://trac.webkit.org/changeset/124022>
> 
> (In reply to comment #50)
> > (From update of attachment 154953 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154953&action=review
> > 
> > > LayoutTests/touchadjustment/disabled-formelements.html:48
> > > +//         adjustedNode = testRoundTouch(16, 8, 5);
> > > +//         shouldBeEqualToString('adjustedNode.id', 'label1');
> > 
> > Did you intend to land this with commented out test code?
> 
> Yes, I forgot to comment on that. They are to be un-commented once bug #91012 lands. I thought it might land before this one.

Oh wait, never mind. I don't think I thought that through. I will remove them when I do the upcoming renovation on touchadjustment tests.