56809 2011-03-22 01:44:57 -0700 Expose Node::isFocusable() in the Chromium WebKit API 2011-03-24 13:06:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 isherman isherman abarth commit-queue dglazkov eric fishd isherman jamesr webkit.review.bot oldest_to_newest 371278 0 isherman 2011-03-22 01:44:57 -0700 Expose Node::isFocusable() in the Chromium WebKit API 371279 1 86439 isherman 2011-03-22 01:45:30 -0700 Created attachment 86439 Patch 371424 2 dglazkov 2011-03-22 09:36:58 -0700 Can you explain how you're going to use this? I am not sure isFocusable is what you really need here. 371705 3 isherman 2011-03-22 15:17:44 -0700 (In reply to comment #2) > Can you explain how you're going to use this? I am not sure isFocusable is what you really need here. We want to be able to identify input elements that should not be autofilled. In particular, we are currently failing to identify elements with style display:none or visibility:hidden. I thought at first that Node::hasNonEmptyBoundingBox() would the right method, but it does not identify elements with visibility:hidden. My next instinct was to add an "isVisible()" method, but Darin recommended against that when the aforementioned method was originally added: https://bugs.webkit.org/show_bug.cgi?id=37625 Do you know if there's a more appropriate method/approach to use here? 371715 4 jamesr 2011-03-22 15:28:32 -0700 Can you define formally what "should not be autofilled" means? 371723 5 isherman 2011-03-22 15:31:53 -0700 (In reply to comment #4) > Can you define formally what "should not be autofilled" means? The relevant sub-clause is: A field should not be autofilled if it is not currently displayed, i.e. if it is not currently user-visible. The two common cases for this are fields styled with display:none and visibility:hidden 371729 6 jamesr 2011-03-22 15:37:08 -0700 Hmm. That's not what isFocusable() means. My question is which side of the API is supposed to be figuring out if a node is a candidate for autofilling - is that supposed to happen on the WebKit side, or on the chrome side using data exposed through the API? 371755 7 isherman 2011-03-22 15:56:48 -0700 (In reply to comment #6) > Hmm. That's not what isFocusable() means. That's true. I would have thought that's what hasNonEmptyBoundingBox() means, but apparently not :( I don't think there is a method that means exactly what I want; but isFocusable() happens to be implemented exactly how I want. I think it's also fully reasonable to never autofill an element that is not focusable... I guess the phrasing I would use to motivate that is: A field should not be autofilled if the user cannot manually fill it. > My question is which side of the API is supposed to be figuring out if a node is a candidate for autofilling - is that supposed to happen on the WebKit side, or on the chrome side using data exposed through the API? Since Chrome is really the only client, the going theory has been that it should happen on the Chrome side. We've been gradually moving more and more of the autofill logic out of WebCore; and wherever possible, out of the WebKit API as well. In principle some of this logic could be shared with Safari, but I have no insight into that codebase, and it seems like a bad idea to make blind guesses. 371757 8 jamesr 2011-03-22 16:01:32 -0700 So here's some thought for food: according to the accessibility folks, content in the fallback section of a <canvas> should be focusable even though the user can't "see" the content. See bug 50126 for instance. What should the desired behavior be in this case? I'm not sure what bits of information you really need in the autofill code so it's hard to decide what the best level of API is - can you point me to the code that uses the WebKit API to decide if a node is an autofill candidate? 372012 9 isherman 2011-03-23 00:51:43 -0700 (In reply to comment #8) > So here's some thought for food: according to the accessibility folks, content in the fallback section of a <canvas> should be focusable even though the user can't "see" the content. See bug 50126 for instance. What should the desired behavior be in this case? I don't really understand this example in bug 50126. Is the expectation that if I tab through the enclosing form, that focus would eventually arrive at the fallback <input> element and I could type into it? Would the canvas then update to display what I am typing? If that's the use-case, then it probably does make sense to autofill that element. On the other hand, if the element is focusable but the user cannot edit its value, then we probably shouldn't autofill it. > I'm not sure what bits of information you really need in the autofill code so it's hard to decide what the best level of API is - can you point me to the code that uses the WebKit API to decide if a node is an autofill candidate? Roughly, that code lives here: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/renderer/autofill/form_manager.cc&q=form_manager.cc&exact_package=chromium&l=847 In particular, the rough notion of "the element is visible" would probably be added to this if-stmt: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/renderer/autofill/form_manager.cc&q=form_manager.cc&exact_package=chromium&l=862 372565 10 86439 jamesr 2011-03-23 17:40:32 -0700 Comment on attachment 86439 Patch We talked in person and I think this looks OK. My main concern is that this function assumes layout is up to date, but it doesn't force update if layout is not. Some WebKit APIs force layout if it's out of date, some return incorrect results if layout is out of date, and some are incorrect if layout is up to date but do not make any effort to ensure that it is. I wish we were a little more consistent here. That being said, I'm inclined to R+ - Dimitri or Darin, do you think this looks all right? 372849 11 86439 dglazkov 2011-03-24 09:36:35 -0700 Comment on attachment 86439 Patch ok. 372867 12 86439 commit-queue 2011-03-24 10:03:48 -0700 Comment on attachment 86439 Patch Clearing flags on attachment: 86439 Committed r81873: <http://trac.webkit.org/changeset/81873> 372869 13 commit-queue 2011-03-24 10:03:54 -0700 All reviewed patches have been landed. Closing bug. 373018 14 webkit.review.bot 2011-03-24 13:06:46 -0700 http://trac.webkit.org/changeset/81873 might have broken GTK Linux 32-bit Debug The following tests are not passing: svg/W3C-SVG-1.1/animate-elem-46-t.svg svg/W3C-SVG-1.1/animate-elem-82-t.svg 86439 2011-03-22 01:45:30 -0700 2011-03-24 10:03:48 -0700 Patch bug-56809-20110322014529.patch text/plain 2024 isherman U3VidmVyc2lvbiBSZXZpc2lvbjogODE2MTIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKaW5kZXgg YmM0NGFiODQ2MDQ4ODYwNDkxODJlNDE3M2UzNjBjOGM1ZWQzZWM3YS4uYmEyMmJjZjZiNzQxNTIx MWJmNTRiMzY4YWQxMmZhZGI0ODFiNGIxZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJv bWl1bS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKQEAg LTEsMyArMSwxNSBAQAorMjAxMS0wMy0yMiAgSWx5YSBTaGVybWFuICA8aXNoZXJtYW5AY2hyb21p dW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEV4cG9zZSBOb2RlOjppc0ZvY3VzYWJsZSgpIGluIHRoZSBDaHJvbWl1bSBXZWJLaXQgQVBJCisg ICAgICAgIEluIHNlcnZpY2Ugb2YgaHR0cHM6Ly9jb2RlLmdvb2dsZS5jb20vcC9jaHJvbWl1bS9p c3N1ZXMvZGV0YWlsP2lkPTcyOTE4CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD01NjgwOQorCisgICAgICAgICogcHVibGljL1dlYk5vZGUuaDoKKyAgICAg ICAgKiBzcmMvV2ViTm9kZS5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYk5vZGU6OmlzRm9jdXNh YmxlKToKKwogMjAxMS0wMy0yMSAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFp bC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjgxMzc3LgpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJsaWMvV2ViTm9kZS5oIGIvU291cmNlL1dl YktpdC9jaHJvbWl1bS9wdWJsaWMvV2ViTm9kZS5oCmluZGV4IDcxMTZkZmFmZjA2OWMwZTJmNGZj ZmRhYTc2MDQxMWFlNzU1OThmNWUuLjkyYjE3ZDQ2OTJjYzliOGFiZjk4OTc4OTllNTA0ZGY0OTAw MjY1N2MgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYk5vZGUu aAorKysgYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJOb2RlLmgKQEAgLTk2LDYg Kzk2LDcgQEAgcHVibGljOgogICAgIFdFQktJVF9BUEkgV2ViTm9kZUxpc3QgY2hpbGROb2Rlcygp OwogICAgIFdFQktJVF9BUEkgV2ViU3RyaW5nIGNyZWF0ZU1hcmt1cCgpIGNvbnN0OwogICAgIFdF QktJVF9BUEkgYm9vbCBpc1RleHROb2RlKCkgY29uc3Q7CisgICAgV0VCS0lUX0FQSSBib29sIGlz Rm9jdXNhYmxlKCkgY29uc3Q7CiAgICAgV0VCS0lUX0FQSSBib29sIGlzQ29udGVudEVkaXRhYmxl KCkgY29uc3Q7CiAgICAgV0VCS0lUX0FQSSBib29sIGlzRWxlbWVudE5vZGUoKSBjb25zdDsKICAg ICBXRUJLSVRfQVBJIHZvaWQgYWRkRXZlbnRMaXN0ZW5lcihjb25zdCBXZWJTdHJpbmcmIGV2ZW50 VHlwZSwgV2ViRE9NRXZlbnRMaXN0ZW5lciogbGlzdGVuZXIsIGJvb2wgdXNlQ2FwdHVyZSk7CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJOb2RlLmNwcCBiL1NvdXJj ZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYk5vZGUuY3BwCmluZGV4IGU5MWQxZWVkNDY2YzFmYzYy MDdhMGRlN2VjZDJiNDNkMjE1ZGYwZDMuLmNmYjg1MjhjODA2MjhiNGRjNTg0MDI3YmIwMTNjNzMz ZmQ0OGQ5MzQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYk5vZGUu Y3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYk5vZGUuY3BwCkBAIC0xNDUs NiArMTQ1LDExIEBAIGJvb2wgV2ViTm9kZTo6aXNUZXh0Tm9kZSgpIGNvbnN0CiAgICAgcmV0dXJu IG1fcHJpdmF0ZS0+aXNUZXh0Tm9kZSgpOwogfQogCitib29sIFdlYk5vZGU6OmlzRm9jdXNhYmxl KCkgY29uc3QKK3sKKyAgICByZXR1cm4gbV9wcml2YXRlLT5pc0ZvY3VzYWJsZSgpOworfQorCiBi b29sIFdlYk5vZGU6OmlzQ29udGVudEVkaXRhYmxlKCkgY29uc3QKIHsKICAgICByZXR1cm4gbV9w cml2YXRlLT5pc0NvbnRlbnRFZGl0YWJsZSgpOwo=