Summary: | AX: WebKit: need a method to get visible text and frame of an element on screen | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
chris fleizach
2021-11-18 14:42:16 PST
Created attachment 444744 [details]
patch
Comment on attachment 444744 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=444744&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:491 > + std::optional<BoundaryPoint> endBoundary = range->end; Can these be auto? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:498 > + std::optional<BoundaryPoint> testStartBoundary = makeBoundaryPoint(nextLinePosition); Can this be auto? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:511 > + std::optional<BoundaryPoint> testEndBoundary = makeBoundaryPoint(previousLinePosition); Can this be auto? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:784 > +- (WebCore::AXCoreObject*)initializeBackingsStore Is this a typo (extra s)? initializeBackingsStore vs. initializeBackingStore Comment on attachment 444744 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=444744&action=review >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:491 >> + std::optional<BoundaryPoint> endBoundary = range->end; > > Can these be auto? no it picks the wrong type >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:498 >> + std::optional<BoundaryPoint> testStartBoundary = makeBoundaryPoint(nextLinePosition); > > Can this be auto? ditto >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:784 >> +- (WebCore::AXCoreObject*)initializeBackingsStore > > Is this a typo (extra s)? initializeBackingsStore vs. initializeBackingStore yea true Created attachment 444750 [details]
patch
Created attachment 444751 [details]
patch
Created attachment 444785 [details]
patch
(In reply to chris fleizach from comment #6) > Created attachment 444785 [details] > patch --- a/LayoutTests/accessibility/mac/visible-character-range.html +++ a/LayoutTests/accessibility/mac/visible-character-range.html +<script src="../../resources/js-test-pre.js"></script> Do just +<script src="../../resources/js-test.js"></script> no -pre. +<p id="description"></p> +<div id="console"></div> Remove these two lines, no needed any more. + if (window.accessibilityController) { + + var text = accessibilityController.accessibleElementById("group").childAtIndex(0); looks like indentation is off here, extra spaces. +<script src="../../resources/js-test-post.js"></script> Remove this line, not needed any more. (In reply to chris fleizach from comment #6) > Created attachment 444785 [details] > patch --- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm +++ a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm +#import "Position.h" ... +#import "SimpleRange.h" Doesn't seem right that we have to import Position.h here. +- (CGRect)frameForRange:(NSRange)range +{ + auto webRange = [self convertToDOMRange:range]; + if (!webRange) + return CGRectZero; + auto rect = self.axBackingObject->boundsForRange(*webRange); + return [self convertRectToSpace:rect space:AccessibilityConversionSpace::Screen]; +} Don't we need to check for if (![self _prepareAccessibilityCall]) --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h +++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h +- (WebCore::FloatRect)accessibilityUnobscuredContentRect; Shouldn't this be in the core code instead of in the wrapper? +- (std::optional<WebCore::SimpleRange>)convertToDOMRange:(NSRange)range; ... +extern NSRange convertToNSRange(const WebCore::SimpleRange&); Can we make the first one also a function so that we have a symmetric pair of convert functions? Also consider "make" instead of "convert" for the names since it seems that that's the trend in other conversion functions in WebKit. --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm +++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm +- (FloatRect)accessibilityUnobscuredContentRect +{ + auto document = self.axBackingObject->document(); + if (!document || !document->view()) + return { }; + return FloatRect(snappedIntRect(document->view()->unobscuredContentRect())); +} Should be an AXCoreObject method instead. +- (NSRange)accessibilityVisibleCharacterRange The algorithm implementation of moving by line and intersecting with the given rect should be a core method, and then this wrapper method would do the conversions to NSRange of the result. + NSRange fullRange = convertToNSRange(SimpleRange(*startBoundary, *endBoundary)); Can we use initializer list instead of SimpleRange(...)? +- (WebCore::AXCoreObject*)initializeBackingStore This does more than initializing, it updates. Since we already are using updateObjectBackingStore, perhaps we can call this one just updateBackingStore? or _updateBackingStore? (In reply to chris fleizach from comment #6) > Created attachment 444785 [details] > patch The test should also be enabled on iOS. Created attachment 444872 [details]
patch
Created attachment 444873 [details]
patch
Created attachment 444874 [details]
patch
Created attachment 444885 [details]
patch
Created attachment 444886 [details]
patch
Created attachment 444887 [details]
patch
Created attachment 444889 [details]
patch
Created attachment 444916 [details]
patch
Created attachment 444917 [details]
patch
Created attachment 444918 [details]
patch
Created attachment 444940 [details]
patch
Created attachment 444941 [details]
patch
Created attachment 444942 [details]
patch
(In reply to chris fleizach from comment #22) > Created attachment 444942 [details] > patch --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp +std::optional<SimpleRange> AccessibilityObject::visibleCharacterRange() const +{ ... + return SimpleRange(*startBoundary, *endBoundary); +} I think you may be able to do the more stylish: return {{ *startBoundary, *endBoundary }}; Created attachment 444966 [details]
patch
Committed r286116 (244503@main): <https://commits.webkit.org/244503@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444966 [details]. |