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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 14 11:02:20 PDT 2023


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

--- Comment #11 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to Jack Wiig from comment #9)
> (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.

Spell out in the commit comment those bug fixes done in addition to the notifications.

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

I think there are both cases throughout the codebase. It depends on the usage. For instance, most of the AXIsolatedTree stuff is conditionally compiled, and you have to #if every call site.
>
> > +#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.

Thanks for the clarification.
>
> >
> > --- 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.

These are internal constants to AX Core so they don't have to match the AppKit name, although in some cases it is good to keep a close resemblance. It is up to you, but I would keep AXObjectCache::AXNotification enumerators clear and concise names.
>
> >
> > --- 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.

Then let's call this struct something like AXObjectCharacterRange and possibly derive from CharacterRange, just adding the AX objectID. Alternatively, and maybe preferrably, you can add this data to the AXIsolatedObject property map as an std::pair<AXID, CharacterRange>, or a tuple.
>
> >
> > --- 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.

Yes, let's clean this up and keep just one method.
>
> >
> > --- 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.

I mean that we don't need this whole method, we'll keep the one that returns the TextMarkerRange.
>
> >
> > +    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/06a7a9f8/attachment-0001.htm>


More information about the webkit-unassigned mailing list