Bug 233336 – AX: WebKit: need a method to get visible text and frame of an element on screen

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

[chris fleizach]

We can determine what elements are on screen, but there is no good way to determine if the content in an element is on screen. If an element is only partially visible, we need to know what the visible content is, and what the accessibilityFrame is of that content.

This is to support Books 

<rdar://problem/83657171>

[chris fleizach]

Created attachment 444744 [details]
patch

[Tyler Wilcock]

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

[chris fleizach]

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

[chris fleizach]

Created attachment 444750 [details]
patch

[chris fleizach]

Created attachment 444751 [details]
patch

[chris fleizach]

Created attachment 444785 [details]
patch

[Andres Gonzalez]

(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.

[Andres Gonzalez]

(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?

[Andres Gonzalez]

(In reply to chris fleizach from comment #6)
> Created attachment 444785 [details]
> patch

The test should also be enabled on iOS.

[chris fleizach]

Created attachment 444872 [details]
patch

[chris fleizach]

Created attachment 444873 [details]
patch

[chris fleizach]

Created attachment 444874 [details]
patch

[chris fleizach]

Created attachment 444885 [details]
patch

[chris fleizach]

Created attachment 444886 [details]
patch

[chris fleizach]

Created attachment 444887 [details]
patch

[chris fleizach]

Created attachment 444889 [details]
patch

[chris fleizach]

Created attachment 444916 [details]
patch

[chris fleizach]

Created attachment 444917 [details]
patch

[chris fleizach]

Created attachment 444918 [details]
patch

[chris fleizach]

Created attachment 444940 [details]
patch

[chris fleizach]

Created attachment 444941 [details]
patch

[chris fleizach]

Created attachment 444942 [details]
patch

[Andres Gonzalez]

(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 }};

[chris fleizach]

Created attachment 444966 [details]
patch

[EWS]

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].