[Webkit-unassigned] [Bug 258228] Post Accessibility notifications during composition contexts indicating start, end, and value change.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=258228

--- Comment #8 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to Jack Wiig from comment #7)
> Created attachment 467039 [details]
> Patch

This patch contains many more changes than what is stated in the commit comments. This should be broken into several patches with more targeted changes. That would make it easier to review and more importantly easier to identify problems down the road. WebKit encourages small, incremental changes.

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

+void AXObjectCache::onTextCompositionChange(Node& node, CompositionState compositionState, bool valueChanged)
 {
+#if HAVE(INLINE_PREDICTIONS)

Shouldn't the whole method be conditionally compiled? I.e.:

+#if HAVE(INLINE_PREDICTIONS)
+void AXObjectCache::onTextCompositionChange(Node& node, CompositionState compositionState, bool valueChanged)
 {
...
}
#endif

The naming also seems off, since we have "inline prediction" and "text composition"to refer to the same thing.


--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h

+        AXTextInputMarkingSessionBegan,
+        AXTextInputMarkingSessionEnded,

Yet another name "text input marking". Can we settle on one name to call this? Either "inline predictions", "text composition" or "text input marking". Or these are different things? In either case, "Session" doesn't seem to contribute any info.


--- a/Source/WebCore/accessibility/AXTextMarker.cpp
+++ b/Source/WebCore/accessibility/AXTextMarker.cpp

+std::optional<CharacterRange> AXTextMarkerRange::characterRange() const
+{
+    if (m_start.m_data.objectID != m_end.m_data.objectID
+        || m_start.m_data.treeID != m_end.m_data.treeID)

Use objectID() and treeID() methods.

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+// This lightweight representation of a text marker range is 16 bytes (instead of 80 bytes), making it far more efficient to store in the isolated object's property map.
+struct AXIsolatedTextMarkerRange {
+    AXID objectID;
+    unsigned start;
+    unsigned end;
+    AXIsolatedTextMarkerRange(const AXTextMarkerRange&);
+};
+#endif

I don't think we need this. If the goal is to store this data in an AXIsolatedObject property map, you don't need to store the objectID again, and hence you can just stor a CharacterRange.

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp


+    if (!range || !range->length)
+        return std::nullopt;

-    return false;
+    return range;

You can write this in 1 line:

+    return range && range->length ? range : std::nullopt;

+AXTextMarkerRange AccessibilityObject::textInputMarkedTextMarkerRange() const
 {
...
+    return { editor.compositionRange() };
 }

So the only purpose of getting the AX object from the editor.compositionNode() is to determine whether it is equal to this? Wouldn't it be better to just compare editor.compositionNode() with this->node()?

--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h

     std::optional<CharacterRange> textInputMarkedRange() const final;
+    AXTextMarkerRange textInputMarkedTextMarkerRange() const final;

Why do we need two methods in this class to do essentially the same thing? I would keep textInputMarkedTextMarkerRange since the other one is just textInputMarkedTextMarkerRange().characterRange().


--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ b/Source/WebCore/accessibility/AccessibilityObjectInterface.h

     virtual std::optional<CharacterRange> textInputMarkedRange() const = 0;
+    virtual AXTextMarkerRange textInputMarkedTextMarkerRange() const = 0;


Even more so if they are exposed in the AXCoreObject interface, we don't want duplication of functionality.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp

+std::optional<CharacterRange> AXIsolatedObject::textInputMarkedRange() const
+{
+    auto range = textInputMarkedTextMarkerRange().characterRange();
+    if (!range || !range->length)
+        return std::nullopt;
+
+    return range;
+}

No needed.

+    auto range = optionalAttributeValue<AXIsolatedTextMarkerRange>(AXPropertyName::TextInputMarkedTextMarkerRange);
+    if (!range || range->start >= range->end)
+        return { };
+
+    return { tree()->treeID(), range->objectID, range->start, range->end };
+}

We should be storing a CharacterRange just for the appropriate IsolatedObject.


--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

-    std::optional<CharacterRange> textInputMarkedRange() const final { return optionalAttributeValue<CharacterRange>(AXPropertyName::TextInputMarkedRange); }
+    std::optional<CharacterRange> textInputMarkedRange() const final;

No needed.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

+        case AXPropertyName::TextInputMarkedTextMarkerRange: {
+            AXIsolatedTextMarkerRange isolatedRange = { axObject.textInputMarkedTextMarkerRange() };
+            propertyMap.set(AXPropertyName::TextInputMarkedTextMarkerRange, isolatedRange);
             break;

Here you can store the CharacterRange if axObject.textInputMarkedTextMarkerRange() returns a non-empty range, or remove otherwise.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

+struct AXIsolatedTextMarkerRange;

No needed.

-using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, PAL::SessionID, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, RetainPtr<NSAttributedString>, InsideLink, Vector<Vector<AXID>>, CharacterRange>;
+using AXPropertyValueVariant = std::variant<std::nullptr_t, AXID, String, bool, int, unsigned, double, float, uint64_t, AccessibilityButtonState, Color, URL, LayoutRect, FloatPoint, FloatRect, IntPoint, IntRect, PAL::SessionID, std::pair<unsigned, unsigned>, Vector<AccessibilityText>, Vector<AXID>, Vector<std::pair<AXID, AXID>>, Vector<String>, Path, OptionSet<AXAncestorFlag>, RetainPtr<NSAttributedString>, InsideLink, Vector<Vector<AXID>>, CharacterRange, AXIsolatedTextMarkerRange>;


Only need the CharacterRange.

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm


+    if ([attributeName isEqualToString:NSAccessibilityTextInputMarkedTextMarkerRangeAttribute]) {
+        if (auto range = backingObject->textInputMarkedTextMarkerRange())
+            return (id)AXTextMarkerRangeRef(range);
+        return nil;
+    }

Return platformData().bridgingAutorelease(), look at the existing instances of returning AXTextMarkerRanges.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20230714/4faa3dd1/attachment.htm>