Bug 40601

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

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

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

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

Created attachment 252642 [details] Patch

Created attachment 252643 [details] Patch

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.

Created attachment 252652 [details] Patch

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?

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?

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

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

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

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

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

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

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

Created attachment 252739 [details] Patch

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

Created attachment 252743 [details] Patch

Comment on attachment 252743 [details] Patch Clearing flags on attachment: 252743 Committed r184015: <http://trac.webkit.org/changeset/184015>

All reviewed patches have been landed. Closing bug.