Bug 221875 – AX: Image should report the embedded accessibility description if available

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

Images may contain metadata meant for screenreaders to consume to users

That data should be exposed through the AX API using the API in MediaAccessibility

[Radar WebKit Bug Importer]

<rdar://problem/74320584>

[chris fleizach]

Created attachment 420238 [details]
patch

[chris fleizach]

Created attachment 420334 [details]
patch

[chris fleizach]

Created attachment 420338 [details]
Patch

[chris fleizach]

Created attachment 420362 [details]
Patch

[chris fleizach]

Created attachment 420365 [details]
Patch

[chris fleizach]

Created attachment 420368 [details]
Patch

[chris fleizach]

Created attachment 420383 [details]
Patch

[chris fleizach]

Created attachment 420393 [details]
Patch

[chris fleizach]

Created attachment 420396 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #10)
> Created attachment 420396 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -1011,6 +1011,7 @@ public:
     virtual double estimatedLoadingProgress() const = 0;
     virtual String brailleLabel() const = 0;
     virtual String brailleRoleDescription() const = 0;
+    virtual String embeddedImageDescription() const = 0;

Do we need to expose this in the AXCoreObject interface? or we are better off adding this to the current image description method.

I.e., we already describe images. So this would be one more source for the image description.

[chris fleizach]

Created attachment 420427 [details]
Patch

[chris fleizach]

(In reply to Andres Gonzalez from comment #11)
> (In reply to chris fleizach from comment #10)
> > Created attachment 420396 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> +++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> @@ -1011,6 +1011,7 @@ public:
>      virtual double estimatedLoadingProgress() const = 0;
>      virtual String brailleLabel() const = 0;
>      virtual String brailleRoleDescription() const = 0;
> +    virtual String embeddedImageDescription() const = 0;
> 
> Do we need to expose this in the AXCoreObject interface? or we are better
> off adding this to the current image description method.
> 
> I.e., we already describe images. So this would be one more source for the
> image description.

This needs to be returned through a new attribute on Mac, so I think we have a good reason to separate it out

[chris fleizach]

Created attachment 420433 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #13)
> (In reply to Andres Gonzalez from comment #11)
> > (In reply to chris fleizach from comment #10)
> > > Created attachment 420396 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> > +++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> > @@ -1011,6 +1011,7 @@ public:
> >      virtual double estimatedLoadingProgress() const = 0;
> >      virtual String brailleLabel() const = 0;
> >      virtual String brailleRoleDescription() const = 0;
> > +    virtual String embeddedImageDescription() const = 0;
> > 
> > Do we need to expose this in the AXCoreObject interface? or we are better
> > off adding this to the current image description method.
> > 
> > I.e., we already describe images. So this would be one more source for the
> > image description.
> 
> This needs to be returned through a new attribute on Mac, so I think we have
> a good reason to separate it out

I don't see why we need a separate attribute on Mac since we already have an attribute that returns an image description. I see this as another source of alt text, but coming from the image metadata instead of from the markup. But I may be missing some use case here.

[Andres Gonzalez]

--- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
+++ a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
@@ -1661,6 +1661,16 @@ bool AccessibilityUIElement::isCollapsed() const
     return false;
 }

+JSRetainPtr<JSStringRef> AccessibilityUIElement::embeddedImageDescription() const
+{
+    BEGIN_AX_OBJC_EXCEPTIONS
+    id value = [m_element accessibilityAttributeValue:@"AXEmbeddedImageDescription"];

Use static attributeValue instead, to do the simulated AX thread in isolated tree mode:

    id value = attributeValue(m_element.get(), @"AXEmbeddedImageDescription");

[chris fleizach]

(In reply to Andres Gonzalez from comment #15)
> (In reply to chris fleizach from comment #13)
> > (In reply to Andres Gonzalez from comment #11)
> > > (In reply to chris fleizach from comment #10)
> > > > 
> 
> I don't see why we need a separate attribute on Mac since we already have an
> attribute that returns an image description. I see this as another source of
> alt text, but coming from the image metadata instead of from the markup. But
> I may be missing some use case here.

I think VoiceOver wants to be able to describe and treat this differently if necessary

[chris fleizach]

Created attachment 420485 [details]
Patch

[chris fleizach]

Created attachment 420529 [details]
Patch

[chris fleizach]

Created attachment 420555 [details]
Patch

[chris fleizach]

Created attachment 420688 [details]
Patch

[chris fleizach]

Created attachment 420756 [details]
Patch

[Jer Noble]

Comment on attachment 420756 [details]
Patch

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

r=me with nits.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1596
> +    if (imageAttrs == nil) {

if (!imageAttrs)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1601
> +        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes]);
> +        [tempArray addObject:NSAccessibilityEmbeddedImageDescriptionAttribute];
> +        [tempArray addObject:NSAccessibilityURLAttribute];
> +        imageAttrs = tempArray.leakRef();
> +    }

adoptNS(...).leakRef() seems to be a no-op, so I'm not sure what it's doing here. This applies to all the other initializers in this function as well.

Additionally, it's a little weird that every attribute dictionary is initialized the first time this method is called, rather than have a separate static/NeverDestroyed method for each, so the cost is only paid when at item of the specific type is passed in. But since this is the existing pattern, maybe it's not a big deal.

> Source/WebCore/platform/cf/MediaAccessibilitySoftLink.h:76
> +SOFT_LINK_FUNCTION_FOR_HEADER(WebCore, MediaAccessibility, MAImageCaptioningCopyCaptionWithSource, CFStringRef, (CGImageSourceRef imageSource, CFErrorRef * CF_RETURNS_RETAINED error), (imageSource, error))
> +#define MAImageCaptioningCopyCaptionWithSource softLink_MediaAccessibility_MAImageCaptioningCopyCaptionWithSource

Should this be the _MAY_FAIL variant?

[chris fleizach]

(In reply to Jer Noble from comment #23)
> Comment on attachment 420756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420756&action=review
> 
> r=me with nits.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1596
> > +    if (imageAttrs == nil) {
> 
> if (!imageAttrs)
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1601
> > +        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes]);
> > +        [tempArray addObject:NSAccessibilityEmbeddedImageDescriptionAttribute];
> > +        [tempArray addObject:NSAccessibilityURLAttribute];
> > +        imageAttrs = tempArray.leakRef();
> > +    }
> 
> adoptNS(...).leakRef() seems to be a no-op, so I'm not sure what it's doing
> here. This applies to all the other initializers in this function as well.
> 
> Additionally, it's a little weird that every attribute dictionary is
> initialized the first time this method is called, rather than have a
> separate static/NeverDestroyed method for each, so the cost is only paid
> when at item of the specific type is passed in. But since this is the
> existing pattern, maybe it's not a big deal.
> 

let me make a new bug to address this. the code is like 20 years old so it's probably doing everything in an out of date way now

https://bugs.webkit.org/show_bug.cgi?id=222125

> > Source/WebCore/platform/cf/MediaAccessibilitySoftLink.h:76
> > +SOFT_LINK_FUNCTION_FOR_HEADER(WebCore, MediaAccessibility, MAImageCaptioningCopyCaptionWithSource, CFStringRef, (CGImageSourceRef imageSource, CFErrorRef * CF_RETURNS_RETAINED error), (imageSource, error))
> > +#define MAImageCaptioningCopyCaptionWithSource softLink_MediaAccessibility_MAImageCaptioningCopyCaptionWithSource
> 
> Should this be the _MAY_FAIL variant?

Yea

[chris fleizach]

Created attachment 420870 [details]
Patch

[chris fleizach]

Created attachment 420880 [details]
Patch

[chris fleizach]

Created attachment 421009 [details]
Patch

[EWS]

Committed r273214: <https://commits.webkit.org/r273214>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421009 [details].