Preliminary accessibility work to support image text extraction.
<rdar://problem/76348740>
Created attachment 425396 [details] Patch
Created attachment 425405 [details] Patch
The content of attachment 425396 [details] has been deleted
The content of attachment 425405 [details] has been deleted
Created attachment 425752 [details] Patch
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
(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.
Created attachment 426105 [details] Patch
Created attachment 426147 [details] Patch
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.
Created attachment 426170 [details] Patch
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
Created attachment 429088 [details] Patch
(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.
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
Created attachment 431742 [details] Patch
Created attachment 431774 [details] Patch Rebasing on master.
Created attachment 431776 [details] Patch Build fix for non-Cocoa platforms.
Created attachment 431862 [details] Patch
Created attachment 431878 [details] Patch Fixes accessibility/image-link.html since imageOverlayElements returns nil instead of a 0-element array.
Created attachment 431982 [details] Patch Uses new format for internals.installImageOverlay.
Created attachment 432045 [details] Patch Skip test on iOS 14.
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].