Bug 40601 - [GTK] Some tests fail because they do not assume the popup menu captures click events
Summary: [GTK] Some tests fail because they do not assume the popup menu captures clic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk, LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2010-06-14 17:17 PDT by Martin Robinson
Modified: 2017-05-30 05:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.94 KB, patch)
2015-05-07 15:59 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (54.05 KB, patch)
2015-05-07 16:08 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (54.60 KB, patch)
2015-05-07 16:57 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Correctly handle Escape (1.52 KB, patch)
2015-05-07 23:54 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (52.68 KB, patch)
2015-05-08 13:18 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (52.68 KB, patch)
2015-05-08 13:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-06-14 17:17:31 PDT
Currently these tests are passing, but should not be. When 40600 they should start failing, as expected.
Comment 1 Martin Robinson 2010-06-14 17:18:03 PDT
To clarify, when 40600 is resolved, these tests will start failing.
Comment 2 Dimitri Glazkov (Google) 2010-11-12 13:39:10 PST
I have one more to add: http://trac.webkit.org/browser/trunk/LayoutTests/media/controls-right-click-on-timebar.html
Comment 3 Zan Dobersek 2012-05-05 09:44:10 PDT
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
Comment 4 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-06 05:05:48 PDT
The editing/selection/5354455-1.html layout test is flaky right now (it passes sometimes).
Comment 5 Martin Robinson 2015-05-07 15:59:08 PDT
Created attachment 252642 [details]
Patch
Comment 6 Martin Robinson 2015-05-07 16:08:30 PDT
Created attachment 252643 [details]
Patch
Comment 7 Martin Robinson 2015-05-07 16:14:24 PDT
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.
Comment 8 Martin Robinson 2015-05-07 16:57:10 PDT
Created attachment 252652 [details]
Patch
Comment 9 Carlos Garcia Campos 2015-05-07 23:47:36 PDT
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?
Comment 10 Carlos Garcia Campos 2015-05-07 23:54:02 PDT
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?
Comment 11 Martin Robinson 2015-05-07 23:57:03 PDT
(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.
Comment 12 Martin Robinson 2015-05-07 23:58:23 PDT
(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,
Comment 13 Carlos Garcia Campos 2015-05-08 00:07:52 PDT
(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!
Comment 14 Martin Robinson 2015-05-08 00:10:52 PDT
(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.
Comment 15 Martin Robinson 2015-05-08 00:12:15 PDT
(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.
Comment 16 Carlos Garcia Campos 2015-05-08 00:16:06 PDT
(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.
Comment 17 Martin Robinson 2015-05-08 00:16:28 PDT
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.
Comment 18 Carlos Garcia Campos 2015-05-08 10:46:49 PDT
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?
Comment 19 Martin Robinson 2015-05-08 11:59:43 PDT
(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.
Comment 20 Martin Robinson 2015-05-08 13:18:04 PDT
Created attachment 252739 [details]
Patch
Comment 21 Martin Robinson 2015-05-08 13:25:49 PDT
(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.
Comment 22 Martin Robinson 2015-05-08 13:26:57 PDT
Created attachment 252743 [details]
Patch
Comment 23 Martin Robinson 2015-05-08 14:22:54 PDT
Comment on attachment 252743 [details]
Patch

Clearing flags on attachment: 252743

Committed r184015: <http://trac.webkit.org/changeset/184015>
Comment 24 Martin Robinson 2015-05-08 14:23:00 PDT
All reviewed patches have been landed.  Closing bug.