Summary: | INPUT_MULTIPLE_FIELDS_UI: Read-only inputs should be focusable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||
Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | haraken, mifenton, morrita, ojan.autocc, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 108447, 108911, 108914, 108924 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Kent Tamura
2013-02-03 23:34:56 PST
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. |