Bug 40601 – [GTK] Some tests fail because they do not assume the popup menu captures click events

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Currently these tests are passing, but should not be. When 40600 they should start failing, as expected.

[Martin Robinson]

To clarify, when 40600 is resolved, these tests will start failing.

[Dimitri Glazkov (Google)]

I have one more to add: http://trac.webkit.org/browser/trunk/LayoutTests/media/controls-right-click-on-timebar.html

[Zan Dobersek]

The following tests were found to be passing consistently after moving from using the Skipped list to using test_expectations.txt:
	editing/selection/5354455-2.html

Their expectations were removed in r116122[1] (covered by bug #85591). Tests that are covered by this bug and are still failing are the following:
	fast/events/mouse-click-events.html
	fast/events/mouseup-from-button2.html
	fast/events/right-click-focus.html
	editing/selection/5354455-1.html
	media/controls-right-click-on-timebar.html

1: http://trac.webkit.org/changeset/116122

[Marcos Chavarría Teijeiro (irc: chavaone)]

The editing/selection/5354455-1.html layout test is flaky right now (it passes sometimes).

[Martin Robinson]

Created attachment 252642 [details]
Patch

[Martin Robinson]

Created attachment 252643 [details]
Patch

[Martin Robinson]

The current strategy, which seems to work well is to manually pop all menus down between tests and before dispatching every event. It would be better to prevent the menus from popping up at all, but this can work as an interim solution.

[Martin Robinson]

Created attachment 252652 [details]
Patch

[Carlos Garcia Campos]

I'm confused with the bug title, so the problem is that we keep popup menus opened between tests? or that they should be open and they should capture click events, but they don't?

[Carlos Garcia Campos]

Created attachment 252692 [details]
Correctly handle Escape

I've noticed that some tests explicitly dismiss context menus by sending the Escape key, with something like this:

eventSender.keyDown(String.fromCharCode(0x001B), null);

see editing/selection/5354455-1.html, for example. We are not correctly handling 0x001B in GTK+ as Escape key, and that's why the context menu remains visible. Is this the problem os this bug? or is it a different issue?

Maybe we could send the Escape key in EventSendingController::contextClick() and then tests won't have to worry about that. 

Could you check if this patch fixes anything?

[Martin Robinson]

(In reply to comment #9)
> I'm confused with the bug title, so the problem is that we keep popup menus
> opened between tests? or that they should be open and they should capture
> click events, but they don't?

I think that popups should never capture click events sent from the EventSender. The bug title refers to the originally detected problem, that events were sent to open popup menus during tests instead of the web view. This is part of a larger class of problems though, where open popup menus maintain grabs, between events sent from the EventSender and also between tests. The patch addresses all these issues, because they are all related to open popups.

[Martin Robinson]

(In reply to comment #10)
> Created attachment 252692 [details]
> Correctly handle Escape
> 
> I've noticed that some tests explicitly dismiss context menus by sending the
> Escape key, with something like this:
> 
> eventSender.keyDown(String.fromCharCode(0x001B), null);
> 
> see editing/selection/5354455-1.html, for example. We are not correctly
> handling 0x001B in GTK+ as Escape key, and that's why the context menu
> remains visible. Is this the problem os this bug? or is it a different issue?
> 
> Maybe we could send the Escape key in EventSendingController::contextClick()
> and then tests won't have to worry about that. 
> 
> Could you check if this patch fixes anything?

I can check tomorrow, though I think we should integrate both patches. I don't believe that all tests dismiss the context menu and some of them definitely don't,

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #10)
> > Created attachment 252692 [details]
> > Correctly handle Escape
> > 
> > I've noticed that some tests explicitly dismiss context menus by sending the
> > Escape key, with something like this:
> > 
> > eventSender.keyDown(String.fromCharCode(0x001B), null);
> > 
> > see editing/selection/5354455-1.html, for example. We are not correctly
> > handling 0x001B in GTK+ as Escape key, and that's why the context menu
> > remains visible. Is this the problem os this bug? or is it a different issue?
> > 
> > Maybe we could send the Escape key in EventSendingController::contextClick()
> > and then tests won't have to worry about that. 
> > 
> > Could you check if this patch fixes anything?
> 
> I can check tomorrow, though I think we should integrate both patches. I
> don't believe that all tests dismiss the context menu and some of them
> definitely don't,

I see, so maybe your patch could be simplified by sending the Escape key in EventSendingController::contextClick()? I'll file a different bug for my patch then. Thanks!

[Martin Robinson]

(In reply to comment #13)

> I see, so maybe your patch could be simplified by sending the Escape key in
> EventSendingController::contextClick()? I'll file a different bug for my
> patch then. Thanks!

If we could guarantee that the escape key went immediately to the menu and not to the WebView this could work well.

Another thing that about my patch is that it should also work for stray list boxes.

[Martin Robinson]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > I see, so maybe your patch could be simplified by sending the Escape key in
> > EventSendingController::contextClick()? I'll file a different bug for my
> > patch then. Thanks!
> 
> If we could guarantee that the escape key went immediately to the menu and
> not to the WebView this could work well.
> 
> Another thing that about my patch is that it should also work for stray list
> boxes.

Sorry, I think I meant to say that sending escape could work in addition to my original patch. I think it would be hard to detect non-context-clicks that pop up list boxes or other controls that take a grab.

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > I see, so maybe your patch could be simplified by sending the Escape key in
> > EventSendingController::contextClick()? I'll file a different bug for my
> > patch then. Thanks!
> 
> If we could guarantee that the escape key went immediately to the menu and
> not to the WebView this could work well.

Yes, because of the grab.

> Another thing that about my patch is that it should also work for stray list
> boxes.

I think popup menus are a bit different, because we run a nested main loop for them IIRC, so I assume those are correctly dismissed.

[Martin Robinson]

Comment on attachment 252652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252652&action=review

> LayoutTests/platform/gtk/TestExpectations:1697
> -webkit.org/b/40601 fast/events/mouse-click-events.html [ Failure ]
> -webkit.org/b/40601 fast/events/mouseup-from-button2.html [ Failure ]
> -webkit.org/b/40601 fast/events/right-click-focus.html [ Failure ]
> +#webkit.org/b/40601 fast/events/mouse-click-events.html [ Failure ]
> +#webkit.org/b/40601 fast/events/mouseup-from-button2.html [ Failure ]
> +#webkit.org/b/40601 fast/events/right-click-focus.html [ Failure ]

I just noticed that I left these commented out instead of removing the lines. I can fix this in the next revision or when I land the patch.

[Carlos Garcia Campos]

Comment on attachment 252652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252652&action=review

Ok, I think I understand the problem now, and I agree there's no simpler way to work aound the issue.

> Tools/WebKitTestRunner/PlatformWebView.h:89
> +    void hideAllPopupMenus();

Please consider call this dismissAllPopupMenus instead.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:171
> +    ASSERT(webView);
> +    webView->hideAllPopupMenus();
> +
>      gtk_main_do_event(event);
>      gdk_event_free(event);

Now I see how this indeed conflicts with my patch to handle the Escape key. Now tests sending Escape to dismiss the context menu will end up also sending the Escape to the view. It's probably hamrless, but maybe we could handle that case here. If we make hideAllPopupMenus() boolean, returning true if any menu has been dismissed, we could do something like:

if (webView->dimissAllPopupMenus() && event->type == GDK_KEY_PRESS && event->key.keyval == GDK_KEY_Escape) {
    // Escape key sent to dismiss the popup menu, don't propagate it to the view.
    gdk_event_free(event);
    return;
}

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:181
> -        dispatchEvent(item.event);
> +        dispatchEvent(m_testController->mainWebView(), item.event);

Could we make dispatchEvent a private member and then we don't need to pass the web view on every call?

> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:149
> +        if (data)

Can this really be NULL?

[Martin Robinson]

(In reply to comment #18)

> > Tools/WebKitTestRunner/PlatformWebView.h:89
> > +    void hideAllPopupMenus();
> 
> Please consider call this dismissAllPopupMenus instead.

Sure. Your suggestion is a bit better.
  
> > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:171
> > +    ASSERT(webView);
> > +    webView->hideAllPopupMenus();
> > +
> >      gtk_main_do_event(event);
> >      gdk_event_free(event);
> 
> Now I see how this indeed conflicts with my patch to handle the Escape key.
> Now tests sending Escape to dismiss the context menu will end up also
> sending the Escape to the view. It's probably hamrless, but maybe we could
> handle that case here. If we make hideAllPopupMenus() boolean, returning
> true if any menu has been dismissed, we could do something like:
> 
> if (webView->dimissAllPopupMenus() && event->type == GDK_KEY_PRESS &&
> event->key.keyval == GDK_KEY_Escape) {
>     // Escape key sent to dismiss the popup menu, don't propagate it to the
> view.
>     gdk_event_free(event);
>     return;
> }

Nice catch. I think your suggestion makes sense, but if we simply reverse the logic, it can be a bit simpler. This is what I'm suggesting:
  
    // If we are sending an escape key to the WebView, this has the side-effect of dismissing
    // any current popups anyway. Chances are that the test is doing this to dismiss the popup
    // anyway. Not all tests properly dismiss popup menus, so we still need to do it manually
    // if this isn't an escape key press.
    if (event->type != GDK_KEY_PRESS || event->key.keyval != GDK_KEY_Escape)
        m_testController->mainWebView()->dismissAllPopupMenus();

I've confirmed that tests still pass with this change.

> > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:181
> > -        dispatchEvent(item.event);
> > +        dispatchEvent(m_testController->mainWebView(), item.event);
> 
> Could we make dispatchEvent a private member and then we don't need to pass
> the web view on every call?
> 
> > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:149
> > +        if (data)
> 
> Can this really be NULL?

Probably not! I'll change it to an assertion.

[Martin Robinson]

Created attachment 252739 [details]
Patch

[Martin Robinson]

(In reply to comment #20)
> Created attachment 252739 [details]
> Patch

In the latest version of the crash, I no longer attach non-context-menu popup menus to the widget, as they seems to cause some assertion failures inside GTK+ and doesn't actually fix any tests. We should consider this later though, if we run into any more issues.

[Martin Robinson]

Created attachment 252743 [details]
Patch

[Martin Robinson]

Comment on attachment 252743 [details]
Patch

Clearing flags on attachment: 252743

Committed r184015: <http://trac.webkit.org/changeset/184015>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.