Bug 110740 – Should not return WebTextInputTypeNone for date input element.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Seigo Nonaka]

In the case of Windows 8, text input state including on-screen keyboard is controlled by the value of WebTextInputType returned from WebViewImpl::textInputType().
In past, it returns WebTextInputTypeDate for date text input but now it returns WebTextInputTypeNone.
WebTextInputTypeNone is used for non editable node, so on-screen keyboard will be hidden if the date text input is focused.

So in the case of Windows 8 tablet without any physical keyboards, tapping up/down arrow is the only way to input date.

[Seigo Nonaka]

Created attachment 190022 [details]
Patch

[Keishi Hattori]

I think this looks good. CCing morrita@ for review and tkent@ as FYI.

[Kent Tamura]

Comment on attachment 190022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190022&action=review

> Source/WebKit/chromium/ChangeLog:7
> +        because textInputType returns same value as non editable element.

Please put information in https://bugs.webkit.org/show_bug.cgi?id=110740#c0 here.

> Source/WebKit/chromium/ChangeLog:10
> +        (WebKit::WebViewImpl::textInputInfo):

Please write why this change is necessary.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2391
> +    if (node->isElementNode()) {
> +        HTMLElement* element = static_cast<HTMLElement*>(node);

This can cause bad casts.  You need to check isHTMLElement.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2393
> +        if (element->isDateTimeFieldElement())
> +            return WebTextInputTypeDate;

Using WebTextInputTypeDate looks wrong.
- We're going to remove it as written in WebTextInputType.h.
- This issue affects all of date/time types.  WebTextInputTypeDate is only for input[type=date].

I think using WebTextInputTypeText is better.

[Kent Tamura]

Comment on attachment 190022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190022&action=review

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2393
>> +            return WebTextInputTypeDate;
> 
> Using WebTextInputTypeDate looks wrong.
> - We're going to remove it as written in WebTextInputType.h.
> - This issue affects all of date/time types.  WebTextInputTypeDate is only for input[type=date].
> 
> I think using WebTextInputTypeText is better.

The best solution is to introduce WebTextInputTypeDateTimeField.

[Seigo Nonaka]

Created attachment 190189 [details]
Patch

[Seigo Nonaka]

Comment on attachment 190022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190022&action=review

>> Source/WebKit/chromium/ChangeLog:7
>> +        because textInputType returns same value as non editable element.
> 
> Please put information in https://bugs.webkit.org/show_bug.cgi?id=110740#c0 here.

Done.

>> Source/WebKit/chromium/ChangeLog:10
>> +        (WebKit::WebViewImpl::textInputInfo):
> 
> Please write why this change is necessary.

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2391
>> +        HTMLElement* element = static_cast<HTMLElement*>(node);
> 
> This can cause bad casts.  You need to check isHTMLElement.

Done.

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:2393
>>> +            return WebTextInputTypeDate;
>> 
>> Using WebTextInputTypeDate looks wrong.
>> - We're going to remove it as written in WebTextInputType.h.
>> - This issue affects all of date/time types.  WebTextInputTypeDate is only for input[type=date].
>> 
>> I think using WebTextInputTypeText is better.
> 
> The best solution is to introduce WebTextInputTypeDateTimeField.

We can't use WebTextInputTypeText for other reasons, so let me introduce WebTextInputTypeDateTimeField.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kent Tamura]

Comment on attachment 190189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190189&action=review

> Source/WebKit/chromium/ChangeLog:18
> +        With this patch, textInputInfo fills type filed regardless of editable or not, but it is
> +        safe because textInputType returns editable type only for known editable element.
> +
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::textInputInfo):

The paragraph should be on the function list like:

* src/WebViewImpl.cpp:
(WebKit::WebViewImpl::textInputInfo): Fills type filed regardless of editable or not. It is safe because textInputType returns editable type only for known editable element.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2391
> +    if (node->isElementNode()) {
> +        HTMLElement* element = static_cast<HTMLElement*>(node);

The code should be:

if (node->isHTMLElement()) {
    HTMLElement* element = toHTMLElement(node);

or

if (node->isElementNode()) {
    Element* element = toElement(node);

[Seigo Nonaka]

Created attachment 190446 [details]
Patch

[Seigo Nonaka]

Comment on attachment 190189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190189&action=review

>> Source/WebKit/chromium/ChangeLog:18
>> +        (WebKit::WebViewImpl::textInputInfo):
> 
> The paragraph should be on the function list like:
> 
> * src/WebViewImpl.cpp:
> (WebKit::WebViewImpl::textInputInfo): Fills type filed regardless of editable or not. It is safe because textInputType returns editable type only for known editable element.

Sure, sorry for my misunderstanding.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2391
>> +        HTMLElement* element = static_cast<HTMLElement*>(node);
> 
> The code should be:
> 
> if (node->isHTMLElement()) {
>     HTMLElement* element = toHTMLElement(node);
> 
> or
> 
> if (node->isElementNode()) {
>     Element* element = toElement(node);

Sure, done.

[Kent Tamura]

Comment on attachment 190446 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 190446 [details]
Patch

Clearing flags on attachment: 190446

Committed r144179: <http://trac.webkit.org/changeset/144179>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110980

[Vsevolod Vlasov]

This patch broke build:

obj/content/renderer/content_renderer.render_widget.o
../../content/renderer/render_widget.cc:1978:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1980:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1982:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1984:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1986:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1988:error: size of array 'mismatching_enum' is negative
../../content/renderer/render_widget.cc:1990:error: size of array 'mismatching_enums' is negative
../../content/renderer/render_widget.cc:1990:error: conflicting declaration 'typedef struct GpuCompileAssert<false> content::mismatching_enums [1]'
../../content/renderer/render_widget.cc:1976:error: 'content::mismatching_enums' has a previous declaration as 'typedef struct GpuCompileAssert<true> content::mismatching_enums [1]'
../../content/renderer/render_widget.cc:1992:error: size of array 'mismatching_enums' is negative
../../content/renderer/render_widget.cc:1992:error: conflicting declaration 'typedef struct GpuCompileAssert<false> content::mismatching_enums [1]'
../../content/renderer/render_widget.cc:1976:error: 'content::mismatching_enums' has a previous declaration as 'typedef struct GpuCompileAssert<true> content::mismatching_enums [1]'
ninja: build stopped: subcommand failed.

[Vsevolod Vlasov]

Reverted: http://trac.webkit.org/changeset/144185

[Seigo Nonaka]

Sorry for build break.
There is a compile assert for checking enum compatibility.

To fix the build breakage, we should add new enum entry TEXT_INPUT_TYPE_DATE_TIME_FIELD into ui/base/ime/text_input_type.h but it should be done at the same time of landing this patch.
Otherwise build will be break again.

I will remove compilation assertion temporary.
Then after landing this patch, I will add compilation assertion again with new enum sets.

Thank you.

[Seigo Nonaka]

James suggested me that we should add new enum entry into the end to preserve existing value.
So I will update the patch.

Thank you.

[Seigo Nonaka]

Created attachment 190538 [details]
Patch

[Kent Tamura]

Comment on attachment 190538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190538&action=review

> Source/WebKit/chromium/public/WebTextInputType.h:64
>      // Input caret is in a contenteditable node (not an INPUT field).
>      WebTextInputTypeContentEditable,
> +    WebTextInputTypeDateTimeField,

WebTextInputTypeDateTimeField looks like a kind of contenteditable because of the comment.

[Seigo Nonaka]

Created attachment 190642 [details]
Patch

[Seigo Nonaka]

Comment on attachment 190538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190538&action=review

>> Source/WebKit/chromium/public/WebTextInputType.h:64
>> +    WebTextInputTypeDateTimeField,
> 
> WebTextInputTypeDateTimeField looks like a kind of contenteditable because of the comment.

Sure, added comment for date time field.

[Kent Tamura]

Comment on attachment 190642 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 190642 [details]
Patch

Clearing flags on attachment: 190642

Committed r144261: <http://trac.webkit.org/changeset/144261>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.