We do not cache TextInputMarkedTextMarkerRange when isolated objects are initialized. This becomes an issue when an object is initialized while there is already marked text.
<rdar://problem/118866640>
Created attachment 468781 [details] Patch
Comment on attachment 468781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468781&action=review > LayoutTests/accessibility/mac/text-input-marked-range.html:27 > + }) For consistency, maybe we add a semicolon here? > LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:25 > + await waitFor(() => { > + text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange()); > + return text == "test"; > + }); > + output += await expectAsync("text", "'test'"); Can this be shortened to: output += await expectAsync("element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange())", "test"); > LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:32 > + await waitFor(() => { > + text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange()); > + return !text; > + }); > + output += await expectAsync("!text", "true"); Can this be shortened to: output += await expectAsync("!element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange())", "true"); > LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:40 > + textInputController.setMarkedText(" message", 4, " message".length); > + await waitFor(() => { > + text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange()); > + return text == " message"; > + }); > + output += expect("text", "' message'"); Can we use await expectAsync here too? > LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:45 > + }) For consistency, maybe we add a semicolon after this parentheses?
Created attachment 468785 [details] Patch
(In reply to Joshua Hoffman from comment #4) > Created attachment 468785 [details] > Patch diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp index 37c619db01e2..8a278862b697 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb if (descriptor.length()) setProperty(AXPropertyName::ExtendedDescription, descriptor.isolatedCopy()); - if (object.isTextControl()) + if (object.isTextControl()) { setProperty(AXPropertyName::SelectedTextRange, object.selectedTextRange()); + std::pair<AXID, CharacterRange> value; + auto range = object.textInputMarkedTextMarkerRange(); + if (auto characterRange = range.characterRange(); range && characterRange) + value = { range.start().objectID(), *characterRange }; + + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value); AG: do we have to store the empty value for this property or we can apply the sparse property map technique? + } + // These properties are only needed on the AXCoreObject interface due to their use in ATSPI, // so only cache them for ATSPI. #if PLATFORM(ATSPI)
(In reply to Andres Gonzalez from comment #5) > (In reply to Joshua Hoffman from comment #4) > > Created attachment 468785 [details] > > Patch > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > index 37c619db01e2..8a278862b697 100644 > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const > Ref<AccessibilityObject>& axOb > if (descriptor.length()) > setProperty(AXPropertyName::ExtendedDescription, > descriptor.isolatedCopy()); > > - if (object.isTextControl()) > + if (object.isTextControl()) { > setProperty(AXPropertyName::SelectedTextRange, > object.selectedTextRange()); > > + std::pair<AXID, CharacterRange> value; > + auto range = object.textInputMarkedTextMarkerRange(); > + if (auto characterRange = range.characterRange(); range && > characterRange) > + value = { range.start().objectID(), *characterRange }; > + > + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value); > > AG: do we have to store the empty value for this property or we can apply > the sparse property map technique? setProperty handles filtering out default values, so this is already covered!
(In reply to Joshua Hoffman from comment #6) > (In reply to Andres Gonzalez from comment #5) > > (In reply to Joshua Hoffman from comment #4) > > > Created attachment 468785 [details] > > > Patch > > > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > index 37c619db01e2..8a278862b697 100644 > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const > > Ref<AccessibilityObject>& axOb > > if (descriptor.length()) > > setProperty(AXPropertyName::ExtendedDescription, > > descriptor.isolatedCopy()); > > > > - if (object.isTextControl()) > > + if (object.isTextControl()) { > > setProperty(AXPropertyName::SelectedTextRange, > > object.selectedTextRange()); > > > > + std::pair<AXID, CharacterRange> value; > > + auto range = object.textInputMarkedTextMarkerRange(); > > + if (auto characterRange = range.characterRange(); range && > > characterRange) > > + value = { range.start().objectID(), *characterRange }; > > + > > + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value); > > > > AG: do we have to store the empty value for this property or we can apply > > the sparse property map technique? > > setProperty handles filtering out default values, so this is already covered! I think that you can then do: + if (auto characterRange = range.characterRange(); range && characterRange) + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, { range.start().objectID(), *characterRange }); + } and save the copy to a temp.
(In reply to Andres Gonzalez from comment #7) > (In reply to Joshua Hoffman from comment #6) > > (In reply to Andres Gonzalez from comment #5) > > > (In reply to Joshua Hoffman from comment #4) > > > > Created attachment 468785 [details] > > > > Patch > > > > > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > > index 37c619db01e2..8a278862b697 100644 > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp > > > @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const > > > Ref<AccessibilityObject>& axOb > > > if (descriptor.length()) > > > setProperty(AXPropertyName::ExtendedDescription, > > > descriptor.isolatedCopy()); > > > > > > - if (object.isTextControl()) > > > + if (object.isTextControl()) { > > > setProperty(AXPropertyName::SelectedTextRange, > > > object.selectedTextRange()); > > > > > > + std::pair<AXID, CharacterRange> value; > > > + auto range = object.textInputMarkedTextMarkerRange(); > > > + if (auto characterRange = range.characterRange(); range && > > > characterRange) > > > + value = { range.start().objectID(), *characterRange }; > > > + > > > + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value); > > > > > > AG: do we have to store the empty value for this property or we can apply > > > the sparse property map technique? > > > > setProperty handles filtering out default values, so this is already covered! > > I think that you can then do: > > + if (auto characterRange = range.characterRange(); range && > characterRange) > + setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, { > range.start().objectID(), *characterRange }); > + } > > and save the copy to a temp. C++ can't infer that type directly, but I can use the constructor inline and achieve the same result. I will upload with that revision included.
Created attachment 468787 [details] Patch
Committed 271251@main (5965a5a92e80): <https://commits.webkit.org/271251@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468787 [details].