Bug 37719 - Some HTML5 Input tags not treated as needing an input method.
Summary: Some HTML5 Input tags not treated as needing an input method.
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware Other
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-04-16 11:47 PDT by Ray
Modified: 2010-06-18 03:03 PDT (History)
8 users (show)

See Also:


Attachments
Proposed fix (1.31 KB, patch)
2010-04-16 14:11 PDT, Ray
no flags Details | Formatted Diff | Diff
Patch set including comments regarding similar defect on other platforms for PASSWORD input fields. (1.33 KB, patch)
2010-04-19 11:48 PDT, Ray
no flags Details | Formatted Diff | Diff
This changeset contains the same fix and fixes for all style errors in the current version. (31.61 KB, patch)
2010-04-19 16:01 PDT, Ray
Raymond.Rischpater: review-
Details | Formatted Diff | Diff
Called out actual method to be changed in ChangeLog for reviewers. (31.76 KB, patch)
2010-04-20 09:59 PDT, Ray
darin: review-
Details | Formatted Diff | Diff
Patch fixes change log entry details. (1.37 KB, patch)
2010-04-21 13:27 PDT, Ray
no flags Details | Formatted Diff | Diff
Implemented as a switch/case per review comment from Darin. (1.70 KB, patch)
2010-04-21 14:21 PDT, Ray
no flags Details | Formatted Diff | Diff
Neglected assert required at end of function for notifying developer of missing types. (1.72 KB, patch)
2010-04-21 14:41 PDT, Ray
darin: review+
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch shouldUseInputMethod() to use isTextField & inputType (1.60 KB, patch)
2010-04-22 14:33 PDT, Ray
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ray 2010-04-16 11:47:49 PDT
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.

(Patch to be supplied for fix shortly.)
Comment 1 Ray 2010-04-16 14:11:26 PDT
Created attachment 53563 [details]
Proposed fix
Comment 2 Alexey Proskuryakov 2010-04-16 16:02:39 PDT
See also: bug 34285, bug 30023.
Comment 3 Ray 2010-04-19 11:45:07 PDT
(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.
Comment 4 Ray 2010-04-19 11:48:17 PDT
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.
Comment 5 Ray 2010-04-19 16:01:56 PDT
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.
Comment 6 Julien Chaffraix 2010-04-20 09:34:16 PDT
> 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!
Comment 7 Ray 2010-04-20 09:59:41 PDT
Created attachment 53830 [details]
Called out actual method to be changed in ChangeLog for reviewers.
Comment 8 Ray 2010-04-20 10:00:41 PDT
(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 9 Darin Adler 2010-04-20 10:32:05 PDT
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?
Comment 10 Ray 2010-04-20 11:57:04 PDT
(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 11 Ray 2010-04-20 11:58:13 PDT
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 12 Ray 2010-04-20 11:59:05 PDT
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.
Comment 13 Kent Tamura 2010-04-20 16:35:10 PDT
(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 14 Darin Adler 2010-04-21 13:19:11 PDT
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.
Comment 15 Ray 2010-04-21 13:27:51 PDT
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 16 Darin Adler 2010-04-21 13:31:20 PDT
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.
Comment 17 Ray 2010-04-21 14:21:20 PDT
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 18 Laszlo Gombos 2010-04-21 14:34:26 PDT
Comment on attachment 53991 [details]
Implemented as a switch/case per review comment from Darin.

Marking the attachment as patch.
Comment 19 Ray 2010-04-21 14:41:37 PDT
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 20 Eric Seidel (no email) 2010-04-21 18:12:06 PDT
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.
Comment 21 Kent Tamura 2010-04-22 01:28:05 PDT
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.
Comment 22 Ray 2010-04-22 09:11:23 PDT
(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!
Comment 23 Oliver Hunt 2010-04-22 09:17:44 PDT
(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.
Comment 24 Kent Tamura 2010-04-22 09:19:58 PDT
(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.
Comment 25 Kent Tamura 2010-04-22 09:24:23 PDT
(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;".
Comment 26 Ray 2010-04-22 14:33:50 PDT
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.
Comment 27 Darin Adler 2010-04-22 17:14:02 PDT
This might be even better:

    return isTextField() && !isPasswordField();
Comment 28 WebKit Commit Bot 2010-04-22 20:40:05 PDT
Comment on attachment 54097 [details]
Patch shouldUseInputMethod() to use isTextField & inputType

Clearing flags on attachment: 54097

Committed r58144: <http://trac.webkit.org/changeset/58144>
Comment 29 WebKit Commit Bot 2010-04-22 20:40:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Simon Hausmann 2010-04-26 03:42:07 PDT
Revision r58144 cherry-picked into qtwebkit-2.0 with commit 2b4fb28811517b9e4512813fdd86b3a0cd43ae0f
Comment 31 Simon Hausmann 2010-06-18 03:03:24 PDT
cherry-picked into qtwebkit-4.6 with commit 4c3c42092e1325861ac1b5201e3be5f38646bf76