Summary: | REGRESSION (r21045): Secure keyboard entry mode remains in effect after leaving a password field by submitting | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Major | CC: | adele | ||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||
Version: | 523.x (Safari 3) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
mitz
2007-04-24 06:46:05 PDT
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.
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). (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. 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
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. |