Bug 108795 – INPUT_MULTIPLE_FIELDS_UI: Read-only inputs should be focusable

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

INPUT_MULTIPLE_FIELDS_UI: Read-only inputs should be focusable.

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#the-readonly-attribute
> Note: The difference between disabled and readonly is that read-only controls are still focusable, so the user can still select the text and interact with it, whereas disabled controls are entirely non-interactive. (For this reason, only text controls can be made read-only: it wouldn't make sense for checkboxes or buttons, for instances.)

[Kent Tamura]

Created attachment 186719 [details]
Patch

[Kentaro Hara]

Comment on attachment 186719 [details]
Patch

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

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:-130
> -    if (isDisabled())
> -        return;

Is it OK to remove it? (Did you check all the call sites of handleKeyboardEvent() ?)

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html:157
> +beginTest('Tab navigation should not skip readonly inputs, but editing operations should be ignored.', '');
>  before.focus();
> +input.value = '01:01';
>  input.readOnly = true;
>  keyDown('\t');
> +shouldBeEqualToString('document.activeElement.id', 'input');
> +shouldBeEqualToString('keyDown("upArrow"); input.value', '01:01');
> +shouldBeEqualToString('keyDown("downArrow"); input.value', '01:01');
> +keyDown('rightArrow');
> +shouldBeEqualToString('keyDown("3"); input.value', '01:01');
> +keyDown('\t');
> +keyDown('\t');

This test looks a bit weak to test the change. Could you test a focused element after each action by using internals.youngestShadowRoot(input).activeElement.getAttribute("pseudo") ? Also it would be good to add a test for keyDown('leftArrow').

[Kent Tamura]

Comment on attachment 186719 [details]
Patch

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

>> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:-130
>> -        return;
> 
> Is it OK to remove it? (Did you check all the call sites of handleKeyboardEvent() ?)

I'm very sure it's ok.  I'll change this part to ASSERT(!isDisabled());

>> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-keyboard-events.html:157
>> +keyDown('\t');
> 
> This test looks a bit weak to test the change. Could you test a focused element after each action by using internals.youngestShadowRoot(input).activeElement.getAttribute("pseudo") ? Also it would be good to add a test for keyDown('leftArrow').

will do

[Kent Tamura]

Created attachment 186737 [details]
Patch 2

assert and test improvement

[Kentaro Hara]

Comment on attachment 186737 [details]
Patch 2

Looks OK

[WebKit Review Bot]

Comment on attachment 186737 [details]
Patch 2

Clearing flags on attachment: 186737

Committed r141960: <http://trac.webkit.org/changeset/141960>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.