[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 #9 from Jack Wiig <jhwiig at gmail.com> ---
(In reply to Andres Gonzalez from comment #8)
> (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.

Yeah, this patch grew from being just notifications to fixing up a few bugs that affected this functionality as well. I agree that if I had more time, I should break it up into more changes, but I don't think that it's currently the best use of my time to do so. Looking forward, I'll try and keep this in mind.

> 
> --- 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 I conditionally compile the method, then I have to conditionally compile the call sites too :( which would be more cumbersome in my opinion. I was trying to follow existing practice here.

> +#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.
> 

I remained consistent with the naming used in AppKit and across our different systems. "Inline Predictions" are a subset of text compositions, which also includes input method events (typing in CJK languages). Text composition is probably the best description of this behavior.

As for the macro gate, that is there because I don't want to break past versions of VoiceOver, and this work was done to support inline predictions specifically. So, the macro define and function names are different, but consistent with each other.

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

These are the AppKit names for the notifications, which I am using for consistency. They may not have the best naming, but we need to use the same ones.

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

I just moved this implementation from Mac specific to all-platform so that I could use it in AXIsolatedObject.cpp. I don't know if I'd feel comfortable changing its implementation since I didn't write any tests to protect that.

> +#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.

We have to store the text marker range on the object that posts the notification, which won't have the same objectID() as the one we need for the text marker range to be correct. We do this for consistency with AppKit & because we don't often deal with the static text, but the form control / text field in VoiceOver.

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

Thanks, will change that.

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

This check is not always sufficient because we are often posting and storing this information a bit higher up the tree, on the element that posts the notification, which is consistent with AppKit. Although I initially tried that, it did not work in Safari contexts.

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

I originally implemented the character range method first, and now it's being used throughout the test harness and for a test. I didn't think it would be worth going back and deleting that stuff.

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

I don't want this returning {0, 0} if it's empty, which it was, so I put it into a wrapper that handles this. I think that unless we can resolve that, it's better to have a method for this than it is to handle it at each usage.

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

See my above comments.

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

See my above comments.

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

See my above comments.

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

Thanks! I'll make that change.

-- 
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/513280bf/attachment-0001.htm>