RESOLVED FIXED 258228
Post Accessibility notifications during composition contexts indicating start, end, and value change. 
https://bugs.webkit.org/show_bug.cgi?id=258228
Summary Post Accessibility notifications during composition contexts indicating start...
Jack Wiig
Reported 2023-06-16 17:03:18 PDT
For performance and consistency reasons, conveying annotations in text notifications will provide assistive technologies with all of the information about the change, so that they won't need to request data. Adding a composition type helps disambiguate normal text replacements from input method event compositions.
Attachments
Patch (57.14 KB, patch)
2023-06-30 13:47 PDT, Jack Wiig 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (57.25 KB, patch)
2023-06-30 14:40 PDT, Jack Wiig 		no flags
DetailsFormatted DiffDiff
Patch (58.28 KB, patch)
2023-07-13 17:52 PDT, Jack Wiig 		no flags
DetailsFormatted DiffDiff
Patch (58.92 KB, patch)
2023-07-13 18:50 PDT, Jack Wiig 		no flags
DetailsFormatted DiffDiff
Patch (58.86 KB, patch)
2023-07-14 10:35 PDT, Jack Wiig 		no flags
DetailsFormatted DiffDiff
Patch (58.04 KB, patch)
2023-07-14 15:10 PDT, Jack Wiig 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2023-06-16 17:03:28 PDT
<rdar://problem/110925384>
Jack Wiig
Comment 2 2023-06-30 13:40:34 PDT
To align ourselves with AppKit, WebKit should post a notification when a composition starts so that assistive technologies know to process any future value changes for that ui element as incomplete composition changes. When a composition ends, post a notification to indicate that the composition session has ended. This model does not convey any information in the text notifications (unlike what my past comment indicates) and posts the notification from the text control containing the text information. This aligns us closer with how other UI frameworks behave.
Jack Wiig
Comment 3 2023-06-30 13:47:58 PDT
Created attachment 466888 [details] Patch
Jack Wiig
Comment 4 2023-06-30 14:40:57 PDT
Created attachment 466889 [details] Patch
Tyler Wilcock
Comment 5 2023-07-01 16:28:09 PDT
Comment on attachment 466889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466889&action=review > Source/WebCore/accessibility/AXTextMarker.h:179 > +class AXIsolatedTextMarkerRange { This feels more like a struct than a class. Do you agree, or is there some reason you made it a class? > LayoutTests/accessibility/mac/text-input-session-notifications.html:7 > +<body> > + <input id="editable" type="text"> Everything within the body tag can be indented at the same level as the body itself. > LayoutTests/accessibility/mac/text-input-session-notifications.html:9 > + <script type="text/javascript"> > + description("This test ensures that the input method marked range is available to accessibility clients as text marker range."); Everything within this script tag can be indented at the same level as the script tag itself. > LayoutTests/accessibility/mac/text-input-session-notifications.html:39 > + var element = accessibilityController.accessibleElementById("editable"); > + > + element.addNotificationListener(notificationCallback); Can probably shorten these two lines to be: accessibilityController.accessibleElementById("editable").addNotificationListener(notificationCallback);
Jack Wiig
Comment 6 2023-07-13 17:52:53 PDT
Created attachment 467038 [details] Patch
Jack Wiig
Comment 7 2023-07-13 18:50:36 PDT
Created attachment 467039 [details] Patch
Andres Gonzalez
Comment 8 2023-07-14 08:34:07 PDT
(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.
Jack Wiig
Comment 9 2023-07-14 09:26:36 PDT
(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.
Jack Wiig
Comment 10 2023-07-14 10:35:50 PDT
Created attachment 467043 [details] Patch
Andres Gonzalez
Comment 11 2023-07-14 11:02:20 PDT
(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.
Jack Wiig
Comment 12 2023-07-14 15:10:34 PDT
Created attachment 467045 [details] Patch
Jack Wiig
Comment 13 2023-07-14 15:17:11 PDT
(In reply to Andres Gonzalez from comment #11) > (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. > Okay, I added the other small changes. Sorry about not doing this in the first place, I should have at least done that from the start. > > > > > > > > --- 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. Oh, that's a good point! Do you have a preference? I'll probably keep it as is unless you have a strong preference. > > > > > > > > --- 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. I renamed the AXObjectCache::AXNotification enum cases to AXTextCompositionBegan and AXTextCompositionEnded to keep them more succinct, but left the ones in the Mac wrapper file alone to match AppKit. I agree, these are much better now. > > > > > > > > --- 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. Thanks for the suggestion! I changed it out to a std::pair<AXID, CharacterRange> and think that it went pretty well. > > > > > > > > --- 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. Thanks for pushing me on this; I think that it was the right move. I removed the CharacterRange based method.
EWS
Comment 14 2023-07-18 10:46:42 PDT
Committed 266133@main (0b3aac159d97): <https://commits.webkit.org/266133@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 467045 [details].
EWS
Comment 15 2024-05-28 22:33:14 PDT
Test gardening commit 279429@main (2e01b0865746): <https://commits.webkit.org/279429@main> Reviewed commits have been landed. Closing PR #29211 and removing active labels.
EWS
Comment 16 2024-06-02 21:14:01 PDT
Test gardening commit 279631@main (5290e0747dc3): <https://commits.webkit.org/279631@main> Reviewed commits have been landed. Closing PR #29440 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.