Bug 37719

Created attachment 53563 [details] Proposed fix

See also: bug 34285, bug 30023.

(In reply to comment #2) > See also: bug 34285, bug 30023. Good point --- oddly, we're not seeing this problem on Symbian; we should probably add the PASSWORD line there as well.

Created attachment 53699 [details] Patch set including comments regarding similar defect on other platforms for PASSWORD input fields. This proposed fix also addresses 34285's problem with PASSWORD type fields on some platforms.

Created attachment 53734 [details] This changeset contains the same fix and fixes for all style errors in the current version. Having not submitted to WebKit before, I wasn't sure if I should provide only the minimal fix meeting our style guidelines, or a larger patch file that also addresses existing style errors in the file before I came along. This patch has both the fix for the issue & fixes for all style problems found by check-webkit-style.

> Having not submitted to WebKit before, I wasn't sure if I should provide only > the minimal fix meeting our style guidelines, or a larger patch file that also > addresses existing style errors in the file before I came along. This patch has > both the fix for the issue & fixes for all style problems found by > check-webkit-style. I really depends on reviewers. Some prefer to leave the style change for another one (it makes the diff difficult to read), other prefer to do them as you go (and avoid clobbering git/svn blame). Just a few comments, your change would definitely like to have an updated version of the ChangeLog to mention which methods are modified and which are just put to the right style. I had a really hard time seeing your change among the style noise. Also make sure you set r? to all patches you want reviewers to look at and not just the first one. Make sure you obsolete the old patches so that they are not reviewed in lieu of the latest one. And welcome to WebKit!

Created attachment 53830 [details] Called out actual method to be changed in ChangeLog for reviewers.

(In reply to comment #6) > > Having not submitted to WebKit before, I wasn't sure if I should provide only > > the minimal fix meeting our style guidelines, or a larger patch file that also > > addresses existing style errors in the file before I came along. This patch has > > both the fix for the issue & fixes for all style problems found by > > check-webkit-style. > > I really depends on reviewers. Some prefer to leave the style change for > another one (it makes the diff difficult to read), other prefer to do them as > you go (and avoid clobbering git/svn blame). > > Just a few comments, your change would definitely like to have an updated > version of the ChangeLog to mention which methods are modified and which are > just put to the right style. I had a really hard time seeing your change among > the style noise. > > Also make sure you set r? to all patches you want reviewers to look at and not > just the first one. Make sure you obsolete the old patches so that they are not > reviewed in lieu of the latest one. > > And welcome to WebKit! Thanks for the warm welcome, and feedback! I've deprecated the older patches, and provided a new patch with a better ChangeLog. The style fixes definitely make the older patch file a bear to read.

Comment on attachment 53830 [details] Called out actual method to be changed in ChangeLog for reviewers. We need to separate the formatting change from the behavior change; it's simply not possible to review without a diff showing what changed. Please do one at a time. I don't care what order they go in. Also, bug fixes need to come with a regression test, either automated or manual. Does this change have any effect on desktop platforms?

(In reply to comment #9) > (From update of attachment 53830 [details]) > We need to separate the formatting change from the behavior change; it's simply > not possible to review without a diff showing what changed. Please do one at a > time. I don't care what order they go in. > > Also, bug fixes need to come with a regression test, either automated or > manual. Does this change have any effect on desktop platforms? Thanks for the feedback on the style fixes. I'll open another bug and provide just the style changes in that bug, and mark the patches here with style fixes as not relevant. I tested on the desktop (Mac) --- this does not have an effect on desktop platforms. I'm not yet sure how best to test this private method with a regression test; I can begin to look at that now if it's necessary.

Comment on attachment 53830 [details] Called out actual method to be changed in ChangeLog for reviewers. Marking obsolete as the original patch without style changes is relevant.

Comment on attachment 53734 [details] This changeset contains the same fix and fixes for all style errors in the current version. Feedback from Darin @ Apple indicates that style changes should not be included with this patch.

(In reply to comment #0) > In HTMLInputElement.cpp, shouldUseInputMethod does not return true for HTML5 > input types (TELEPHONE, NUMBER, URL, and EMAIL). > > When running on a touch-only device, touching one of these HTML5 input types > does not result in a virtual keyboard, and the user cannot enter data. Why do you check shouldUseInputMethod() for keyboard input in your platform? shouldUseInputMethod() was introduced to control IME, which is a software layer to input non-ASCII characters such as Chinese and Japanese.

Comment on attachment 53699 [details] Patch set including comments regarding similar defect on other platforms for PASSWORD input fields. This should use a switch statement so we get a warning if we leave out any input types. This would prevent this kind of problem in the future as well.

Created attachment 53984 [details] Patch fixes change log entry details. Over-the-shoulder review showed I was using prepare-ChangeLog incorrectly; change log now correctly generated with prepare-ChangeLog to ensure all fields appear.

Comment on attachment 53984 [details] Patch fixes change log entry details. This is OK, but it would be better to use a switch statement so we get a warning when we add a new input type without thinking about the value this function should return.

Created attachment 53991 [details] Implemented as a switch/case per review comment from Darin. Darin suggests a switch statement for the change; implemented as suggested. Added Darin Adler to reviewer list.

Comment on attachment 53991 [details] Implemented as a switch/case per review comment from Darin. Marking the attachment as patch.

Created attachment 53993 [details] Neglected assert required at end of function for notifying developer of missing types. Wasn't sure at first if case statements were verified by compile-time / test-time checks or not. Added assertion per other switch/case statements.

Comment on attachment 53984 [details] Patch fixes change log entry details. Cleared Darin Adler's review+ from obsolete attachment 53984 [details] so that this bug does not appear in http://webkit.org/pending-commit.

This change will make a regression on Safari/Windows and Chromium/Windows. shouldUseInputMethod() doesn't return true for type=password so that users type their passwords with IME mistakenly. I don't agree that shouldUseInputMethod() returns true for NUMBER, EMAIL and PASSWORD.

(In reply to comment #21) > This change will make a regression on Safari/Windows and Chromium/Windows. > shouldUseInputMethod() doesn't return true for type=password so that users type > their passwords with IME mistakenly. > I don't agree that shouldUseInputMethod() returns true for NUMBER, EMAIL and > PASSWORD. Understood. Would a separate set of virtual-input-only events and a test for same, and handling of the events in the Qt port be agreeable? I can see you don't want to blur the purpose of the existing events and how they're handled. Thanks!

(In reply to comment #21) > This change will make a regression on Safari/Windows and Chromium/Windows. > shouldUseInputMethod() doesn't return true for type=password so that users type > their passwords with IME mistakenly. > I don't agree that shouldUseInputMethod() returns true for NUMBER, EMAIL and > PASSWORD. The reason IME's are disabled for the password field is because IMEs can access the underlying password and display it in clear text -- eg. you can use it to access the stored password for any site somewhat trivially.

(In reply to comment #22) > Would a separate set of virtual-input-only events and a test for same, and > handling of the events in the Qt port be agreeable? I can see you don't want to > blur the purpose of the existing events and how they're handled. I think isContentEditable() || isTextFormControl() is sufficient for your purpose.

(In reply to comment #23) > The reason IME's are disabled for the password field is because IMEs can access > the underlying password and display it in clear text -- eg. you can use it to > access the stored password for any site somewhat trivially. Interesting. We had better have a comment about it in the code. So shouldUseInputMethod() should be "return isTextField() && inputType() != PASSWORD;".

Created attachment 54097 [details] Patch shouldUseInputMethod() to use isTextField & inputType Changed to include comment & use isTextField() && inputType() to determine result instead of hard-coded comparisons. shouldUseInputMethod now returns true if the field in question is a text field (as determined by isTextField) that is editable and not a password field, as previously suggested.

This might be even better: return isTextField() && !isPasswordField();

Comment on attachment 54097 [details] Patch shouldUseInputMethod() to use isTextField & inputType Clearing flags on attachment: 54097 Committed r58144: <http://trac.webkit.org/changeset/58144>

All reviewed patches have been landed. Closing bug.

Revision r58144 cherry-picked into qtwebkit-2.0 with commit 2b4fb28811517b9e4512813fdd86b3a0cd43ae0f

cherry-picked into qtwebkit-4.6 with commit 4c3c42092e1325861ac1b5201e3be5f38646bf76