Bug 127760

Summary: Change a logic for touching slider in "RangeInput::handleTouchEvent"
Product: WebKit Reporter: Byung Jun Kim <bj1987.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: benjamin, buildbot, commit-queue, joepeck, jonlee, lucas.de.marchi, rniwa, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch none

Description Byung Jun Kim 2014-01-27 21:33:24 PST
If we touch slider and other side ,It weren't recognized as multi touch.
So change "TargetTouches" to "touches" in RangeInput::handleTouchEvent.
Comment 1 Byung Jun Kim 2014-01-27 23:06:12 PST
Created attachment 222407 [details]
Patch
Comment 2 Joseph Pecoraro 2014-01-28 11:10:55 PST
Comment on attachment 222407 [details]
Patch

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

> Source/WebCore/html/RangeInputType.cpp:177
> -    TouchList* touches = event->targetTouches();
> +    TouchList* touches = event->touches();
>      if (touches->length() == 1) {
>          typedSliderThumbElement().setPositionFromPoint(touches->item(0)->absoluteLocation());
>          event->setDefaultHandled();

This is in non-iOS code, so I probably won't be the final reviewer, but why would you want to change from targetTouches to touches here?

Could you try to better describe the scenario you're fixing? I am having trouble understanding your comment in the ChangeLog.
Comment 3 Byung Jun Kim 2014-03-30 21:10:59 PDT
Created attachment 228143 [details]
Patch
Comment 4 WebKit Commit Bot 2014-03-30 21:11:59 PDT
Attachment 228143 [details] did not pass style-queue:


ERROR: 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 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Byung Jun Kim 2014-03-30 21:16:56 PDT
Created attachment 228144 [details]
Patch
Comment 6 Byung Jun Kim 2014-03-30 21:18:14 PDT
In slider, nomal operation of Multi touch is changed from first touch.
So, thumb of slider should move only first touch.
But, If we touch thumb first and track second, slider moves from second touch.
Thumb and track is not same target. It isn't recognized multi touch because of "TargetTouch"
So we should change "touch" to "targetTouch".
Comment 7 WebKit Commit Bot 2014-03-30 21:18:31 PDT
Attachment 228144 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Byung Jun Kim 2014-03-30 21:22:24 PDT
Created attachment 228145 [details]
Patch
Comment 9 Jinwoo Song 2014-03-30 22:05:47 PDT
Comment on attachment 228145 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        So we should change "touch" to "targetTouch".

You'd better redescribe the change log as the sentences are not grammatical and it is hard to understand.
Comment 10 Build Bot 2014-03-30 22:19:29 PDT
Comment on attachment 228145 [details]
Patch

Attachment 228145 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6225372660105216

New failing tests:
fast/forms/range/slider-multi-touch.html
Comment 11 Build Bot 2014-03-30 22:19:32 PDT
Created attachment 228147 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2014-03-30 22:45:45 PDT
Comment on attachment 228145 [details]
Patch

Attachment 228145 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5021683916210176

New failing tests:
fast/forms/range/slider-multi-touch.html
Comment 13 Build Bot 2014-03-30 22:45:50 PDT
Created attachment 228149 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Byung Jun Kim 2014-04-01 21:00:07 PDT
Created attachment 228361 [details]
Patch
Comment 15 Byung Jun Kim 2014-04-01 21:01:48 PDT
If we touch thumb and track, slider should move from first touch. 
In current, slider is changed by second touch because of "targetTouches"
Thumb and track is not same target. So it isn't recognized multi touch.
So we should change "targetTouches" to "touches".
Comment 16 Gyuyoung Kim 2014-06-23 18:30:32 PDT
Comment on attachment 228361 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        If we touch thumb and track, slider should move from first touch. 

I don't still understand this description. So, I'm not 100% sure if my change is correct though, I modify this description as below,

When we touch thumb or track widget, *slider* on the thumb or track widgets is moved by first touch. However the slider under current implementation is worked by second touch. Because the slider is influenced by targetTouches() function as well. However, thumb and track widgets aren't same target widget, so those aren't able to be recognized by multiple touches. To fix this problem, we need to use *touches()* function instead of the *targetTouches()*
Comment 17 Gyuyoung Kim 2014-06-23 18:32:50 PDT
Comment on attachment 228361 [details]
Patch

Cleared review? from attachment 228361 [details] so that this bug does not appear in http://webkit.org/pending-review.