| Summary: | [touchadjustment] touch-links-longpress tests passes incorrectly | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rick Byers <rbyers> | ||||||
| Component: | UI Events | Assignee: | Rick Byers <rbyers> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | allan.jensen, tdanderson, tonikitoo, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 96677 | ||||||||
| Bug Blocks: | 100619 | ||||||||
| Attachments: |
|
||||||||
|
Description
Rick Byers
2012-09-14 11:13:17 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. 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. 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. Created attachment 170639 [details]
Patch
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? 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.
(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. (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. (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? (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. Created attachment 170687 [details]
Patch
(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). 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.
(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. (In reply to comment #11) > Created an attachment (id=170687) [details] > Patch This change looks good to me. Comment on attachment 170687 [details]
Patch
Thanks Antonio!
Comment on attachment 170687 [details] Patch Clearing flags on attachment: 170687 Committed r132929: <http://trac.webkit.org/changeset/132929> All reviewed patches have been landed. Closing bug. |