RESOLVED FIXED 96810
[touchadjustment] touch-links-longpress tests passes incorrectly 
https://bugs.webkit.org/show_bug.cgi?id=96810
Summary [touchadjustment] touch-links-longpress tests passes incorrectly
Rick Byers
Reported 2012-09-14 11:13:17 PDT
the touchadjustment/touch-links-longpress.html test has some bugs that result in it passing without actually testing touch adjustment (it's actually touching on the elements, instead of nearby them). I've got fixes, as well as some changes to make the test more robust.
Attachments
Patch (7.63 KB, patch)
2012-10-25 07:08 PDT, Rick Byers 		no flags
DetailsFormatted DiffDiff
Patch (8.25 KB, patch)
2012-10-25 10:35 PDT, Rick Byers 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Rick Byers
Comment 1 2012-09-14 11:48:47 PDT
My fix for bug 96677 has to go in first (as it adds another test of this style). Once it lands, I'll update this (and any other necessary) tests with the same fixes.
Allan Sandfeld Jensen
Comment 2 2012-09-17 05:45:26 PDT
That adjustment works for context-menus is already be tested by the touchadjustment/context-menu-* tests. So all this test need to test, is that the adjustment is applied to the longpress event.
Allan Sandfeld Jensen
Comment 3 2012-10-02 03:30:59 PDT
Note this test also failed with the patch in bug #96908, this means some of the hits are also outside of the frame. If that is not intended, it should also be fixed.
Rick Byers
Comment 4 2012-10-25 07:08:12 PDT
Created attachment 170639 [details] Patch
Rick Byers
Comment 5 2012-10-25 07:10:21 PDT
Finally getting back to this little issue I discovered when working on bug 96677. My patch just applies the same technique I used there (touch adjustment tests for tap down gesture) to this test (longpress gesture). Allan / Terry - can you guys take a look at this little patch please?
Allan Sandfeld Jensen
Comment 6 2012-10-25 07:27:57 PDT
Comment on attachment 170639 [details] Patch You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap.
Rick Byers
Comment 7 2012-10-25 07:37:52 PDT
(In reply to comment #6) > (From update of attachment 170639 [details]) > You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap. This isn't a new test - it's already skipped there.
Allan Sandfeld Jensen
Comment 8 2012-10-25 07:59:28 PDT
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 170639 [details] [details]) > > You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap. > > This isn't a new test - it's already skipped there. Okay. I thought I had unskipped it when I implemented GestureLongPress.
Rick Byers
Comment 9 2012-10-25 10:22:09 PDT
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 170639 [details] [details] [details]) > > > You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap. > > > > This isn't a new test - it's already skipped there. > > Okay. I thought I had unskipped it when I implemented GestureLongPress. Oh I'm sorry - you're right, I didn't realize that had changed. It looks like the qt EWS passed though, does it run the touchadjustment tests? Perhaps (unlike chromium) qt doesn't require width/height to be supplied to enable touch adjustment (do you apply some default perhaps)? Anyway I'm happy to skip it again if you like. If it's failing now with my change then touch adjustment of long press really wasn't working before anyway - right?
Allan Sandfeld Jensen
Comment 10 2012-10-25 10:29:39 PDT
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > (From update of attachment 170639 [details] [details] [details] [details]) > > > > You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap. > > > > > > This isn't a new test - it's already skipped there. > > > > Okay. I thought I had unskipped it when I implemented GestureLongPress. > > Oh I'm sorry - you're right, I didn't realize that had changed. It looks like the qt EWS passed though, does it run the touchadjustment tests? Perhaps (unlike chromium) qt doesn't require width/height to be supplied to enable touch adjustment (do you apply some default perhaps)? > > Anyway I'm happy to skip it again if you like. If it's failing now with my change then touch adjustment of long press really wasn't working before anyway - right? Only Chromium and Mac runs the layout tests in the EWS. Qt only runs the layout-tests in the waterfall console.
Rick Byers
Comment 11 2012-10-25 10:35:58 PDT
Created attachment 170687 [details] Patch
Rick Byers
Comment 12 2012-10-25 10:37:02 PDT
(In reply to comment #11) > Created an attachment (id=170687) [details] > Patch (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > (In reply to comment #6) > > > > > (From update of attachment 170639 [details] [details] [details] [details] [details]) > > > > > You need to at least skip the test on Qt, and any other platforms that have implement EventSender::gestureLongTap. > > > > > > > > This isn't a new test - it's already skipped there. > > > > > > Okay. I thought I had unskipped it when I implemented GestureLongPress. > > > > Oh I'm sorry - you're right, I didn't realize that had changed. It looks like the qt EWS passed though, does it run the touchadjustment tests? Perhaps (unlike chromium) qt doesn't require width/height to be supplied to enable touch adjustment (do you apply some default perhaps)? > > > > Anyway I'm happy to skip it again if you like. If it's failing now with my change then touch adjustment of long press really wasn't working before anyway - right? > > Only Chromium and Mac runs the layout tests in the EWS. Qt only runs the layout-tests in the waterfall console. Thanks, I've skipped it for QT then (only change in this patch is to qt/TestExpectations).
WebKit Review Bot
Comment 13 2012-10-25 10:39:04 PDT
Attachment 170687 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/platform/qt/TestExpectations:2480: Path does not exist. [test/expectations] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Rick Byers
Comment 14 2012-10-25 10:55:02 PDT
(In reply to comment #13) > Attachment 170687 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > LayoutTests/platform/qt/TestExpectations:2480: Path does not exist. [test/expectations] [5] > Total errors found: 1 in 7 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. It looks like there's a rebaseline going on right now triggering this - eg. see r126957.
Terry Anderson
Comment 15 2012-10-29 13:38:35 PDT
(In reply to comment #11) > Created an attachment (id=170687) [details] > Patch This change looks good to me.
Rick Byers
Comment 16 2012-10-30 11:21:09 PDT
Comment on attachment 170687 [details] Patch Thanks Antonio!
WebKit Review Bot
Comment 17 2012-10-30 11:45:02 PDT
Comment on attachment 170687 [details] Patch Clearing flags on attachment: 170687 Committed r132929: <http://trac.webkit.org/changeset/132929>
WebKit Review Bot
Comment 18 2012-10-30 11:45:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.