Bug 171492 – REGRESSION(r215188?): Test platform/gtk/fast/forms/menulist-typeahead-find.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

r215187                       NOERROR
[r215188-r215192]             UNKNOWN
r215193                       TEXT (Expected: PASS)

[Carlos Garcia Campos]

Expectations updated in r216011

[Adrian Perez]

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.

[Adrian Perez]

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.

[Adrian Perez]

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

[Adrian Perez]

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.

[Carlos Garcia Campos]

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

[Adrian Perez]

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

[Adrian Perez]

Created attachment 313077 [details]
Patch

[Adrian Perez]

Committed r218391: <http://trac.webkit.org/changeset/218391>