Bug 171792

Summary: [GTK] Test cases for typehead in form menu lists should start from known state
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WebKitGTKAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, clopez, commit-queue, mcatanzaro, webkit-bug-importer
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 171492    
Attachments:
Description Flags
Patch
none
Patch
none
Patch cgarcia: review+, cgarcia: commit-queue-

Adrian Perez
Reported 2017-05-07 05:58:09 PDT
See bug #171492 for details, in particular this comment: https://bugs.webkit.org/show_bug.cgi?id=171492#c2 The TL;DR is: * The “testTypeAheadFunction()“ function doesn't set an initial state: the result each call is dependent on the state in which previous calls left the element. * The fix for bug #170680 introduced pre-selection of the active element when the popup menu appears, to mimic the behavior of GtkComboBox. Before, when popping up the menu, the type-ahead search (which is implemented in GTK+) would always start from the beginning of the list. Now that we tell GTK+ to preactivate an item, it does not and the layout test breaks.
Attachments
Patch (1.53 KB, patch)
2017-05-07 06:07 PDT, Adrian Perez 		no flags
DetailsFormatted DiffDiff
Patch (1.88 KB, patch)
2017-05-07 06:08 PDT, Adrian Perez 		no flags
DetailsFormatted DiffDiff
Patch (1.88 KB, patch)
2017-05-07 13:40 PDT, Adrian Perez 		cgarcia: review+
cgarcia: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adrian Perez
Comment 1 2017-05-07 06:07:08 PDT
Created attachment 309321 [details] Patch
Adrian Perez
Comment 2 2017-05-07 06:08:51 PDT
Created attachment 309322 [details] Patch
Adrian Perez
Comment 3 2017-05-07 09:59:04 PDT
(In reply to Adrian Perez from comment #0) > See bug #171492 for details, in particular this comment: > > https://bugs.webkit.org/show_bug.cgi?id=171492#c2 > > [...] > > Before, when popping up the menu, the type-ahead search (which > is implemented in GTK+) [...] Well, forget about this comment: I have just noticed that the type-ahead search is implemented in “WebPopupMenuProxyGtk::typeAheadFind()”. The fact that the test should start from a known state still applies :-P
Michael Catanzaro
Comment 4 2017-05-07 12:42:36 PDT
What is the blur for?
Adrian Perez
Comment 5 2017-05-07 13:32:55 PDT
(In reply to Michael Catanzaro from comment #4) > What is the blur for? It causes the HTML element to lose keyboard focus: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/blur When the HTML for the layout test is loaded, the <select> element is not focused, so calling “.blur()” on it ensures that it is not focused before “testTypeAheadFunction()” starts sending mouse and keyboard events, regardless of the state it was left on previous uses of the function. (NB: It does not really make a difference here, but for consistency it seems like a good idea to remove the focus from the element as well to guarantee the same state as after page load.)
Adrian Perez
Comment 6 2017-05-07 13:40:12 PDT
Created attachment 309327 [details] Patch
Adrian Perez
Comment 7 2017-05-07 13:42:03 PDT
I did some double checking, and right after page load the first element is the selected one, so the patch is not updated to set “.selectedIndex=0”.
Carlos Garcia Campos
Comment 8 2017-05-30 07:18:52 PDT
Comment on attachment 309327 [details] Patch Wait, r171792 has nothing to do with this, it's a very old revision
Carlos Garcia Campos
Comment 9 2017-05-30 07:19:31 PDT
(In reply to Carlos Garcia Campos from comment #8) > Comment on attachment 309327 [details] > Patch > > Wait, r171792 has nothing to do with this, it's a very old revision It seems you used the bug number, instead of the revision number.
Adrian Perez
Comment 10 2017-05-30 07:28:01 PDT
(In reply to Carlos Garcia Campos from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > Comment on attachment 309327 [details] > > Patch > > > > Wait, r171792 has nothing to do with this, it's a very old revision > > It seems you used the bug number, instead of the revision number. You are right, the correct revision is r215188: http://trac.webkit.org/changeset/215188 I'll update the ChangeLog accordingly before landing this.
Adrian Perez
Comment 11 2017-05-30 07:33:28 PDT
Committed r217554: <http://trac.webkit.org/changeset/217554>
Note You need to log in before you can comment on or make changes to this bug.