Bug 221875 - AX: Image should report the embedded accessibility description if available
Summary: AX: Image should report the embedded accessibility description if available
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-14 00:25 PST by chris fleizach
Modified: 2021-02-20 20:16 PST (History)
26 users (show)

See Also:


Attachments
patch (53.50 KB, patch)
2021-02-14 01:43 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (53.50 KB, patch)
2021-02-15 10:53 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (54.75 KB, patch)
2021-02-15 11:08 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (55.49 KB, patch)
2021-02-15 13:32 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (55.50 KB, patch)
2021-02-15 13:44 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (55.71 KB, patch)
2021-02-15 13:59 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.67 KB, patch)
2021-02-15 15:04 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.67 KB, patch)
2021-02-15 16:37 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.74 KB, patch)
2021-02-15 17:07 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (57.21 KB, patch)
2021-02-15 23:43 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (56.61 KB, patch)
2021-02-16 00:54 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.77 KB, patch)
2021-02-16 10:00 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (56.83 KB, patch)
2021-02-16 13:00 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.80 KB, patch)
2021-02-16 15:53 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (56.69 KB, patch)
2021-02-17 11:52 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (56.55 KB, patch)
2021-02-17 16:28 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (57.56 KB, patch)
2021-02-18 14:17 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (57.70 KB, patch)
2021-02-18 15:53 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (57.59 KB, patch)
2021-02-19 12:28 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2021-02-14 00:25:47 PST
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
Comment 1 Radar WebKit Bug Importer 2021-02-14 00:25:55 PST
<rdar://problem/74320584>
Comment 2 chris fleizach 2021-02-14 01:43:26 PST
Created attachment 420238 [details]
patch
Comment 3 chris fleizach 2021-02-15 10:53:47 PST
Created attachment 420334 [details]
patch
Comment 4 chris fleizach 2021-02-15 11:08:34 PST
Created attachment 420338 [details]
Patch
Comment 5 chris fleizach 2021-02-15 13:32:35 PST
Created attachment 420362 [details]
Patch
Comment 6 chris fleizach 2021-02-15 13:44:22 PST
Created attachment 420365 [details]
Patch
Comment 7 chris fleizach 2021-02-15 13:59:20 PST
Created attachment 420368 [details]
Patch
Comment 8 chris fleizach 2021-02-15 15:04:10 PST
Created attachment 420383 [details]
Patch
Comment 9 chris fleizach 2021-02-15 16:37:55 PST
Created attachment 420393 [details]
Patch
Comment 10 chris fleizach 2021-02-15 17:07:58 PST
Created attachment 420396 [details]
Patch
Comment 11 Andres Gonzalez 2021-02-15 18:58:46 PST
(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.
Comment 12 chris fleizach 2021-02-15 23:43:01 PST
Created attachment 420427 [details]
Patch
Comment 13 chris fleizach 2021-02-15 23:44:10 PST
(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
Comment 14 chris fleizach 2021-02-16 00:54:56 PST
Created attachment 420433 [details]
Patch
Comment 15 Andres Gonzalez 2021-02-16 05:09:55 PST
(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.
Comment 16 Andres Gonzalez 2021-02-16 05:15:06 PST
--- 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");
Comment 17 chris fleizach 2021-02-16 09:39:53 PST
(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
Comment 18 chris fleizach 2021-02-16 10:00:41 PST
Created attachment 420485 [details]
Patch
Comment 19 chris fleizach 2021-02-16 13:00:45 PST
Created attachment 420529 [details]
Patch
Comment 20 chris fleizach 2021-02-16 15:53:52 PST
Created attachment 420555 [details]
Patch
Comment 21 chris fleizach 2021-02-17 11:52:26 PST
Created attachment 420688 [details]
Patch
Comment 22 chris fleizach 2021-02-17 16:28:29 PST
Created attachment 420756 [details]
Patch
Comment 23 Jer Noble 2021-02-18 11:01:13 PST
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?
Comment 24 chris fleizach 2021-02-18 12:32:36 PST
(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
Comment 25 chris fleizach 2021-02-18 14:17:38 PST
Created attachment 420870 [details]
Patch
Comment 26 chris fleizach 2021-02-18 15:53:00 PST
Created attachment 420880 [details]
Patch
Comment 27 chris fleizach 2021-02-19 12:28:51 PST
Created attachment 421009 [details]
Patch
Comment 28 EWS 2021-02-20 20:15:57 PST
Committed r273214: <https://commits.webkit.org/r273214>

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