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.)
Created attachment 186719 [details] Patch
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').
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
Created attachment 186737 [details] Patch 2 assert and test improvement
Comment on attachment 186737 [details] Patch 2 Looks OK
Comment on attachment 186737 [details] Patch 2 Clearing flags on attachment: 186737 Committed r141960: <http://trac.webkit.org/changeset/141960>
All reviewed patches have been landed. Closing bug.