Bug 144788 – [GTK] WTR doesn't correctly handle the Escape key

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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, because gdk_unicode_to_keyval() doesn't handle it.

[Carlos Garcia Campos]

Created attachment 252694 [details]
Patch

This might fix some layout tests, but test expectations will be updated as part of bug #40601

[Martin Robinson]

Ideally this should unskip at least one test.

[Martin Robinson]

Once my original patch lands, I can run layout tests with this patch as well and report back.

[Carlos Garcia Campos]

(In reply to comment #3)
> Once my original patch lands, I can run layout tests with this patch as well
> and report back.

Ideally, this patch blocks #40601, because with this, your patch might be reduced to a one line :-)

[Carlos Garcia Campos]

Expected to fail, but passed: (1)
  editing/selection/5354455-1.html

I'll unskip this one.

[Carlos Garcia Campos]

Created attachment 252696 [details]
Patch

[Martin Robinson]

(In reply to comment #4)
> (In reply to comment #3)
> > Once my original patch lands, I can run layout tests with this patch as well
> > and report back.
> 
> Ideally, this patch blocks #40601, because with this, your patch might be
> reduced to a one line :-)

I spent quite a few tedious hours running the tests and generating results, so I'd much prefer to just land my original patch and then go back and improve it later. It will be simpler for me and I won't have to worry about going down a rat hole. I promise that I will try out your escape key approach, which looks promising and propose another patch if it works.

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Once my original patch lands, I can run layout tests with this patch as well
> > > and report back.
> > 
> > Ideally, this patch blocks #40601, because with this, your patch might be
> > reduced to a one line :-)
> 
> I spent quite a few tedious hours running the tests and generating results,
> so I'd much prefer to just land my original patch and then go back and
> improve it later. It will be simpler for me and I won't have to worry about
> going down a rat hole. I promise that I will try out your escape key
> approach, which looks promising and propose another patch if it works.

For an example of an uncertainty with the escape key approach: if we send the key between tests, will it pollute the results of tests before and after it? I'm not sure, but I do know that the gtk_menu_popdown approach works without a hitch.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Once my original patch lands, I can run layout tests with this patch as well
> > > and report back.
> > 
> > Ideally, this patch blocks #40601, because with this, your patch might be
> > reduced to a one line :-)
> 
> I spent quite a few tedious hours running the tests and generating results,
> so I'd much prefer to just land my original patch and then go back and
> improve it later. It will be simpler for me and I won't have to worry about
> going down a rat hole. I promise that I will try out your escape key
> approach, which looks promising and propose another patch if it works.

I don't like that patch that much, TBH. I won't oppose it if there isn't a better alternative, though.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > Once my original patch lands, I can run layout tests with this patch as well
> > > > and report back.
> > > 
> > > Ideally, this patch blocks #40601, because with this, your patch might be
> > > reduced to a one line :-)
> > 
> > I spent quite a few tedious hours running the tests and generating results,
> > so I'd much prefer to just land my original patch and then go back and
> > improve it later. It will be simpler for me and I won't have to worry about
> > going down a rat hole. I promise that I will try out your escape key
> > approach, which looks promising and propose another patch if it works.
> 
> For an example of an uncertainty with the escape key approach: if we send
> the key between tests, will it pollute the results of tests before and after
> it? I'm not sure, but I do know that the gtk_menu_popdown approach works
> without a hitch.

I don't send Escape between tests, but after running the context menu.

[Martin Robinson]

(In reply to comment #10)

> > For an example of an uncertainty with the escape key approach: if we send
> > the key between tests, will it pollute the results of tests before and after
> > it? I'm not sure, but I do know that the gtk_menu_popdown approach works
> > without a hitch.
> 
> I don't send Escape between tests, but after running the context menu.

Let me explain then: At times the context menus remains popped up between tests, as well as after context click. This is a problem because, as you know, context menus maintain grabs which block events to the web view. These are two issues between tests and between different calls to send an event from the test harness. I think a good fix would be to never actually pop up context menus in the first place, but that could be more involved. The reason we need to handle this between tests is that not all tests dismiss their context menus, and context menus don't disappear between page loads (you can verify this by test it in minibrowser).

There are many tests that do not dismiss the context menu explicitly. For instance look at editing/spelling/context-menu-suggestions.html.

Your escape key approach looks interesting, but really the escape key is just going to call gtk_menu_popdown, so I prefer to just do that directly. The reason is that the test harness is basically a machine that eats key events and poops out test expectations. I feel nervous sending it another key event, after again spending hours, running layout tests and looking at results. 

> I don't like that patch that much, TBH. I won't oppose it if there isn't a better alternative, though.

Would you please go to the patch and articulate your concerns? Just blocking it without reason or because you have an untested idea is incredibly frustrating. I'd like to move on from this.

[Martin Robinson]

Comment on attachment 252696 [details]
Patch

This is actually a great patch though because it fixes the test without a work-around.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > > For an example of an uncertainty with the escape key approach: if we send
> > > the key between tests, will it pollute the results of tests before and after
> > > it? I'm not sure, but I do know that the gtk_menu_popdown approach works
> > > without a hitch.
> > 
> > I don't send Escape between tests, but after running the context menu.
> 
> Let me explain then: At times the context menus remains popped up between
> tests, as well as after context click. This is a problem because, as you
> know, context menus maintain grabs which block events to the web view. These
> are two issues between tests and between different calls to send an event
> from the test harness. I think a good fix would be to never actually pop up
> context menus in the first place, but that could be more involved.

That was also my first thought, but the fact that some test explicitly dismiss context menus made thought that the menu was expected to be popped up.

> The
> reason we need to handle this between tests is that not all tests dismiss
> their context menus, and context menus don't disappear between page loads
> (you can verify this by test it in minibrowser).

That sounds to me like a bug in the web view/page proxy, not in WTR.

> There are many tests that do not dismiss the context menu explicitly. For
> instance look at editing/spelling/context-menu-suggestions.html.
> 
> Your escape key approach looks interesting, but really the escape key is
> just going to call gtk_menu_popdown, so I prefer to just do that directly.
> The reason is that the test harness is basically a machine that eats key
> events and poops out test expectations. I feel nervous sending it another
> key event, after again spending hours, running layout tests and looking at
> results. 

My main concern with your approach is that it's very GTK+ specific, what makes me wonder if the problem is not in WTR but in the web view, and by fixing it in WTR we are just hiding the actual bug even more. Why do other ports don't need this?

> > I don't like that patch that much, TBH. I won't oppose it if there isn't a better alternative, though.
> 
> Would you please go to the patch and articulate your concerns? Just blocking
> it without reason or because you have an untested idea is incredibly
> frustrating. I'd like to move on from this.

I'm not blocking anything, just proposing a possible simpler alternative, but you already confirmed it's not enough and it's a related but different issue, and that's why I opened this bug. I just need to understand the problem and make sure this is not a work around of an actual bug before reviewing the patch. I understand your frustration, though.

[Carlos Garcia Campos]

Committed r183995: <http://trac.webkit.org/changeset/183995>

[Martin Robinson]

(In reply to comment #13)

> > The
> > reason we need to handle this between tests is that not all tests dismiss
> > their context menus, and context menus don't disappear between page loads
> > (you can verify this by test it in minibrowser).
> 
> That sounds to me like a bug in the web view/page proxy, not in WTR.

I don't think it's a bug. I confirmed that Safari and Chromium worked this way as well. You can confirm yourself by going to a page and running "setTimeout(function() { window.history.back(); }, 1500);" and then open up the context menu before the page goes back.

> 
> > There are many tests that do not dismiss the context menu explicitly. For
> > instance look at editing/spelling/context-menu-suggestions.html.
> > 
> > Your escape key approach looks interesting, but really the escape key is
> > just going to call gtk_menu_popdown, so I prefer to just do that directly.
> > The reason is that the test harness is basically a machine that eats key
> > events and poops out test expectations. I feel nervous sending it another
> > key event, after again spending hours, running layout tests and looking at
> > results. 
> 
> My main concern with your approach is that it's very GTK+ specific, what
> makes me wonder if the problem is not in WTR but in the web view, and by
> fixing it in WTR we are just hiding the actual bug even more. Why do other
> ports don't need this?

I believe what is going on here is that the Mac EventSender send its events directly to the widget, while we use the main GTK+ event queue. I think in the long term we should try for an approach more like that and remove the work-arounds. It's unclear if we can do that without abusing the GTK+ API though.

> I'm not blocking anything, just proposing a possible simpler alternative,
> but you already confirmed it's not enough and it's a related but different
> issue, and that's why I opened this bug. I just need to understand the
> problem and make sure this is not a work around of an actual bug before
> reviewing the patch. I understand your frustration, though.

Thanks I appreciate you taking the time to look at it.