Bug 56809 – Expose Node::isFocusable() in the Chromium WebKit API

• Component: New Bugs
• Status: RESOLVED FIXED

[Ilya Sherman]

Expose Node::isFocusable() in the Chromium WebKit API

[Ilya Sherman]

Created attachment 86439 [details]
Patch

[Dimitri Glazkov (Google)]

Can you explain how you're going to use this? I am not sure isFocusable is what you really need here.

[Ilya Sherman]

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

[James Robinson]

Can you define formally what "should not be autofilled" means?

[Ilya Sherman]

(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

[James Robinson]

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?

[Ilya Sherman]

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

[James Robinson]

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?

[Ilya Sherman]

(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

[James Robinson]

Comment on attachment 86439 [details]
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?

[Dimitri Glazkov (Google)]

Comment on attachment 86439 [details]
Patch

ok.

[WebKit Commit Bot]

Comment on attachment 86439 [details]
Patch

Clearing flags on attachment: 86439

Committed r81873: <http://trac.webkit.org/changeset/81873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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