Bug 156045 – AX: <attachment> element not accessible

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Need to make this new element accessible

<rdar://problem/25323108>

[chris fleizach]

Created attachment 275236 [details]
patch

[Joanmarie Diggs]

Comment on attachment 275236 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:95
> +{

Disclaimer: I need to locate and read docs about the Attachment element. I'll do so, but not tonight. If my subsequent questions about this implementation are silly, feel free to say so. :)

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:110
> +            combinedLabel.append(", ");

Summary question: Is having a comma-separated, combined label really desired (or defined as expected, e.g. in a spec)?

Sub-questions if this exposure is not defined, e.g. in a spec:
1. Will any AT have a need to do anything other than present the combined result in speech and/or braille?
2. Will parsing ever be needed (to, say, speak/repeat just the action or just the title)?
3. Might a screen reader want to pause as a means to provide the user with an auditory hint of where the title stops and the subtitle begins?
4. If the user has set punctuation level to all, won't the commas be spoken? I think they might be. Is this the expected/desired outcome? After all they seem to be an artifact of this exposure; not commas that are actually there in the text alternative.
5. (Another flavor of 4) Can one of the attributes end with punctuation? Will concatenating that with a comma result in expected presentation?

If the answers to any of the above sub-questions are yes, then I don't think having a comma-separated, combined label is desired. (But I'm happy to be convinced otherwise. See disclaimer above.)

All of the above also makes me wonder: Would it make sense to add some new types to AccessibilityTextSource to handle the title, subtitle, and action?

[chris fleizach]

(In reply to comment #2)
> Comment on attachment 275236 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275236&action=review
> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:95
> > +{
> 
> Disclaimer: I need to locate and read docs about the Attachment element.
> I'll do so, but not tonight. If my subsequent questions about this
> implementation are silly, feel free to say so. :)
> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:110
> > +            combinedLabel.append(", ");
> 
> Summary question: Is having a comma-separated, combined label really desired
> (or defined as expected, e.g. in a spec)?

not defined anywhere. it seems a reasonable thing to do for me. at least the fields are separated somehow. otherwise they'd just run together

> 
> Sub-questions if this exposure is not defined, e.g. in a spec:
> 1. Will any AT have a need to do anything other than present the combined
> result in speech and/or braille?

what else were you thinking of ?

> 2. Will parsing ever be needed (to, say, speak/repeat just the action or
> just the title)?

Not at this time

> 3. Might a screen reader want to pause as a means to provide the user with
> an auditory hint of where the title stops and the subtitle begins?

probably. thats what the comma serves in my mind

> 4. If the user has set punctuation level to all, won't the commas be spoken?
> I think they might be. Is this the expected/desired outcome? After all they
> seem to be an artifact of this exposure; not commas that are actually there
> in the text alternative.

Yes this is true. It's a side effect of combining multiple pieces of text into one contiguous element. since it's not a piece of content per se, but a user interface element, i don't think that's too much of an issue

> 5. (Another flavor of 4) Can one of the attributes end with punctuation?
> Will concatenating that with a comma result in expected presentation?

I think so. Please review my implementation

> 
> If the answers to any of the above sub-questions are yes, then I don't think
> having a comma-separated, combined label is desired. (But I'm happy to be
> convinced otherwise. See disclaimer above.)
> 
> All of the above also makes me wonder: Would it make sense to add some new
> types to AccessibilityTextSource to handle the title, subtitle, and action?

Possibly, but would it be used elsewhere? not sure. then each platform presumably would have to combine in some way

[Joanmarie Diggs]

(In reply to comment #3)
> > Sub-questions if this exposure is not defined, e.g. in a spec:
> > 1. Will any AT have a need to do anything other than present the combined
> > result in speech and/or braille?
> 
> what else were you thinking of ?

Since I've not yet read up on the attachment element, the following is completely artificial. But... Some screen readers have a command to provide detailed information (in Orca it's the WhereAmI command). Hypothetically, Orca might wish to say:

Title is: Foo
Subtitle is: Bar
Action is: Baz

Some screen readers have "brief" and "verbose" settings. Maybe users of the "brief" verbosity level would not want to hear the subtitle by default.
 
> > 3. Might a screen reader want to pause as a means to provide the user with
> > an auditory hint of where the title stops and the subtitle begins?
> 
> probably. thats what the comma serves in my mind

That was my suspicion. What if there's a title which contains a comma, no subtitle, and an action?

> Yes this is true. It's a side effect of combining multiple pieces of text
> into one contiguous element. since it's not a piece of content per se, but a
> user interface element, i don't think that's too much of an issue

End of the world issue? No, I agree with you. Something end users might report as a bug, I think it might be. Again, what if one of the attributes has an actual, author-provided comma? A user might want that author-provided comma spoken, but not the commas that come from WebKit.

IF concatenated strings are indeed desired, what about using a newline char instead of a comma? I *think* most synthesizers pause at a newline char, similar to what they would do for comma. I do not *think* most synthesizers would speak the newline char as a literal char, even at its most verbose settings. Though I'm not positive about that.

> > 5. (Another flavor of 4) Can one of the attributes end with punctuation?
> > Will concatenating that with a comma result in expected presentation?
> 
> I think so. Please review my implementation

I think in at least some cases it might not. It would depend on the synthesizer's handling of two back-to-back punctuation characters.

> > All of the above also makes me wonder: Would it make sense to add some new
> > types to AccessibilityTextSource to handle the title, subtitle, and action?
> 
> Possibly, but would it be used elsewhere? not sure. then each platform
> presumably would have to combine in some way

True. But each platform might want them combined (or not) in different ways.

@Mario: Any opinions?

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > > Sub-questions if this exposure is not defined, e.g. in a spec:
> > > 1. Will any AT have a need to do anything other than present the combined
> > > result in speech and/or braille?
> > 
> > what else were you thinking of ?
> 
> Since I've not yet read up on the attachment element, the following is
> completely artificial. But... Some screen readers have a command to provide
> detailed information (in Orca it's the WhereAmI command). Hypothetically,
> Orca might wish to say:
> 
> Title is: Foo
> Subtitle is: Bar
> Action is: Baz
> 
> Some screen readers have "brief" and "verbose" settings. Maybe users of the
> "brief" verbosity level would not want to hear the subtitle by default.
>  
> > > 3. Might a screen reader want to pause as a means to provide the user with
> > > an auditory hint of where the title stops and the subtitle begins?
> > 
> > probably. thats what the comma serves in my mind
> 
> That was my suspicion. What if there's a title which contains a comma, no
> subtitle, and an action?
> 
> > Yes this is true. It's a side effect of combining multiple pieces of text
> > into one contiguous element. since it's not a piece of content per se, but a
> > user interface element, i don't think that's too much of an issue
> 
> End of the world issue? No, I agree with you. Something end users might
> report as a bug, I think it might be. Again, what if one of the attributes
> has an actual, author-provided comma? A user might want that author-provided
> comma spoken, but not the commas that come from WebKit.
> 
> IF concatenated strings are indeed desired, what about using a newline char
> instead of a comma? I *think* most synthesizers pause at a newline char,
> similar to what they would do for comma. I do not *think* most synthesizers
> would speak the newline char as a literal char, even at its most verbose
> settings. Though I'm not positive about that.
> 
> > > 5. (Another flavor of 4) Can one of the attributes end with punctuation?
> > > Will concatenating that with a comma result in expected presentation?
> > 
> > I think so. Please review my implementation
> 
> I think in at least some cases it might not. It would depend on the
> synthesizer's handling of two back-to-back punctuation characters.
> 
> > > All of the above also makes me wonder: Would it make sense to add some new
> > > types to AccessibilityTextSource to handle the title, subtitle, and action?
> > 
> > Possibly, but would it be used elsewhere? not sure. then each platform
> > presumably would have to combine in some way
> 
> True. But each platform might want them combined (or not) in different ways.
> 
> @Mario: Any opinions?

Will see if I can modify accessibilityText to break this sub components down further. thanks

[chris fleizach]

Created attachment 275298 [details]
patch

[chris fleizach]

Joanmarie how does this look?

[Joanmarie Diggs]

Comment on attachment 275298 [details]
patch

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

This looks 1000 times better to me. I really appreciate your taking this approach.

As stated inline, the EWS build failures seem to be quite valid and directly the result of this patch. I'm assuming the fix will be trivial, so r+ing now. If my assumption is wrong, ping me. Thanks!

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:79
> +    return AXAttachmentRoleText();

Where is AXAttachmentRoleText() defined? This, btw, appears to be why some of the EWS bots went red with your patch.

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:98
> +        textOrder.append(AccessibilityText(title, TitleText));

Thank you!

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:101
> +        textOrder.append(AccessibilityText(subtitle, SubtitleText));

Thank you!

> Source/WebCore/accessibility/AccessibilityAttachment.cpp:104
> +        textOrder.append(AccessibilityText(action, ActionText));

Thank you!

> Source/WebCore/accessibility/AccessibilityAttachment.h:58
> +SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityAttachment, isAttachment())

Should this be isAttachmentElement() instead of isAttachment()?

Related aside: Looking at where isAttachment() gets used, I get the impression that isAttachment() is for things which have nothing to do with the 'attachment' element. If that is indeed the case, it's obviously beyond the scope of this bug and implementation, but perhaps worth addressing in a new bug.

> LayoutTests/platform/gtk/TestExpectations:92
> +webkit.org/b/156045 accessibility/attachment-element.html [ Skip ]

Thanks for doing this, too.

[chris fleizach]

(In reply to comment #8)
> Comment on attachment 275298 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275298&action=review
> 
> This looks 1000 times better to me. I really appreciate your taking this
> approach.
> 
> As stated inline, the EWS build failures seem to be quite valid and directly
> the result of this patch. I'm assuming the fix will be trivial, so r+ing
> now. If my assumption is wrong, ping me. Thanks!

yea will make sure this works.

> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:79
> > +    return AXAttachmentRoleText();
> 
> Where is AXAttachmentRoleText() defined? This, btw, appears to be why some
> of the EWS bots went red with your patch.

yea probably forgot to include in the patch

> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:98
> > +        textOrder.append(AccessibilityText(title, TitleText));
> 
> Thank you!
> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:101
> > +        textOrder.append(AccessibilityText(subtitle, SubtitleText));
> 
> Thank you!
> 
> > Source/WebCore/accessibility/AccessibilityAttachment.cpp:104
> > +        textOrder.append(AccessibilityText(action, ActionText));
> 
> Thank you!
> 
> > Source/WebCore/accessibility/AccessibilityAttachment.h:58
> > +SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityAttachment, isAttachment())
> 
> Should this be isAttachmentElement() instead of isAttachment()?

Ok

> 
> Related aside: Looking at where isAttachment() gets used, I get the
> impression that isAttachment() is for things which have nothing to do with
> the 'attachment' element. If that is indeed the case, it's obviously beyond
> the scope of this bug and implementation, but perhaps worth addressing in a
> new bug.
> 

yea, we'll need some new naming for that old attachment concept

> > LayoutTests/platform/gtk/TestExpectations:92
> > +webkit.org/b/156045 accessibility/attachment-element.html [ Skip ]
> 
> Thanks for doing this, too.

[chris fleizach]

Created attachment 275329 [details]
patch

[Build Bot]

Comment on attachment 275329 [details]
patch

Attachment 275329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1077281

New failing tests:
accessibility/attachment-element.html

[Build Bot]

Created attachment 275337 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275329 [details]
patch

Attachment 275329 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1077347

New failing tests:
accessibility/attachment-element.html

[Build Bot]

Created attachment 275340 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275329 [details]
patch

Attachment 275329 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1077360

New failing tests:
accessibility/attachment-element.html

[Build Bot]

Created attachment 275342 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joanmarie Diggs]

Comment on attachment 275329 [details]
patch

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

The isAttachment() versus isAttachmentElement() change wasn't made in this version.

Regarding the layout test failure, it looks like the platform-specific exposure -- in your case the concatenation -- isn't being done in... baseAccessibilityDescription()? My apologies for not catching that before.

> Source/WebCore/accessibility/AccessibilityAttachment.h:58
> +SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityAttachment, isAttachment())

isAttachmentElement(), right?

[chris fleizach]

(In reply to comment #17)
> Comment on attachment 275329 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275329&action=review
> 
> The isAttachment() versus isAttachmentElement() change wasn't made in this
> version.
> 
> Regarding the layout test failure, it looks like the platform-specific
> exposure -- in your case the concatenation -- isn't being done in...
> baseAccessibilityDescription()? My apologies for not catching that before.

the tests run fine for me locally, so i don't know whats going on right now

> 
> > Source/WebCore/accessibility/AccessibilityAttachment.h:58
> > +SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityAttachment, isAttachment())
> 
> isAttachmentElement(), right?

[chris fleizach]

Created attachment 275358 [details]
patch

[WebKit Commit Bot]

Attachment 275358 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:206:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 275359 [details]
patch

[Joanmarie Diggs]

Comment on attachment 275359 [details]
patch

> the tests run fine for me locally, so i don't know whats going on right now

Maybe webkit-patch upload gremlins. I've had it do weird things to me in the past. <insert shrug here>

Anyway, thanks again for making the change! I've not yet found any docs on the attachment element. Are there any? It looks like a neat addition, and one I'd like to make accessible in WebKitGtk. But my google powers have failed me. And I've not found docs pointed to by the initial (non-accessibility) support commits.

[chris fleizach]

(In reply to comment #22)
> Comment on attachment 275359 [details]
> patch
> 
> > the tests run fine for me locally, so i don't know whats going on right now
> 
> Maybe webkit-patch upload gremlins. I've had it do weird things to me in the
> past. <insert shrug here>
> 
> Anyway, thanks again for making the change! I've not yet found any docs on
> the attachment element. Are there any? It looks like a neat addition, and
> one I'd like to make accessible in WebKitGtk. But my google powers have
> failed me. And I've not found docs pointed to by the initial
> (non-accessibility) support commits.

my guess is that it's an apple only thing right now

[WebKit Commit Bot]

Comment on attachment 275359 [details]
patch

Clearing flags on attachment: 275359

Committed r198942: <http://trac.webkit.org/changeset/198942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.