Bug 88807 – Input elements with type=range do not have default touch handlers.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kevin Ellis]

Input elements with type=range do not have default touch handlers.  Cannot manipulate a slider on a touch screen.

[Kevin Ellis]

Created attachment 146912 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


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

[Build Bot]

Comment on attachment 146912 [details]
Patch

Attachment 146912 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12943352

[Build Bot]

Comment on attachment 146912 [details]
Patch

Attachment 146912 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12935821

[Robert Kroeger]

Comment on attachment 146912 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

You need to update this or it will get bounced from the commit queue.

Also, a longer more detailed ChangeLog is necessary.

> Source/WebCore/dom/Touch.cpp:76
> +{

you will need to test your slider inside a transform. Like rotated.

> Source/WebCore/dom/Touch.h:69
> +    RefPtr<Frame> m_frame;

this seems wrong to me. How do other events deal with this?

> Source/WebCore/dom/TouchEvent.h:72
> +    virtual bool isTouchEvent() const OVERRIDE;

afaik WebCore doesn't use the OVERRIDE macro

> Source/WebCore/html/RangeInputType.cpp:37
> +#include "Frame.h"

i expect that you should avoid this.

> Source/WebCore/html/RangeInputType.cpp:155
> +        event->preventDefault();

you are the default handler? do you need to call this?

> Source/WebCore/html/RangeInputType.h:54
> +    virtual void handleTouchEvent(TouchEvent*) OVERRIDE;

ok... maybe I was wrong about the OVERRIDE. :-)

[Kevin Ellis]

Created attachment 147321 [details]
Patch

(In reply to comment #5)
> (From update of attachment 146912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146912&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        No new tests. (OOPS!)
> > 
> > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> You need to update this or it will get bounced from the commit queue.
> 
> Also, a longer more detailed ChangeLog is necessary.
> 
Added touch slider test, and more detail in the Change Log.

> > Source/WebCore/dom/Touch.cpp:76
> > +{
> 
> you will need to test your slider inside a transform. Like rotated.
> 
> > Source/WebCore/dom/Touch.h:69
> > +    RefPtr<Frame> m_frame;
> 
> this seems wrong to me. How do other events deal with this?
> 
Removed keeping a reference to the frame.  The thumb scroll element relies on absolute page position and not adjusted page position.  The logic for correcting the page scale and zoom is consistent with MouseRelatedEvent.

> > Source/WebCore/dom/TouchEvent.h:72
> > +    virtual bool isTouchEvent() const OVERRIDE;
> 
> afaik WebCore doesn't use the OVERRIDE macro
> 
> > Source/WebCore/html/RangeInputType.cpp:37
> > +#include "Frame.h"
> 
> i expect that you should avoid this.
> 
> > Source/WebCore/html/RangeInputType.cpp:155
> > +        event->preventDefault();
> 
> you are the default handler? do you need to call this?
> 
Unless preventDefault is called, the gesture recognizer can still scroll the page while trying to drag the slider.

> > Source/WebCore/html/RangeInputType.h:54
> > +    virtual void handleTouchEvent(TouchEvent*) OVERRIDE;
> 
> ok... maybe I was wrong about the OVERRIDE. :-)

[Build Bot]

Comment on attachment 147321 [details]
Patch

Attachment 147321 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12945896

[Build Bot]

Comment on attachment 147321 [details]
Patch

Attachment 147321 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12948818

[Kevin Ellis]

Created attachment 147365 [details]
Patch

Fix to conditionally add touch support.

[Kent Tamura]

Comment on attachment 147365 [details]
Patch

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

> Source/WebCore/html/RangeInputType.cpp:156
> +    if (event->type() == eventNames().touchendEvent)
> +        return;
> +
> +    if (element()->disabled() || element()->readOnly())
> +        return;
> +
> +    TouchList* touches = event->targetTouches();
> +    if (touches->length() == 1) {
> +        Touch* touch = touches->item(0);
> +        SliderThumbElement* thumb = sliderThumbElementOf(element());
> +        thumb->setPositionFromPoint(touch->absoluteLocation());

So, we don't start dragging the thumb unlike mousedown.  Is it intentional?  Is ti compatible with synthetic mouse events?

> Source/WebCore/html/RangeInputType.cpp:158
> +        event->preventDefault();

preventDefault is not needed.

[Kevin Ellis]

Comment on attachment 147365 [details]
Patch

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

>> Source/WebCore/html/RangeInputType.cpp:156
>> +        thumb->setPositionFromPoint(touch->absoluteLocation());
> 
> So, we don't start dragging the thumb unlike mousedown.  Is it intentional?  Is ti compatible with synthetic mouse events?

Yes, not triggering drag on the thumb element is deliberate.  For mouse handling, there is extra work to be done to start and stop mouse capture on the thumb element.  For touch events, subsequent moves are delivered to the same target as the touch start, so no additional capture control is needed.  Handling touch moves inside of the thumb element would still require forwarding the event from the input element.  Implementing a simple snap-to directly from the range input type seemed like a reasonable solution.

>> Source/WebCore/html/RangeInputType.cpp:158
>> +        event->preventDefault();
> 
> preventDefault is not needed.

Without preventDefault, the gesture recognizer in Chrome can trigger scrolling.  Tested with and without preventDefault using a slider inside of a div with width="200%".

[Robert Kroeger]

Comment on attachment 147365 [details]
Patch

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

>>> Source/WebCore/html/RangeInputType.cpp:158
>>> +        event->preventDefault();
>> 
>> preventDefault is not needed.
> 
> Without preventDefault, the gesture recognizer in Chrome can trigger scrolling.  Tested with and without preventDefault using a slider inside of a div with width="200%".

This might be so but I concur with tkent@ that it is probably not right.

I believe that this patch is the first time that we introduce a default action on touch events that do not involve punting off to the gesture recognizer. Since there are *no* other touch default actions, setDefaultHandled probably does not have the right consequence in EventHandler::handleTouchEvent

Note how mouse events compute the boolean that is returned to the embedder: http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/MouseEvent.cpp&exact_package=chromium&q=MouseEventDispatchMediator&type=cs&l=208

where swallowEvent = event()->defaultHandled() || event()->defaultPrevented();

I think that http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&exact_package=chromium&q=handleTouchEvent&type=cs&l=3617

needs to be altered to:

defaultPrevented = defaultPrevented || touchEvent->defaultPrevented() || touchEvent->defaultHandled();

btw: this means that defaultPrevented should be renamed to swallowEvent in EventHandler::handleTouchEvent.

[Kevin Ellis]

Created attachment 148160 [details]
Patch

[Kevin Ellis]

Comment on attachment 147365 [details]
Patch

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

>>>> Source/WebCore/html/RangeInputType.cpp:158
>>>> +        event->preventDefault();
>>> 
>>> preventDefault is not needed.
>> 
>> Without preventDefault, the gesture recognizer in Chrome can trigger scrolling.  Tested with and without preventDefault using a slider inside of a div with width="200%".
> 
> This might be so but I concur with tkent@ that it is probably not right.
> 
> I believe that this patch is the first time that we introduce a default action on touch events that do not involve punting off to the gesture recognizer. Since there are *no* other touch default actions, setDefaultHandled probably does not have the right consequence in EventHandler::handleTouchEvent
> 
> Note how mouse events compute the boolean that is returned to the embedder: http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/MouseEvent.cpp&exact_package=chromium&q=MouseEventDispatchMediator&type=cs&l=208
> 
> where swallowEvent = event()->defaultHandled() || event()->defaultPrevented();
> 
> I think that http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&exact_package=chromium&q=handleTouchEvent&type=cs&l=3617
> 
> needs to be altered to:
> 
> defaultPrevented = defaultPrevented || touchEvent->defaultPrevented() || touchEvent->defaultHandled();
> 
> btw: this means that defaultPrevented should be renamed to swallowEvent in EventHandler::handleTouchEvent.

Confirmed that preventDefault is no longer needed with Rob's patch.  Removed.

[Robert Kroeger]

Comment on attachment 148160 [details]
Patch

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

> LayoutTests/fast/events/touch/script-tests/touch-slider.js:6
> +slider.style.height = "30px";

please add a test where the slider is CSS transformed (scale, rotation) to be sure that things work correctly.

[Robert Kroeger]

fwiw: this looks reasonable to me. Just add an additional test.

[Kent Tamura]

Comment on attachment 148160 [details]
Patch

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

Changes in WebCore/html/ looks ok.
I'm not sure WebCore/dom/ changes.

> LayoutTests/fast/events/touch/script-tests/touch-slider.js:63
> +    eventSender.addTouchPoint(x + w/2, y);
> +    eventSender.touchStart();
> +
> +    eventSender.updateTouchPoint(0, x, y);
> +    eventSender.touchMove();
> +
> +    eventSender.updateTouchPoint(0, x + w, y);
> +    eventSender.touchMove();
> +
> +    eventSender.updateTouchPoint(0, x + w/2, y);

Let's move y position too.

[Kevin Ellis]

Created attachment 148371 [details]
Patch

[Kevin Ellis]

Comment on attachment 148160 [details]
Patch

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

>> LayoutTests/fast/events/touch/script-tests/touch-slider.js:6
>> +slider.style.height = "30px";
> 
> please add a test where the slider is CSS transformed (scale, rotation) to be sure that things work correctly.

Added rotated slider and slider with "-webkit-appearance: slider-vertical".

>> LayoutTests/fast/events/touch/script-tests/touch-slider.js:63
>> +    eventSender.updateTouchPoint(0, x + w/2, y);
> 
> Let's move y position too.

Done

[Ben Murdoch]

Comment on attachment 148371 [details]
Patch

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

> Source/WebCore/html/RangeInputType.cpp:146
> +    if (event->type() == eventNames().touchendEvent)

should we also set default handled in this case?

[Robert Kroeger]

Comment on attachment 148371 [details]
Patch

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

>> Source/WebCore/html/RangeInputType.cpp:146
>> +    if (event->type() == eventNames().touchendEvent)
> 
> should we also set default handled in this case?

good catch. This makes sense.

[Kevin Ellis]

Created attachment 149538 [details]
Patch

[Kevin Ellis]

Comment on attachment 148371 [details]
Patch

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

>>> Source/WebCore/html/RangeInputType.cpp:146
>>> +    if (event->type() == eventNames().touchendEvent)
>> 
>> should we also set default handled in this case?
> 
> good catch. This makes sense.

Done.

[Adam Barth]

Comment on attachment 149538 [details]
Patch

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

> Source/WebCore/dom/Touch.cpp:74
> +    float scaleFactor = frame->pageZoomFactor() * frame->frameScaleFactor();
> +    float x = pageX * scaleFactor;
> +    float y = pageY * scaleFactor;

I'm not sure whether this is the right idiom.  Should we be using FrameView::mapFromCSSToLayoutUnits instead?

[Adam Barth]

@tkent: Are you interested in reviewing this patch?

[Antonio Gomes]

some questions: does chromium dor android do any touch-to-mouse conversion?

In the way we did for the BlackBerry port, in the case of a input slide, the touch move event is not handled, but the mouse move event converted from the touch move is

It was basically one-line of code (to be upstreamed):

270 void SliderThumbElement::defaultEventHandler(Event* event)
271 {
272     if (!event->isMouseEvent()) {
273         HTMLDivElement::defaultEventHandler(event);
274         return;
275     }
...
292     // mouse events.
293     if (eventType == eventNames().mousedownEvent && isLeftButton) {
294         startDragging();
295         return;
296     } else if (eventType == eventNames().mouseupEvent && isLeftButton) {
297         stopDragging();
298         return;
299     } else if (eventType == eventNames().mousemoveEvent) {
300         if (m_inDragMode)
301             setPositionFromPoint(mouseEvent->absoluteLocation());
302 #if PLATFORM(BLACKBERRY)
303         // On Playbook ENABLE(DRAG_SUPPORT) is false, so consume event here
304         event->setDefaultHandled();
305 #endif
306         return;

[Kevin Ellis]

(In reply to comment #26)
> some questions: does chromium dor android do any touch-to-mouse conversion?
> 
> In the way we did for the BlackBerry port, in the case of a input slide, the touch move event is not handled, but the mouse move event converted from the touch move is
> 
> It was basically one-line of code (to be upstreamed):
> 
> 270 void SliderThumbElement::defaultEventHandler(Event* event)
> 271 {
> 272     if (!event->isMouseEvent()) {
> 273         HTMLDivElement::defaultEventHandler(event);
> 274         return;
> 275     }
> ...
> 292     // mouse events.
> 293     if (eventType == eventNames().mousedownEvent && isLeftButton) {
> 294         startDragging();
> 295         return;
> 296     } else if (eventType == eventNames().mouseupEvent && isLeftButton) {
> 297         stopDragging();
> 298         return;
> 299     } else if (eventType == eventNames().mousemoveEvent) {
> 300         if (m_inDragMode)
> 301             setPositionFromPoint(mouseEvent->absoluteLocation());
> 302 #if PLATFORM(BLACKBERRY)
> 303         // On Playbook ENABLE(DRAG_SUPPORT) is false, so consume event here
> 304         event->setDefaultHandled();
> 305 #endif
> 306         return;

In Chromium, only tap and scroll gestures are converted to synthetic mouse events.  For the input slider, no synthetic mouse events are generated for a drag operation.

[Antonio Gomes]

(In reply to comment #27)
> (In reply to comment #26)
> > some questions: does chromium dor android do any touch-to-mouse conversion?
> > 
> > In the way we did for the BlackBerry port, in the case of a input slide, > 
> In Chromium, only tap and scroll gestures are converted to synthetic mouse events.  For the input slider, no synthetic mouse events are generated for a drag operation.

Ok, for BlackBerry that could also need some other bits to be plumbed in to make use of it

4027 bool WebPage::touchEvent(const Platform::TouchEvent& event)
4028 {
...
4086     bool handled = false;
4087 
4088     if (d->m_needTouchEvents && !event.hasGesture(Platform::Gesture::Injected))
4089         handled = d->m_mainFrame->eventHandler()->handleTouchEvent(PlatformTouchEvent(&tEvent))

the first condition in the IF allows us to skip submitting Touch events to WebCore if there is not touch listeners attached to the page.

See also Document.cpp

4040 void Document::addListenerTypeIfNeeded(const AtomicString& eventType)
4041 {
...
4068 #if ENABLE(TOUCH_EVENTS)
4069     else if (eventType == eventNames().touchstartEvent
4070              || eventType == eventNames().touchmoveEvent
4071              || eventType == eventNames().touchendEvent
4072              || eventType == eventNames().touchcancelEvent) {
4073         addListenerType(TOUCH_LISTENER);
4074         if (Page* page = this->page())
4075             page->chrome()->client()->needTouchEvents(true);
4076     }
4077 #endif

[Kevin Ellis]

Created attachment 150168 [details]
Patch

[Kevin Ellis]

Comment on attachment 149538 [details]
Patch

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

>> Source/WebCore/dom/Touch.cpp:74
>> +    float y = pageY * scaleFactor;
> 
> I'm not sure whether this is the right idiom.  Should we be using FrameView::mapFromCSSToLayoutUnits instead?

Consistent with approach used in MouseReleatedEvent.cpp and EventHandler.cpp for handling scaling.

[Kevin Ellis]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > some questions: does chromium dor android do any touch-to-mouse conversion?
> > > 
> > > In the way we did for the BlackBerry port, in the case of a input slide, > 
> > In Chromium, only tap and scroll gestures are converted to synthetic mouse events.  For the input slider, no synthetic mouse events are generated for a drag operation.
> 
> Ok, for BlackBerry that could also need some other bits to be plumbed in to make use of it
> 
> 4027 bool WebPage::touchEvent(const Platform::TouchEvent& event)
> 4028 {
> ...
> 4086     bool handled = false;
> 4087 
> 4088     if (d->m_needTouchEvents && !event.hasGesture(Platform::Gesture::Injected))
> 4089         handled = d->m_mainFrame->eventHandler()->handleTouchEvent(PlatformTouchEvent(&tEvent))
> 
> the first condition in the IF allows us to skip submitting Touch events to WebCore if there is not touch listeners attached to the page.
> 
> See also Document.cpp
> 
> 4040 void Document::addListenerTypeIfNeeded(const AtomicString& eventType)
> 4041 {
> ...
> 4068 #if ENABLE(TOUCH_EVENTS)
> 4069     else if (eventType == eventNames().touchstartEvent
> 4070              || eventType == eventNames().touchmoveEvent
> 4071              || eventType == eventNames().touchendEvent
> 4072              || eventType == eventNames().touchcancelEvent) {
> 4073         addListenerType(TOUCH_LISTENER);
> 4074         if (Page* page = this->page())
> 4075             page->chrome()->client()->needTouchEvents(true);
> 4076     }
> 4077 #endif

Added mechanism to add/remove the touch handler.

[Build Bot]

Comment on attachment 150168 [details]
Patch

Attachment 150168 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13117314

[Kevin Ellis]

Created attachment 150172 [details]
Patch

[Antonio Gomes]

I think the patch looks fine, but I would like a way to opt-out it (maybe a USE macro is an overkill?), so BlackBerry port have time to adjust to the implications of this change.

I.e., we will certainly make use of it, and it will save us lots of special cases we handle, but as is it will break other cases.

Does it sound reasonable?

[Antonio Gomes]

Comment on attachment 150172 [details]
Patch

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

> Source/WebCore/html/InputType.cpp:887
> +bool InputType::hasTouchEventHandler()
> +{
> +    return false;
> +}
> +#endif

const

> Source/WebCore/html/RangeInputType.h:56
> +    virtual bool hasTouchEventHandler() OVERRIDE;

const

[Antonio Gomes]

(In reply to comment #34)
> I think the patch looks fine, but I would like a way to opt-out it (maybe a USE macro is an overkill?), so BlackBerry port have time to adjust to the implications of this change.
> 
> I.e., we will certainly make use of it, and it will save us lots of special cases we handle, but as is it will break other cases.
> 
> Does it sound reasonable?

At this point, my main concern is about media (video and audio) timeline elements working with it...

[Kevin Ellis]

Created attachment 151317 [details]
Patch

[Kevin Ellis]

Comment on attachment 150172 [details]
Patch

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

>> Source/WebCore/html/InputType.cpp:887
>> +#endif
> 
> const

Done.

>> Source/WebCore/html/RangeInputType.h:56
>> +    virtual bool hasTouchEventHandler() OVERRIDE;
> 
> const

Done.

[Kevin Ellis]

(In reply to comment #36)
> (In reply to comment #34)
> > I think the patch looks fine, but I would like a way to opt-out it (maybe a USE macro is an overkill?), so BlackBerry port have time to adjust to the implications of this change.
> > 
> > I.e., we will certainly make use of it, and it will save us lots of special cases we handle, but as is it will break other cases.
> > 
> > Does it sound reasonable?
> 
> At this point, my main concern is about media (video and audio) timeline elements working with it...

Added ENABLE(TOUCH_SLIDER) for conditional support.  Currently enabled for Chromium only.  

Tested on video and audio tags, and fix for <input type="range"> works for the other tags as well.

[Antonio Gomes]

Comment on attachment 151317 [details]
Patch

thanks!

[Kevin Ellis]

Created attachment 151473 [details]
Patch

[Kevin Ellis]

Added conditional expectations.  Tests are expected to fail unless ENABLE_TOUCH_SLIDER is set, which is currently only enabled for Chromium.

[WebKit Review Bot]

Comment on attachment 151473 [details]
Patch

Rejecting attachment 151473 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11988 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 11988.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13198090

[Adam Barth]

    The following ChangeLog files contain OOPS:

        trunk/LayoutTests/ChangeLog
        trunk/Source/WebCore/ChangeLog
        trunk/Source/WebKit/chromium/ChangeLog

[Kevin Ellis]

Created attachment 151533 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151533 [details]
Patch

Clearing flags on attachment: 151533

Committed r122286: <http://trac.webkit.org/changeset/122286>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 148371 [details]
Patch

Cleared review? from obsolete attachment 148371 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Adam Barth]

Comment on attachment 149538 [details]
Patch

Cleared review? from obsolete attachment 149538 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Adam Barth]

Comment on attachment 150172 [details]
Patch

Cleared review? from obsolete attachment 150172 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).