Input elements with type=range do not have default touch handlers. Cannot manipulate a slider on a touch screen.
Created attachment 146912 [details] Patch
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.
Comment on attachment 146912 [details] Patch Attachment 146912 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12943352
Comment on attachment 146912 [details] Patch Attachment 146912 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12935821
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. :-)
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. :-)
Comment on attachment 147321 [details] Patch Attachment 147321 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12945896
Comment on attachment 147321 [details] Patch Attachment 147321 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12948818
Created attachment 147365 [details] Patch Fix to conditionally add touch support.
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.
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%".
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.
Created attachment 148160 [details] Patch
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.
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.
fwiw: this looks reasonable to me. Just add an additional test.
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.
Created attachment 148371 [details] Patch
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
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?
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.
Created attachment 149538 [details] Patch
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.
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?
@tkent: Are you interested in reviewing this patch?
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 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.
(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
Created attachment 150168 [details] Patch
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.
(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.
Comment on attachment 150168 [details] Patch Attachment 150168 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13117314
Created attachment 150172 [details] Patch
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?
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
(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...
Created attachment 151317 [details] Patch
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.
(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.
Comment on attachment 151317 [details] Patch thanks!
Created attachment 151473 [details] Patch
Added conditional expectations. Tests are expected to fail unless ENABLE_TOUCH_SLIDER is set, which is currently only enabled for Chromium.
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
The following ChangeLog files contain OOPS: trunk/LayoutTests/ChangeLog trunk/Source/WebCore/ChangeLog trunk/Source/WebKit/chromium/ChangeLog
Created attachment 151533 [details] Patch
Comment on attachment 151533 [details] Patch Clearing flags on attachment: 151533 Committed r122286: <http://trac.webkit.org/changeset/122286>
All reviewed patches have been landed. Closing bug.
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).
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).
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).