171492 2017-05-01 01:37:40 -0700 REGRESSION(r215188?): Test platform/gtk/fast/forms/menulist-typeahead-find.html is failing 2017-07-02 09:19:43 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=170680 https://bugs.webkit.org/show_bug.cgi?id=40601 Gtk, LayoutTestFailure P2 Normal --- 171792 172708 171803 1 cgarcia aperez aperez bugs-noreply mcatanzaro oldest_to_newest 1303207 0 cgarcia 2017-05-01 01:37:40 -0700 r215187 NOERROR [r215188-r215192] UNKNOWN r215193 TEXT (Expected: PASS) 1303209 1 cgarcia 2017-05-01 01:40:15 -0700 Expectations updated in r216011 1305712 2 aperez 2017-05-07 05:41:38 -0700 This started failing after the following set of fixes which I made for fixing the following popup-related bugs: * Bug #145866: [GTK] Attach popup menu to web view widget Landed: http://trac.webkit.org/changeset/215225 * Bug #170553: [GTK] Misplaced right click menu on web page due to deprecated gtk_menu_popup() Landed: http://trac.webkit.org/changeset/215190 * Bug #170680: [GTK] Opening a popup menu does not pre-select the active item Landed: http://trac.webkit.org/changeset/215188 Reverting r215225 makes the typeahead find elements again, but the items being found sometimes are not exactly the ones from the test expectation. The diff is: --- /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/fast/forms/menulist-typeahead-find-expected.txt +++ /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/fast/forms/menulist-typeahead-find-actual.txt @@ -3,8 +3,8 @@ Cannot run interactively. Expected Apple: was Apple Expected Carrot: was Carrot -Expected Cauliflower: was Cauliflower -Expected Corn: was Corn +Expected Cauliflower: was Carrot +Expected Corn: was Cauliflower Expected Carrot: was Carrot Expected Apple: was Apple Expected Cauliflower: was Cauliflower The bit that is not working is this part of the test: // This tests that iterating via pressing the same key works. testTypeAheadFind(['c', 'c'], "Cauliflower"); testTypeAheadFind(['c', 'c', 'c'], "Corn"); Checking the code of the “testTypeAheadFind()” function, I see that it does not attempt to reset the status of the form element to an initial know state, so each call to this function will behave differently depending on the state leftover from the previous call. After fixing bug #170680 now the code is pre-selecting the active item on popup (simulated here by sending a pair of button-down/button-up events): that was not happening before and the typeahead was searching always from the beginning of the list. IMHO the correct thing to do in this regard is to always reset the form element to a known state at the beginning of “testTypeAheadFind()”. I still don't know why attaching the popup menu to the web widget makes the test function always choose the “Apple” item, though. That needs more investigation. 1305720 3 aperez 2017-05-07 10:45:44 -0700 The following is the diff with the patch in bug #171792 applied, but *not* reverting the patch to attach the popup to the web view: --- /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/fast/forms/menulist-typeahead-find-expected.txt +++ /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/gtk/fast/forms/menulist-typeahead-find-actual.txt @@ -2,10 +2,10 @@ Cannot run interactively. Expected Apple: was Apple -Expected Carrot: was Carrot -Expected Cauliflower: was Cauliflower -Expected Corn: was Corn -Expected Carrot: was Carrot -Expected Apple: was Apple -Expected Cauliflower: was Cauliflower -Expected Star fruit: was Star fruit +Expected Carrot: was +Expected Cauliflower: was +Expected Corn: was +Expected Carrot: was +Expected Apple: was +Expected Cauliflower: was +Expected Star fruit: was It's quite interesting that the first attempt of using the type ahead search works, but the rest don't. This leads me to think that it might be needed to detach the popup from the web view after the popup menu is closed (either by choosing a new value with keyboard/mouse, or by cancelling it e.g. with “Escape”). I'll give that a try, but if that doesn't solve the issue, it could be that some digging into the GTK+ code for gtk_menu_attach_to_widget() may be needed. 1305751 4 aperez 2017-05-07 15:13:17 -0700 (In reply to Adrian Perez from comment #3) > The following is the diff with the patch in bug #171792 applied, but > *not* reverting the patch to attach the popup to the web view: > > [...] > > It's quite interesting that the first attempt of using the type ahead > search works, but the rest don't [...] This happens because gtk_menu_attach_to_widget() is sinking the floating reference to the passed GtkMenu, making the GTK+ innards “own” the widget. That means that GTK+ unrefs the GtkMenu and we are left with an invalid “m_popup” pointer. We need to either sink the reference ourselves when constructing a WebPopupMenuProxyGtk, or add an additional reference. Doing one of those fixes the case of no items appearing as selectd after the first one — but I still need to figure out how to get the test to pass. 1307241 5 aperez 2017-05-11 05:21:15 -0700 Some observations I made while trying to figure out what's going on: 1. The first time a popup is opened, a WebPopupMenuProxyGtk is instantiated, and ::showPopupMenu() called on it. 2. Once the menu is dismissed, the WebPopupMenuGtk instance is kept around. 3. The next times a menu is opened: 3.a. The object created in (1) is destroyed. 3.b. Then a new WebPopupMenuProxyGtk is created (even if the menu being opened is exactly the same one used previously). 3.c. WebPopupMenuProxyGtk::showPopupMenu() is called. I double-checked the above by debugging while using MiniBrowser. When using WebKitTestRunner to run the test, only steps (1) to (3.a) happen, and new instances of WebPopupMenuProxyGtk are not ever created. It looks like the events synthesized with “eventSender.mouse{Down,Up}()” used to programmatically open the popup menu from the test are only reaching WebCore on the first use, and then things get stalled from there. 1313676 6 cgarcia 2017-05-30 05:49:02 -0700 (In reply to Adrian Perez from comment #2) > This started failing after the following set of fixes which I made for > fixing the following popup-related bugs: > > * Bug #145866: [GTK] Attach popup menu to web view widget > Landed: http://trac.webkit.org/changeset/215225 > > * Bug #170553: [GTK] Misplaced right click menu on web page due to > deprecated gtk_menu_popup() > Landed: http://trac.webkit.org/changeset/215190 > > * Bug #170680: [GTK] Opening a popup menu does not pre-select the active > item > Landed: http://trac.webkit.org/changeset/215188 > > Reverting r215225 makes the typeahead find elements again, but the > items being found sometimes are not exactly the ones from the test > expectation. The diff is: > > --- > /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/ > gtk/fast/forms/menulist-typeahead-find-expected.txt > +++ > /home/aperez/devel/WebKit/WebKitBuild/Release/layout-test-results/platform/ > gtk/fast/forms/menulist-typeahead-find-actual.txt > @@ -3,8 +3,8 @@ > Cannot run interactively. > Expected Apple: was Apple > Expected Carrot: was Carrot > -Expected Cauliflower: was Cauliflower > -Expected Corn: was Corn > +Expected Cauliflower: was Carrot > +Expected Corn: was Cauliflower > Expected Carrot: was Carrot > Expected Apple: was Apple > Expected Cauliflower: was Cauliflower > > The bit that is not working is this part of the test: > > // This tests that iterating via pressing the same key works. > testTypeAheadFind(['c', 'c'], "Cauliflower"); > testTypeAheadFind(['c', 'c', 'c'], "Corn"); > > Checking the code of the “testTypeAheadFind()” function, I see that it > does not attempt to reset the status of the form element to an initial > know state, so each call to this function will behave differently > depending on the state leftover from the previous call. > > After fixing bug #170680 now the code is pre-selecting the active item > on popup (simulated here by sending a pair of button-down/button-up > events): that was not happening before and the typeahead was searching > always from the beginning of the list. IMHO the correct thing to do in > this regard is to always reset the form element to a known state at the > beginning of “testTypeAheadFind()”. > > I still don't know why attaching the popup menu to the web widget makes > the test function always choose the “Apple” item, though. That needs more > investigation. This is because we are dismissing any menus attached to the web view for every event we inject. This was done for the context menu in r184015, but now we are also attaching popup menus. So, we show the popup menu and as soon as we try to inject a keyevent, the popup is dismissed. That was a workaround in the end, so it's time to properly fix bug #40601. 1313754 7 aperez 2017-05-30 10:01:11 -0700 (In reply to Carlos Garcia Campos from comment #6) > This is because we are dismissing any menus attached to the web view for > every event we inject. This was done for the context menu in r184015, but > now we are also attaching popup menus. So, we show the popup menu and as > soon as we try to inject a keyevent, the popup is dismissed. That was a > workaround in the end, so it's time to properly fix bug #40601. Wow, I wasn't aware of the workaround. Thanks a lot for looking into this and figuring out the reason for the weird behavior of the event handling in WebKitTestRunner. 1319959 8 313077 aperez 2017-06-16 08:27:04 -0700 Created attachment 313077 Patch 1319960 9 aperez 2017-06-16 08:34:20 -0700 Committed r218391: <http://trac.webkit.org/changeset/218391> 313077 2017-06-16 08:27:04 -0700 2017-07-02 09:19:43 -0700 Patch bug-171492-20170616182702.patch text/plain 1593 aperez U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4Mzg3CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggNTgyNTFmMjUwNTIyNmQ0ZWFlMDJj YmM1NWRlOGJmYTJlZTMwYjUxMC4uMTFmOTE1ZmMwYTY0Y2Q4YTY4OTQ4NDc5NTE4N2QyZjBiNjg5 OTVkMCAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE3LTA2LTE2ICBBZHJpYW4gUGVyZXogZGUg Q2FzdHJvICA8YXBlcmV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjE1MTg4 Pyk6IFRlc3QgcGxhdGZvcm0vZ3RrL2Zhc3QvZm9ybXMvbWVudWxpc3QtdHlwZWFoZWFkLWZpbmQu aHRtbCBpcyBmYWlsaW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xNzE0OTIKKworICAgICAgICBVbnJldmlld2VkIGdhcmRlbmluZy4KKworICAgICAg ICAqIHBsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zOiBUaGUgZml4ZXMgbmVlZGVkIGZvciB0 aGUgdGVzdCB0byBwYXNzCisgICAgICAgIGhhdmUgbGFuZGVkLCBzbyB0aGUgdGVzdCBjYW4gYmUg dW5za2lwcGVkLgorCiAyMDE3LTA2LTE2ICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlp QHNvbnkuY29tPgogCiAgICAgICAgIFtHVEtdIExheW91dCBUZXN0IGZhc3QvY2FudmFzL3dlYmds L3RleC1pbWFnZS1hbmQtc3ViLWltYWdlLTJkLXdpdGgtdmlkZW8uaHRtbCBtYWtlcyB0aGUgc3Vi c2VxdWVudCB0ZXN0IGNhc2UgZmxha3kgY3Jhc2guCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9w bGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucyBiL0xheW91dFRlc3RzL3BsYXRmb3JtL2d0ay9U ZXN0RXhwZWN0YXRpb25zCmluZGV4IGJkNjZiMTNkYmNiZDA4MDZmMmQwYzViOWVkOWFlN2U2YzI1 NmRiZGYuLjI2NTc4NWRmOTI1YzIxOGU3ZjI4M2EyZDc1OWJlNzExNDY1MWQ0MWQgMTAwNjQ0Ci0t LSBhL0xheW91dFRlc3RzL3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zCisrKyBiL0xheW91 dFRlc3RzL3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zCkBAIC0zMzg2LDggKzMzODYsNiBA QCB3ZWJraXQub3JnL2IvMTcwMDUzIG1lZGlhL3ZpZGVvLWxvYWQtcmVxdWlyZS11c2VyLWdlc3R1 cmUuaHRtbCBbIEZhaWx1cmUgXQogCiB3ZWJraXQub3JnL2IvMTcwMzM1IGNhbnZhcy9waGlsaXAv dGVzdHMvMmQucGF0dGVybi5hbmltYXRlZC5naWYuaHRtbCBbIEZhaWx1cmUgXQogCi13ZWJraXQu b3JnL2IvMTcxNDkyIHBsYXRmb3JtL2d0ay9mYXN0L2Zvcm1zL21lbnVsaXN0LXR5cGVhaGVhZC1m aW5kLmh0bWwgWyBGYWlsdXJlIF0KLQogd2Via2l0Lm9yZy9iLzE3MTU5OCBhY2Nlc3NpYmlsaXR5 L2d0ay9tZW51LWxpc3QtdW5mb2N1c2VkLW5vdGlmaWNhdGlvbnMuaHRtbCBbIEZhaWx1cmUgXQog CiB3ZWJraXQub3JnL2IvMTcxNzI2IGltcG9ydGVkL3czYy93ZWItcGxhdGZvcm0tdGVzdHMvbWVk aWEtc291cmNlL21lZGlhc291cmNlLXJlZHVuZGFudC1zZWVrLmh0bWwgWyBGYWlsdXJlIF0K