Bug 13471 – REGRESSION (r21045): Secure keyboard entry mode remains in effect after leaving a password field by submitting

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

To reproduce:
1) Use System Preferences > International > Input Menu to enable a non-Roman input source, e.g. Arabic.
2) Open the attached test case and click in the password field.
3) Check that non-Roman input sources are disabled in the Input menu (the flag on the right side of the menu bar).
4) Press Return.
5) Click in the text field.
6) Check that non-Roman input sources are enabled and selectable in the Input menu.

Expected results: disabled in 3) and enabled in 6).
Actual results: disabled in 3) and in 6).

Regressed in <http://trac.webkit.org/projects/webkit/changeset/21045>.

Created attachment 14156 [details]
Test case

Created attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

The reason why you can reset to false unconditionally is that pages containing password fields are not allowed in the page cache, so you know that in setView() you're never entering a page with a focused password field.

If you think it's necessary, I can add a FIXME in FrameLoader::canCachePage() saying that if you ever decide to allow password fields in the page cache, then you should address this problem.

[Darin Adler]

Comment on attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

Why not spell it "submit" everywhere?

What's the key reason this needs to be called? Is it really the change in view that's the issue? Doesn't the input element lose focus? I think this fix is fine, but it would be stronger if I understood exactly why this is the right point in time to clear this. It has to have something about the input that was sponsoring secure keyboard entry.

r=me

Comment on attachment 14157 [details]
Reset secure keyboard entry mode in Frame::setView

(In reply to comment #3)
> (From update of attachment 14157 [details] [edit])
> Why not spell it "submit" everywhere?

Why not say "you made a typo"? ;-)

> What's the key reason this needs to be called? Is it really the change in view
> that's the issue? Doesn't the input element lose focus?

In many senses, it never does (so I think that even prior to r21045, if you happened to be in a secure text field when the new page was loaded, you'd remain in Roman-only mode). The Frame doesn't keep track of focus, it just serves requests from nodes to switch secure mode on or off. Presumably, those nodes are in the Frame's Document. So now I'm thinking it may be more appropriate to patch setDocument() instead of setView(). It would still just reset to false unconditionally, since it can't (and doesn't need to) ask the new document's focused node if it requires secure keyboard entry. However, that could be changed in the future (if password fields can go into the page cache).

[Darin Adler]

(In reply to comment #4)
> Why not say "you made a typo"? ;-)

Sorry, I was just in a silly mood.

> > What's the key reason this needs to be called? Is it really the change in view
> > that's the issue? Doesn't the input element lose focus?
> 
> In many senses, it never does (so I think that even prior to r21045, if you
> happened to be in a secure text field when the new page was loaded, you'd
> remain in Roman-only mode). The Frame doesn't keep track of focus, it just
> serves requests from nodes to switch secure mode on or off. Presumably, those
> nodes are in the Frame's Document. So now I'm thinking it may be more
> appropriate to patch setDocument() instead of setView().

Sounds good; easier to explain at least. I think either is fine in practice.

> It would still just
> reset to false unconditionally, since it can't (and doesn't need to) ask the
> new document's focused node if it requires secure keyboard entry. However, that
> could be changed in the future (if password fields can go into the page cache).

Makes sense. This is subtle enough that I think we need a comment or two. I really like the idea of putting a comment in FrameLoader::canCachePage -- I don't think it needs to be a FIXME, just a warning that if you change the rule then this other code will need to change too.

Created attachment 14159 [details]
Reset secure entry mode in setDocument

This turned out to be a slightly bigger change.

I can go all the way and de-friend FrameLoader and Frame by adding a couple of public accessors and making cleanupScriptObjects() public. Feel free to r- if you want me to do it (or for any other reason!).

Comment on attachment 14159 [details]
Reset secure entry mode in setDocument

Wait! Found another typo!

Created attachment 14160 [details]
Reset secure entry mode in setDocument

See comment #6.

[Darin Adler]

Comment on attachment 14160 [details]
Reset secure entry mode in setDocument

I do like the idea of "going all the way" and eliminating the friendship, but I don't see any need to do this as part of this patch.

setDocument should be changed to take a PassRefPtr.

r=me

[Darin Adler]

<rdar://problem/5163498>

[Darin Adler]

Sending        WebCore/ChangeLog
Sending        WebCore/loader/FrameLoader.cpp
Adding         WebCore/manual-tests/secure-keyboard-enabled-after-submit.html
Sending        WebCore/page/Frame.cpp
Transmitting file data ....
Committed revision 21172.