Bug 224280 – Accessibility support for image text recognition.

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

[Andres Gonzalez]

Preliminary accessibility work to support image text extraction.

[Radar WebKit Bug Importer]

<rdar://problem/76348740>

[Andres Gonzalez]

Created attachment 425396 [details]
Patch

[Andres Gonzalez]

Created attachment 425405 [details]
Patch

[David Kilzer (:ddkilzer)]

The content of attachment 425396 [details] has been deleted

[David Kilzer (:ddkilzer)]

The content of attachment 425405 [details] has been deleted

[Andres Gonzalez]

Created attachment 425752 [details]
Patch

[chris fleizach]

Comment on attachment 425752 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:469
> +    bool isAXImageInstance() const override;

we should probably call this

isAccessibilityImageInstance() to match the other methods

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> Comment on attachment 425752 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425752&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:469
> > +    bool isAXImageInstance() const override;
> 
> we should probably call this
> 
> isAccessibilityImageInstance() to match the other methods

I was following the convention isClassNameInstance, and the class name AXImage, similar to isAXIsolatedObjectInstance. I very much prefer the AX prefix for class names instead of the whole Accessibility, and plan to do a gradual renaming. But I can be persuaded if you feel strongly otherwise.

[Andres Gonzalez]

Created attachment 426105 [details]
Patch

[Andres Gonzalez]

Created attachment 426147 [details]
Patch

[Sam Weinig]

Comment on attachment 426147 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Preliminary accessibility work

Please provide a more clear title.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please include the reason no tests are being included.

[Andres Gonzalez]

Created attachment 426170 [details]
Patch

[chris fleizach]

Comment on attachment 426170 [details]
Patch

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

> Source/WebCore/accessibility/AXImage.cpp:62
> +    if (m_imageOverlayHost)

should this be !m_imageoverlayHost?
and second, do we want to return an empty array here in that case?

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:436
> +- (NSArray *)accessibilityElements
> +{

I don't think we should add this. this is actually more expensive, because to just know how many children there are, we have to create the NSArray and return it, rather than just returning the size of the array in elementCount

I also think that if we find that axElementAtIndex and friends are implemented we prefer that over accessibilityElements.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1863
> +    auto imageOverlayElements = self.axBackingObject->imageOverlayElements();

I feel like if the count is empty here, we should return nil. it will be a more efficient operation to return nil (which will be the common case(

> Source/WebCore/platform/Logging.cpp:93
> +    LogAccessibility.state = WTFLogChannelState::On;

this should probably be off right

> LayoutTests/ChangeLog:8
> +        * accessibility/image-link-expected.txt:

we'll probably need to skip these on the non supported platforms

[Andres Gonzalez]

Created attachment 429088 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #13)
> Comment on attachment 426170 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426170&action=review
> 
> > Source/WebCore/accessibility/AXImage.cpp:62
> > +    if (m_imageOverlayHost)
> 
> should this be !m_imageoverlayHost?
> and second, do we want to return an empty array here in that case?

The purpose of the block should be clearer in this revision. The return value of the function is now an Optional.

> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:436
> > +- (NSArray *)accessibilityElements
> > +{
> 
> I don't think we should add this. this is actually more expensive, because
> to just know how many children there are, we have to create the NSArray and
> return it, rather than just returning the size of the array in elementCount
> 
> I also think that if we find that axElementAtIndex and friends are
> implemented we prefer that over accessibilityElements.

I removed the comment saying that this is preferable. I'd like to keep the method if possible cause it makes writing scripts clearer. As for what is more efficient, I'm not convinced that neither one is preferable. I believe the critical path is in retrieving the CoreObject children, and in the by index methods that has to happen multiple times to just iterate over the array. In any case, this is not taking away the by index/count methods.

> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1863
> > +    auto imageOverlayElements = self.axBackingObject->imageOverlayElements();
> 
> I feel like if the count is empty here, we should return nil. it will be a
> more efficient operation to return nil (which will be the common case(

Fixed with imageOverlayElements() returning an Optional.

> 
> > Source/WebCore/platform/Logging.cpp:93
> > +    LogAccessibility.state = WTFLogChannelState::On;
> 
> this should probably be off right

Yes.

> 
> > LayoutTests/ChangeLog:8
> > +        * accessibility/image-link-expected.txt:
> 
> we'll probably need to skip these on the non supported platforms

Yes.

[chris fleizach]

Comment on attachment 429088 [details]
Patch

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

> Source/WebCore/accessibility/AXImage.cpp:68
> +        auto* axImage = axObjectCache->getOrCreate(m_imageOverlayHost.get());

this can be done in one line

> Source/WebCore/accessibility/AXImage.cpp:79
> +        WTFLogAlways("aqui estoy %p %p", this, imageOverlayHost.get());

remove this log

> Source/WebCore/accessibility/AXImage.cpp:82
> +        // FIXME: post notification.

what notification do we need here?

> Source/WebCore/accessibility/AXImage.cpp:86
> +    return children(); // FIXME: once notification is added, this should be return WTF::nullopt;

we need to return the image overlay again right?

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:433
> +- (NSArray *)accessibilityElements

is this necessary to implement?  isn't this handled by accessibilityElementAtIndex?

> Source/WebCore/platform/Logging.cpp:93
> +    LogAccessibility.state = WTFLogChannelState::On;

undo

> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:471
> +    WTFLogAlways("PageClientImpl::requestImageExtraction {");

probably remove these logs in this method

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7348
> +    WTFLogAlways("WebPage::requestImageExtraction {");

remove logging

[Andres Gonzalez]

Created attachment 431742 [details]
Patch

[Andres Gonzalez]

Created attachment 431774 [details]
Patch

Rebasing on master.

[Andres Gonzalez]

Created attachment 431776 [details]
Patch

Build fix for non-Cocoa platforms.

[Andres Gonzalez]

Created attachment 431862 [details]
Patch

[Andres Gonzalez]

Created attachment 431878 [details]
Patch

Fixes accessibility/image-link.html since imageOverlayElements returns nil instead of a 0-element array.

[Andres Gonzalez]

Created attachment 431982 [details]
Patch

Uses new format for internals.installImageOverlay.

[Andres Gonzalez]

Created attachment 432045 [details]
Patch

Skip test on iOS 14.

[EWS]

Committed r279171 (239068@main): <https://commits.webkit.org/239068@main>

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