Bug 96810 – [touchadjustment] touch-links-longpress tests passes incorrectly

• Component: UI Events
• Status: RESOLVED FIXED

[Rick Byers]

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.

[Rick Byers]

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]

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]

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]

Created attachment 170639 [details]
Patch

[Rick Byers]

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 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]

(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]

(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]

(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]

(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]

Created attachment 170687 [details]
Patch

[Rick Byers]

(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]

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]

(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]

(In reply to comment #11)
> Created an attachment (id=170687) [details]
> Patch

This change looks good to me.

[Rick Byers]

Comment on attachment 170687 [details]
Patch

Thanks Antonio!

[WebKit Review Bot]

Comment on attachment 170687 [details]
Patch

Clearing flags on attachment: 170687

Committed r132929: <http://trac.webkit.org/changeset/132929>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.