Summary: | AX: Image should report the embedded accessibility description if available | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, annulen, apinheiro, changseok, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, keith_miller, kondapallykalyan, mark.lam, msaboff, pdr, philipj, ryuan.choi, saam, samuel_white, sergio, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
chris fleizach
2021-02-14 00:25:47 PST
Created attachment 420238 [details]
patch
Created attachment 420334 [details]
patch
Created attachment 420338 [details]
Patch
Created attachment 420362 [details]
Patch
Created attachment 420365 [details]
Patch
Created attachment 420368 [details]
Patch
Created attachment 420383 [details]
Patch
Created attachment 420393 [details]
Patch
Created attachment 420396 [details]
Patch
(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. Created attachment 420427 [details]
Patch
(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 Created attachment 420433 [details]
Patch
(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. --- 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"); (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 Created attachment 420485 [details]
Patch
Created attachment 420529 [details]
Patch
Created attachment 420555 [details]
Patch
Created attachment 420688 [details]
Patch
Created attachment 420756 [details]
Patch
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? (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 Created attachment 420870 [details]
Patch
Created attachment 420880 [details]
Patch
Created attachment 421009 [details]
Patch
Committed r273214: <https://commits.webkit.org/r273214> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421009 [details]. |