Bug 96032

Summary: New time input needs accessibility
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: AccessibilityAssignee: Dominic Mazzoni <dmazzoni>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, donggwan.kim, keishi, tkent, yosin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96050, 96076, 96081    
Bug Blocks: 88970    
Attachments:
Description Flags
Patch
none
Proof of Concept 2
none
Patch 1
none
Patch 2
none
Patch 3
none
Patch 4 none

Description Dominic Mazzoni 2012-09-06 15:24:55 PDT
The shadow dom of the new <input type=time> implementation needs accessibility.
Comment 1 Dominic Mazzoni 2012-09-06 15:59:32 PDT
Created attachment 162608 [details]
Patch
Comment 2 Dominic Mazzoni 2012-09-06 16:04:16 PDT
Comment on attachment 162608 [details]
Patch

This isn't actually ready for review.

yosin, can you help finish this patch?
Comment 3 yosin 2012-09-07 00:28:42 PDT
Created attachment 162690 [details]
Proof of Concept 2
Comment 4 yosin 2012-09-07 00:34:31 PDT
Comment on attachment 162690 [details]
Proof of Concept 2

This is proof of concept, not for review neither commit.
Comment 5 yosin 2012-09-07 02:21:30 PDT
To fix this bug, we need to have localized strings in Chromium, http://codereview.chromium.org/10911141/
and DEPS in WebKit.
Comment 6 yosin 2012-09-11 04:00:01 PDT
Created attachment 163327 [details]
Patch 1
Comment 7 yosin 2012-09-11 04:02:26 PDT
Comment on attachment 163327 [details]
Patch 1

Could you review this patch?
Thanks in advance.

P.S.
This patch will be committed after Chromium r155713 is in WebKit/chromium/DEPS to use localized string in tests.
Comment 8 chris fleizach 2012-09-11 09:39:08 PDT
Comment on attachment 163327 [details]
Patch 1

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

My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:135
> +void DateTimeFieldElement::initialize(const AtomicString& shadowPseudoId, const String& axHelpText)

where is the axHelpText being set? i didn't see it in this patch
Is this an example of it?
field->initialize(ampmPsuedoId, AXAMPMFieldText());

In that case, it seems that aria_helpAttr might not be the right attribute

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:37
> +using namespace HTMLNames;

doesn't look like you need to use HTMLNames here based on changes to this file
Comment 9 yosin 2012-09-11 19:27:58 PDT
(In reply to comment #8)
> (From update of attachment 163327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163327&action=review
> 
> My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks

Instances of DateTimeFieldElement, reside in shadow DOM tree and they aren't seen from Web authors. Having HTML attributes, in this case ARIA attributes, in DateTimeFieldElement doesn't affect user's JavaScript code and user's code can't manipulate them.

This is the differences other elements which are visible from user's code and use AX object to implement accessibility.

> 
> > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:135
> > +void DateTimeFieldElement::initialize(const AtomicString& shadowPseudoId, const String& axHelpText)
> 
> where is the axHelpText being set? i didn't see it in this patch
> Is this an example of it?
> field->initialize(ampmPsuedoId, AXAMPMFieldText());
> 
> In that case, it seems that aria_helpAttr might not be the right attribute

axHelpText has localized message string for each field.
AXAMPMFieldText(), AXHourFieldText(), and so on, return localized string.

> 
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:37
> > +using namespace HTMLNames;
> 
> doesn't look like you need to use HTMLNames here based on changes to this file
Thanks removed.
Comment 10 yosin 2012-09-11 20:51:45 PDT
Created attachment 163511 [details]
Patch 2
Comment 11 yosin 2012-09-11 20:53:07 PDT
Comment on attachment 163511 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Remove references of HTMLNames.
Comment 12 chris fleizach 2012-09-11 22:26:39 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 163327 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163327&action=review
> > 
> > My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks
> 
> Instances of DateTimeFieldElement, reside in shadow DOM tree and they aren't seen from Web authors. Having HTML attributes, in this case ARIA attributes, in DateTimeFieldElement doesn't affect user's JavaScript code and user's code can't manipulate them.
> 
> This is the differences other elements which are visible from user's code and use AX object to implement accessibility.

Yes but that is different from things like AccessibilitySpinButton and AccessibilityListBox. Those are specific ax elements to wrap elements. What's the reason that pattern can't be followed here.

> 
> > 
> > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:135
> > > +void DateTimeFieldElement::initialize(const AtomicString& shadowPseudoId, const String& axHelpText)
> > 
> > where is the axHelpText being set? i didn't see it in this patch
> > Is this an example of it?
> > field->initialize(ampmPsuedoId, AXAMPMFieldText());
> > 
> > In that case, it seems that aria_helpAttr might not be the right attribute
> 
> axHelpText has localized message string for each field.
> AXAMPMFieldText(), AXHourFieldText(), and so on, return localized string.
> 

What are some of those examples? I suspect aria-help is not the right attribute to use

> > 
> > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:37
> > > +using namespace HTMLNames;
> > 
> > doesn't look like you need to use HTMLNames here based on changes to this file
> Thanks removed.
Comment 13 yosin 2012-09-11 23:19:46 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 163327 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=163327&action=review
> > > 
> > > My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks
> > 
> > Instances of DateTimeFieldElement, reside in shadow DOM tree and they aren't seen from Web authors. Having HTML attributes, in this case ARIA attributes, in DateTimeFieldElement doesn't affect user's JavaScript code and user's code can't manipulate them.
> > 
> > This is the differences other elements which are visible from user's code and use AX object to implement accessibility.
> 
> Yes but that is different from things like AccessibilitySpinButton and AccessibilityListBox. Those are specific ax elements to wrap elements. What's the reason that pattern can't be followed here.

Ease for implementation and reduce code size for ease of maintenance. 
Actually, I don't have strong reason not to have AX object. This is the first time to work on accessibility on WebKit.

So, should I have AccessibilityDateTimeField object?

> > > 
> > > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:135
> > > > +void DateTimeFieldElement::initialize(const AtomicString& shadowPseudoId, const String& axHelpText)
> > > 
> > > where is the axHelpText being set? i didn't see it in this patch
> > > Is this an example of it?
> > > field->initialize(ampmPsuedoId, AXAMPMFieldText());
> > > 
> > > In that case, it seems that aria_helpAttr might not be the right attribute
> > 
> > axHelpText has localized message string for each field.
> > AXAMPMFieldText(), AXHourFieldText(), and so on, return localized string.
> > 
> 
> What are some of those examples? I suspect aria-help is not the right attribute to use
> 

How about using "aria-label" instead of "aria-help"? I assume screen reader reads this string when field gets focused.
Comment 14 chris fleizach 2012-09-11 23:27:52 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (From update of attachment 163327 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=163327&action=review
> > > > 
> > > > My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks
> > > 
> > > Instances of DateTimeFieldElement, reside in shadow DOM tree and they aren't seen from Web authors. Having HTML attributes, in this case ARIA attributes, in DateTimeFieldElement doesn't affect user's JavaScript code and user's code can't manipulate them.
> > > 
> > > This is the differences other elements which are visible from user's code and use AX object to implement accessibility.
> > 
> > Yes but that is different from things like AccessibilitySpinButton and AccessibilityListBox. Those are specific ax elements to wrap elements. What's the reason that pattern can't be followed here.
> 
> Ease for implementation and reduce code size for ease of maintenance. 
> Actually, I don't have strong reason not to have AX object. This is the first time to work on accessibility on WebKit.
> 
> So, should I have AccessibilityDateTimeField object?
> 

I don't think it's necessarily wrong, I wanted to know why you chose that method

> > > > 
> > > > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:135
> > > > > +void DateTimeFieldElement::initialize(const AtomicString& shadowPseudoId, const String& axHelpText)
> > > > 
> > > > where is the axHelpText being set? i didn't see it in this patch
> > > > Is this an example of it?
> > > > field->initialize(ampmPsuedoId, AXAMPMFieldText());
> > > > 
> > > > In that case, it seems that aria_helpAttr might not be the right attribute
> > > 
> > > axHelpText has localized message string for each field.
> > > AXAMPMFieldText(), AXHourFieldText(), and so on, return localized string.
> > > 
> > 
> > What are some of those examples? I suspect aria-help is not the right attribute to use
> > 
> 
> How about using "aria-label" instead of "aria-help"? I assume screen reader reads this string when field gets focused.

Can you point to some of the strings that are used. I'm not sure what field these best fall under right now.

thanks
Comment 15 yosin 2012-09-11 23:30:59 PDT
Here is definitions of localized strings used for aria-help in "Patch 2":
http://codereview.chromium.org/10911141/diff/6/webkit/glue/webkit_strings.grd
Comment 16 chris fleizach 2012-09-11 23:37:01 PDT
(In reply to comment #15)
> Here is definitions of localized strings used for aria-help in "Patch 2":
> http://codereview.chromium.org/10911141/diff/6/webkit/glue/webkit_strings.grd

It looks like the most appropriate field is aria-valuetext
http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuetext
Comment 17 yosin 2012-09-11 23:44:21 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Here is definitions of localized strings used for aria-help in "Patch 2":
> > http://codereview.chromium.org/10911141/diff/6/webkit/glue/webkit_strings.grd
> 
> It looks like the most appropriate field is aria-valuetext
> http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuetext

We used aria-valuetext for specifying "AM" or "PM" for AM/PM field. aria-valuenow doesn't have useful information on AM/PM field, e.g. 1 or 2.

I assume AX clients use "aria-valuetext" with "aria-valuenow" at "value changed" AX notification. If we set fixed string to aria-valuetext, e.g. AM/PM, Hours, AX clients may not be able to tell current value of field.
Comment 18 chris fleizach 2012-09-11 23:47:09 PDT
Comment on attachment 163511 [details]
Patch 2

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

a few more comments. thanks

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:50
> +    setAttribute(roleAttr, "spinbutton");

a comment is needed as to what this is going to do

> Source/WebCore/html/shadow/DateTimeFieldElement.h:80
> +    void initialize(const AtomicString& shadowPseudoId, const String& axHelpText);

Part of me feels like there should be an explicit method for setting the ax attribute.
something like void setAccessibilityValueDescription(String)

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:122
> +}

if min or max changes, are we updating aria-minmax?

> LayoutTests/ChangeLog:17
> +        * fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html: Added to check accessibility notification supports in multiple fields time input UI.

these layout tests should be in the accessibility folder

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes-expected.txt:17
> +PASS helpTextOfFocusField() is "AXHelp: Hours, AXValueDescription: blank, 1, 12"

these results will only work on Mac due to the way attributes are outputted. 
You could make this test mac only, or you could do debug() output and gather expected results on other platforms

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes.html:11
> +function enableAccessibility()

this method is probably not needed

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:9
> +description('This test checks value changed accessibility notification.');

notification -> notifications.

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:11
> +function enableAccessibility()

i don't think you need a method here. it can just be inline

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:14
> +        debug("Please run inside DumpRenderTree.");

this is not really necessary

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:36
> +keyDown('\t');

why is the tab key needed?
Comment 19 chris fleizach 2012-09-11 23:51:13 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Here is definitions of localized strings used for aria-help in "Patch 2":
> > > http://codereview.chromium.org/10911141/diff/6/webkit/glue/webkit_strings.grd
> > 
> > It looks like the most appropriate field is aria-valuetext
> > http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuetext
> 
> We used aria-valuetext for specifying "AM" or "PM" for AM/PM field. aria-valuenow doesn't have useful information on AM/PM field, e.g. 1 or 2.

Yea, I feel like there's not an appropriate field to use in aria. on the mac AX implementation there is a field called

AXDateTimeComponents, which specifies what kind of date time you are looking at. 

Then the screen reader chooses the appropriate string.

I think we should ask someone in ARIA about this

> 
> I assume AX clients use "aria-valuetext" with "aria-valuenow" at "value changed" AX notification. If we set fixed string to aria-valuetext, e.g. AM/PM, Hours, AX clients may not be able to tell current value of field.
Comment 20 yosin 2012-09-12 00:01:21 PDT
(In reply to comment #18)
> (From update of attachment 163511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163511&action=review
> 
> a few more comments. thanks
> 
> > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:50
> > +    setAttribute(roleAttr, "spinbutton");
> 
> a comment is needed as to what this is going to do
> 
Will put following:

// On accessibility, DateTimeFieldElement acts like spin button.

> > Source/WebCore/html/shadow/DateTimeFieldElement.h:80
> > +    void initialize(const AtomicString& shadowPseudoId, const String& axHelpText);
> 
> Part of me feels like there should be an explicit method for setting the ax attribute.
> something like void setAccessibilityValueDescription(String)
> 
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:122
> > +}
> 
> if min or max changes, are we updating aria-minmax?

Min/max are immutable. DateTimeFieldElement is designed as immutable object except for value. DateTimeEditElement has newly create elements when layout of fields are changed, e.g. "step", "min", "max" attribute changed on the "input" element.

> > LayoutTests/ChangeLog:17
> > +        * fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html: Added to check accessibility notification supports in multiple fields time input UI.
> 
> these layout tests should be in the accessibility folder

I'll move them to accessibility folder.
 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes-expected.txt:17
> > +PASS helpTextOfFocusField() is "AXHelp: Hours, AXValueDescription: blank, 1, 12"
> 
> these results will only work on Mac due to the way attributes are outputted. 
> You could make this test mac only, or you could do debug() output and gather expected results on other platforms

I'll change to test string containment rather than string equal.

BTW, Chromium ports also uses prefix text "AXHelp" and "AXValueDescription".

http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestRunner/AccessibilityUIElementChromium.cpp#L263

> 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes.html:11
> > +function enableAccessibility()
> 
> this method is probably not needed
> 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:9
> > +description('This test checks value changed accessibility notification.');
> 
> notification -> notifications.
> 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:11
> > +function enableAccessibility()
> 
> i don't think you need a method here. it can just be inline
> 
> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:14
> > +        debug("Please run inside DumpRenderTree.");
> 
> this is not really necessary

We tried to make this test gracefully when someone opens this file in browser.

> > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html:36
> > +keyDown('\t');
> 
> why is the tab key needed?

This steps checks we have "focus" accessibility notification when focus moved into another field in same DateTimeEditElement.
Comment 21 yosin 2012-09-12 00:04:38 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Here is definitions of localized strings used for aria-help in "Patch 2":
> > > > http://codereview.chromium.org/10911141/diff/6/webkit/glue/webkit_strings.grd
> > > 
> > > It looks like the most appropriate field is aria-valuetext
> > > http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuetext
> > 
> > We used aria-valuetext for specifying "AM" or "PM" for AM/PM field. aria-valuenow doesn't have useful information on AM/PM field, e.g. 1 or 2.
> 
> Yea, I feel like there's not an appropriate field to use in aria. on the mac AX implementation there is a field called
> 
> AXDateTimeComponents, which specifies what kind of date time you are looking at. 
> 
> Then the screen reader chooses the appropriate string.
> 
> I think we should ask someone in ARIA about this
> 
> > 
> > I assume AX clients use "aria-valuetext" with "aria-valuenow" at "value changed" AX notification. If we set fixed string to aria-valuetext, e.g. AM/PM, Hours, AX clients may not be able to tell current value of field.

Dominic, could you tell us your opinion? Thanks in advance.
Comment 22 Dominic Mazzoni 2012-09-12 00:38:43 PDT
(In reply to comment #12)
> > > My main question is why did you choose to put accessibility code inside these files instead of making accessibility elements that wrapped these objects (that's the existing pattern, of course whether it's better or not is a different question).  Thanks

That question should be directed at me. I uploaded the initial proof-of-concept patch that yosin@ graciously offered to continue, so I should get the blame. :)

Here are some reasons I think it's better to implement it this way :

* The implementation of <input type=time> doesn't add any new capabilities to the rendering engine that weren't there before. The UI is all built using existing elements. So it seems easier to just add a few ARIA attributes in order to make this new control accessible, rather than add a new C++ class that would need to be maintained and kept in sync as the implementation changes.

* The fundamental idea behind how <input type=time> is implemented using existing elements is "shadow dom" or "web components". It lets you have a shadow subtree within an element that is hidden from the surrounding page. Shadow subtrees are already accessible - the accessibility tree digs into the shadow and everything works great. These APIs are going to be opened up to JS developers soon, allowing anyone to write their own subclasses of HTMLInputElement (and any other element) using pure JS. It's really important that accessibility works there, so we should eat our own dogfood and make shadow-dom-based WebKit controls accessible the same way we'd expect JS developers to do it. If functionality is missing, we should solve it now.

* It's not clear that it'd be possible to expose native "date/time picker" roles on all or most platforms. The native controls on each platform might behave differently. It'd be nice if we could get there, but from my initial exploration it doesn't look like this would "just work".

> How about using "aria-label" instead of "aria-help"? I assume screen reader reads this string when field gets focused.

This is kind of a hack. The reason I did this is because if I set a label on the <input type=time> element, the screen reader was only reading the description of the subfield (like "Hours" or "Minutes") and not the label for the control itself (like "Alarm time"). But if I put the field labels in aria-help, then screen readers get both a description and a help string for each field, and read both. Now when you tab between the fields, you hear something like:

  04, Alarm time, Hours spin button
  30, Alarm time, Minutes spin button
  00, Alarm time, Seconds spin button

It's not perfect, but I think it's the best possible without actually modifying the screen reader.

(We should definitely propose adding date/time roles to ARIA 2.0, and then get screen readers to handle them natively.)

> these results will only work on Mac due to the way attributes are outputted. 

I copied this pattern for Chromium so more of the test expectations would be the same between Mac & Chromium.

The pattern I use now to write cross-platform tests is that I test that the string ends with something, like endsWith(description, "Hours").

> > > I assume AX clients use "aria-valuetext" with "aria-valuenow" at "value changed" AX notification. If we set fixed string to aria-valuetext, e.g. AM/PM, Hours, AX clients may not be able to tell current value of field.
> 
> Dominic, could you tell us your opinion? Thanks in advance.

aria-valuetext is meant to be dynamic. It changes as you change the field value. It's a textual description of the current value of the control, when just reading the number wouldn't be clear enough. This seems like a perfect use for it, and it's well-supported across screen readers now.

I think the aria markup is pretty reasonable here, with the only exception being aria-help instead of the more common aria-label. I would prefer we figure out a way to solve this some other way eventually - maybe identify when something is in shadow DOM and return both an inner label and outer label appended together. But I think that's beyond the scope of this change. I propose we open a new bug for that issue - come up with a clean way for a focusable control with shadow dom to add a label to a subelement that augments, rather than replaces, a label on the control element itself.
Comment 23 Dominic Mazzoni 2012-09-12 00:43:19 PDT
FYI, in Chrome it's possible to view the shadow dom in the inspector / developer tools. It might be possible in WebKit nightly as well, but I'm not sure how to turn on optional developer tools experiments there.

If you do this, you get a dom something like this:

<input type="time" value="04:30">
  #shadow-root
    <div>
      <span tabindex=0 role="spinbutton" aria-help="Hours" aria-valuetext="04"...>04</span>
      <span tabindex=0 role="spinbutton" aria-help="Minutes" aria-valuetext="30"...>30</span>
      <span tabindex=0 role="spinbutton" aria-help="AM/PM" aria-valuetext="AM"...>AM</span>
    </div>
</input>
Comment 24 yosin 2012-09-12 01:08:13 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 163511 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163511&action=review
> > > LayoutTests/ChangeLog:17
> > > +        * fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html: Added to check accessibility notification supports in multiple fields time input UI.
> > 
> > these layout tests should be in the accessibility folder
> 
> I'll move them to accessibility folder.

We, forms team, wants to keep these tests under fast/forms/time-multiple-fields.
The reasons are:
  1. The implementation of AX in html/shadow instead of accessibility directory.
  2. We'll change DateTime{Edit,Field}Element soon for implementing "data", "datetime", "month", and "week". We usually run fast/forms at first try. So, it is easy to catch break on AX in DateTimeFieldElement, rather than run accessibility tests in later.
  3. Code of DateTime{Edit,Field}Element are used when ENABLE_INPUT_TYPE_TIME and ENABLE_INPUT_TYPE_TIME_MULTIPLE_FIELDS flags enabled, at this time Chromium ports only. All ports have "Skip" directive for fast/forms/time-multiple-fields directory.
Comment 25 chris fleizach 2012-09-12 08:35:03 PDT
(In reply to comment #24)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > (From update of attachment 163511 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=163511&action=review
> > > > LayoutTests/ChangeLog:17
> > > > +        * fast/forms/time-multiple-fields/time-multiple-fields-ax-value-changed-notification.html: Added to check accessibility notification supports in multiple fields time input UI.
> > > 
> > > these layout tests should be in the accessibility folder
> > 
> > I'll move them to accessibility folder.
> 
> We, forms team, wants to keep these tests under fast/forms/time-multiple-fields.
> The reasons are:
>   1. The implementation of AX in html/shadow instead of accessibility directory.
>   2. We'll change DateTime{Edit,Field}Element soon for implementing "data", "datetime", "month", and "week". We usually run fast/forms at first try. So, it is easy to catch break on AX in DateTimeFieldElement, rather than run accessibility tests in later.
>   3. Code of DateTime{Edit,Field}Element are used when ENABLE_INPUT_TYPE_TIME and ENABLE_INPUT_TYPE_TIME_MULTIPLE_FIELDS flags enabled, at this time Chromium ports only. All ports have "Skip" directive for fast/forms/time-multiple-fields directory.

Ok thanks for all the explanations. Once we can determine the best current way for identifying date components for the time being using ARIA, I think the approach looks ok
Comment 26 yosin 2012-09-12 19:09:20 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 163511 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163511&action=review
> > > LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes-expected.txt:17
> > > +PASS helpTextOfFocusField() is "AXHelp: Hours, AXValueDescription: blank, 1, 12"
> > 
> > these results will only work on Mac due to the way attributes are outputted. 
> > You could make this test mac only, or you could do debug() output and gather expected results on other platforms
> 
> I'll change to test string containment rather than string equal.
> 
> BTW, Chromium ports also uses prefix text "AXHelp" and "AXValueDescription".

It seems other ports supporting accessibility also use "AX" prefix, e.g. "AXHelp: ". To make test scripts simpler, I would like to use string equal operator rather than checking containing tests.

Note: At this time, multiple fields time input UI is enabled only for Chromium ports, except Android.
Comment 27 yosin 2012-09-12 19:16:04 PDT
Created attachment 163759 [details]
Patch 3
Comment 28 yosin 2012-09-12 19:18:45 PDT
Comment on attachment 163759 [details]
Patch 3

It seems we agree on current approach. We'll work to adopt Indie and other AX spec.

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Add a comment why we set ARIA role for DateTimeFieldElement.
* Change a word "notication" in time-multiple-fields-ax-value-changed.html
Comment 29 chris fleizach 2012-09-12 21:57:07 PDT
Comment on attachment 163759 [details]
Patch 3

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

I don't like that aria-help is used for this, but I agree there's no better choice in ARIA at this point. We should continue to pursue better API for these items

> LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-ax-aria-attributes.html:19
> +function helpTextOfFocusField()

This function should be named something like "focusedFieldValueDescription()"
Comment 30 yosin 2012-09-12 22:47:13 PDT
Created attachment 163779 [details]
Patch 4
Comment 31 yosin 2012-09-12 22:49:41 PDT
Comment on attachment 163779 [details]
Patch 4

Clearing flags on attachment: 163779

Committed r128404: <http://trac.webkit.org/changeset/128404>
Comment 32 yosin 2012-09-12 22:49:48 PDT
All reviewed patches have been landed.  Closing bug.