144788 2015-05-08 00:10:41 -0700 [GTK] WTR doesn't correctly handle the Escape key 2015-05-08 09:17:32 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk, LayoutTestFailure P2 Normal --- 1 cgarcia webkit-unassigned mrobinson oldest_to_newest 1093082 0 cgarcia 2015-05-08 00:10:41 -0700 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. 1093085 1 252694 cgarcia 2015-05-08 00:12:25 -0700 Created attachment 252694 Patch This might fix some layout tests, but test expectations will be updated as part of bug #40601 1093086 2 mrobinson 2015-05-08 00:13:22 -0700 Ideally this should unskip at least one test. 1093087 3 mrobinson 2015-05-08 00:14:27 -0700 Once my original patch lands, I can run layout tests with this patch as well and report back. 1093090 4 cgarcia 2015-05-08 00:18:12 -0700 (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 :-) 1093093 5 cgarcia 2015-05-08 00:27:07 -0700 Expected to fail, but passed: (1) editing/selection/5354455-1.html I'll unskip this one. 1093094 6 252696 cgarcia 2015-05-08 00:28:54 -0700 Created attachment 252696 Patch 1093096 7 mrobinson 2015-05-08 00:29:48 -0700 (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. 1093098 8 mrobinson 2015-05-08 00:33:38 -0700 (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. 1093106 9 cgarcia 2015-05-08 00:54:15 -0700 (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. 1093107 10 cgarcia 2015-05-08 00:54:59 -0700 (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. 1093178 11 mrobinson 2015-05-08 08:05:37 -0700 (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. 1093184 12 252696 mrobinson 2015-05-08 08:17:20 -0700 Comment on attachment 252696 Patch This is actually a great patch though because it fixes the test without a work-around. 1093189 13 cgarcia 2015-05-08 09:07:36 -0700 (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. 1093190 14 cgarcia 2015-05-08 09:11:11 -0700 Committed r183995: <http://trac.webkit.org/changeset/183995> 1093192 15 mrobinson 2015-05-08 09:17:32 -0700 (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. 252694 2015-05-08 00:12:25 -0700 2015-05-08 00:28:54 -0700 Patch wtr-escape.diff text/plain 1517 cgarcia ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCAyNzcz NTNiLi40YjhjZjNjIDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMjAgQEAKIDIwMTUtMDUtMDcgIENhcmxvcyBHYXJjaWEgQ2FtcG9z ICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCisgICAgICAgIFtHVEtdIFdUUiBkb2Vzbid0IGNvcnJl Y3RseSBoYW5kbGUgdGhlIEVzY2FwZSBrZXkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE0NDc4OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIENvcnJlY3RseSBoYW5kbGUgMHgwMDFCIGNoYXJhY3RlciBjb2Rl IGluIEdUSysgZXZlbnQgc2VuZGVyIHRvIGJlCisgICAgICAgIHJlY29nbml6ZWQgYXMgRXNjYXBl IGtleSwgYmVjYXVzZSBnZGtfdW5pY29kZV90b19rZXl2YWwoKSBkb2Vzbid0CisgICAgICAgIGhh bmRsZSBpdC4gVGhpcyBmaXhlcyBsYXlvdXQgdGVzdHMgdXNpbmcga2V5RG93bihTdHJpbmcuZnJv bUNoYXJDb2RlKDB4MDAxQiksIG51bGwpOworICAgICAgICB0byBkaXNtaXNzIHBvcHVwIG1lbnVz LgorCisgICAgICAgICogV2ViS2l0VGVzdFJ1bm5lci9ndGsvRXZlbnRTZW5kZXJQcm94eUd0ay5j cHA6CisgICAgICAgIChXVFI6OmdldEdES0tleVN5bUZvcktleVJlZik6CisKKzIwMTUtMDUtMDcg IENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCiAgICAgICAgIFtH VEtdIEV4cG9zZSBhbGxvd0ZpbGVBY2Nlc3NGcm9tRmlsZVVSTHMgc2V0dGluZyB0byBXZWJLaXQy IEdUSysgQVBJCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xNDQ3NDgKIApkaWZmIC0tZ2l0IGEvVG9vbHMvV2ViS2l0VGVzdFJ1bm5lci9ndGsvRXZlbnRT ZW5kZXJQcm94eUd0ay5jcHAgYi9Ub29scy9XZWJLaXRUZXN0UnVubmVyL2d0ay9FdmVudFNlbmRl clByb3h5R3RrLmNwcAppbmRleCA1MDczODVjLi5kMzliYmQwIDEwMDY0NAotLS0gYS9Ub29scy9X ZWJLaXRUZXN0UnVubmVyL2d0ay9FdmVudFNlbmRlclByb3h5R3RrLmNwcAorKysgYi9Ub29scy9X ZWJLaXRUZXN0UnVubmVyL2d0ay9FdmVudFNlbmRlclByb3h5R3RrLmNwcApAQCAtMjc3LDYgKzI3 Nyw4IEBAIGludCBnZXRHREtLZXlTeW1Gb3JLZXlSZWYoV0tTdHJpbmdSZWYga2V5UmVmLCB1bnNp Z25lZCBsb2NhdGlvbiwgZ3VpbnQqIG1vZGlmaWVyCiAgICAgICAgIHJldHVybiBHREtfS0VZX1Rh YjsKICAgICBpZiAoY2hhckNvZGUgPT0gJ1x4OCcpCiAgICAgICAgIHJldHVybiBHREtfS0VZX0Jh Y2tTcGFjZTsKKyAgICBpZiAoY2hhckNvZGUgPT0gMHgwMDFCKQorICAgICAgICByZXR1cm4gR0RL X0tFWV9Fc2NhcGU7CiAKICAgICBpZiAoV1RGOjppc0FTQ0lJVXBwZXIoY2hhckNvZGUpKQogICAg ICAgICAqbW9kaWZpZXJzIHw9IEdES19TSElGVF9NQVNLOwo= 252696 2015-05-08 00:28:54 -0700 2015-05-08 08:17:20 -0700 Patch wtr-escape.diff text/plain 2779 cgarcia ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBkYWUwNGYzLi4wNjZhNDdmIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTUtMDUt MDggIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCisgICAgICAg IFtHVEtdIFdUUiBkb2Vzbid0IGNvcnJlY3RseSBoYW5kbGUgdGhlIEVzY2FwZSBrZXkKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NDc4OAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVuc2tpcCBlZGl0aW5n L3NlbGVjdGlvbi81MzU0NDU1LTEuaHRtbC4KKworICAgICAgICAqIHBsYXRmb3JtL2d0ay9UZXN0 RXhwZWN0YXRpb25zOgorCiAyMDE1LTA1LTA3ICBDc2FiYSBPc3p0cm9nb27DoWMgIDxvc3N5QHdl YmtpdC5vcmc+CiAKICAgICAgICAgW0VGTF0gVW5yZXZpZXdlZCBnYXJkZW5pbmcuCmRpZmYgLS1n aXQgYS9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucyBiL0xheW91dFRl c3RzL3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zCmluZGV4IGVjNGY1ZGQuLmZkYTcxZDcg MTAwNjQ0Ci0tLSBhL0xheW91dFRlc3RzL3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zCisr KyBiL0xheW91dFRlc3RzL3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zCkBAIC0xNDU3LDcg KzE0NTcsNiBAQCB3ZWJraXQub3JnL2IvNjM3MDYgZWRpdGluZy9wYXN0ZWJvYXJkL2NsZWFudXAt b24tbW92ZS5odG1sIFsgRmFpbHVyZSBdCiB3ZWJraXQub3JnL2IvNjM3MDYgZWRpdGluZy9wYXN0 ZWJvYXJkL2RyYWctbGlzdC1pdGVtLmh0bWwgWyBGYWlsdXJlIF0KIAogIyBXZWJLaXQyIG5lZWRz IHRlc3RSdW5uZXIuc2V0RWRpdGluZ0JlaGF2aW91ciBhbmQgb3RoZXIgaXNzdWVzLgotd2Via2l0 Lm9yZy9iLzQwNjAxIGVkaXRpbmcvc2VsZWN0aW9uLzUzNTQ0NTUtMS5odG1sIFsgRmFpbHVyZSBd CiBCdWcoR1RLKSBlZGl0aW5nL3NlbGVjdGlvbi9leHRlbmQtYWZ0ZXItbW91c2Utc2VsZWN0aW9u Lmh0bWwgWyBGYWlsdXJlIF0KIAogIyBXZWJLaXQyIG5lZWRzIHRleHRJbnB1dENvbnRyb2xsZXIu c2VsZWN0ZWRSYW5nZQpkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9vbHMvQ2hhbmdl TG9nCmluZGV4IDI3NzM1M2IuLjU1NDM0OGIgMTAwNjQ0Ci0tLSBhL1Rvb2xzL0NoYW5nZUxvZwor KysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsNSArMSwyMiBAQAogMjAxNS0wNS0wNyAgQ2FybG9z IEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKKyAgICAgICAgW0dUS10gV1RS IGRvZXNuJ3QgY29ycmVjdGx5IGhhbmRsZSB0aGUgRXNjYXBlIGtleQorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ0Nzg4CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ29ycmVjdGx5IGhhbmRsZSAweDAwMUIg Y2hhcmFjdGVyIGNvZGUgaW4gR1RLKyBldmVudCBzZW5kZXIgdG8gYmUKKyAgICAgICAgcmVjb2du aXplZCBhcyBFc2NhcGUga2V5LCBiZWNhdXNlIGdka191bmljb2RlX3RvX2tleXZhbCgpIGRvZXNu J3QKKyAgICAgICAgaGFuZGxlIGl0LiBUaGlzIGZpeGVzIGxheW91dCB0ZXN0cyB1c2luZyBrZXlE b3duKFN0cmluZy5mcm9tQ2hhckNvZGUoMHgwMDFCKSwgbnVsbCk7CisgICAgICAgIHRvIGRpc21p c3MgcG9wdXAgbWVudXMuCisKKyAgICAgICAgRml4ZXMgZWRpdGluZy9zZWxlY3Rpb24vNTM1NDQ1 NS0xLmh0bWwuCisKKyAgICAgICAgKiBXZWJLaXRUZXN0UnVubmVyL2d0ay9FdmVudFNlbmRlclBy b3h5R3RrLmNwcDoKKyAgICAgICAgKFdUUjo6Z2V0R0RLS2V5U3ltRm9yS2V5UmVmKToKKworMjAx NS0wNS0wNyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKICAg ICAgICAgW0dUS10gRXhwb3NlIGFsbG93RmlsZUFjY2Vzc0Zyb21GaWxlVVJMcyBzZXR0aW5nIHRv IFdlYktpdDIgR1RLKyBBUEkKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE0NDc0OAogCmRpZmYgLS1naXQgYS9Ub29scy9XZWJLaXRUZXN0UnVubmVyL2d0 ay9FdmVudFNlbmRlclByb3h5R3RrLmNwcCBiL1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvZ3RrL0V2 ZW50U2VuZGVyUHJveHlHdGsuY3BwCmluZGV4IDUwNzM4NWMuLmQzOWJiZDAgMTAwNjQ0Ci0tLSBh L1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvZ3RrL0V2ZW50U2VuZGVyUHJveHlHdGsuY3BwCisrKyBi L1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvZ3RrL0V2ZW50U2VuZGVyUHJveHlHdGsuY3BwCkBAIC0y NzcsNiArMjc3LDggQEAgaW50IGdldEdES0tleVN5bUZvcktleVJlZihXS1N0cmluZ1JlZiBrZXlS ZWYsIHVuc2lnbmVkIGxvY2F0aW9uLCBndWludCogbW9kaWZpZXIKICAgICAgICAgcmV0dXJuIEdE S19LRVlfVGFiOwogICAgIGlmIChjaGFyQ29kZSA9PSAnXHg4JykKICAgICAgICAgcmV0dXJuIEdE S19LRVlfQmFja1NwYWNlOworICAgIGlmIChjaGFyQ29kZSA9PSAweDAwMUIpCisgICAgICAgIHJl dHVybiBHREtfS0VZX0VzY2FwZTsKIAogICAgIGlmIChXVEY6OmlzQVNDSUlVcHBlcihjaGFyQ29k ZSkpCiAgICAgICAgICptb2RpZmllcnMgfD0gR0RLX1NISUZUX01BU0s7Cg==