Skip to content

Commit

Permalink
AX: Move AXSelectedTextMarkerRange off of the main thread
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=262433
rdar://116128660

Reviewed by Chris Fleizach.

This patch moves the AXSelectedTextMarkerRange attribute off of the main thread in
isolated tree mode, by caching the relevant text marker data.

When selection changes on the web, the AXObjectCache is notified through a call to
postTextStateChangeNotification from FrameSelection::notifyAccessibilityForSelectionChange.
From there, a new method, onSelectedTextChanged, handles updating the isolated tree's
m_selectedTextMarkerRange with the relevant text markers for the start and end of the
selection. These are stored in a pair of TextMarkerData structs.

An important piece of this is that we need the nodes of the start and end text markers
to exist in the isolated tree. So, if one or both of these nodes are ignored, they are added
to the isolated tree as unconnected nodes. This is handled in onSelectedTextChanged.

Finally, when the range is requested via AXIsolatedObject::selectedTextMarkerRange(),
an AXTextMarkerRange is constructed from the cached TextMarkerData pair.

This patch also modifies previous tests to make them asynchronous for this new behavior.

* LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash-expected.txt:
* LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash.html:
* LayoutTests/accessibility/mac/replace-text-with-range-expected.txt:
* LayoutTests/accessibility/mac/replace-text-with-range-value-change-notification.html:
* LayoutTests/accessibility/mac/replace-text-with-range.html:
* LayoutTests/accessibility/mac/text-marker-p-tags-expected.txt:
* LayoutTests/accessibility/mac/text-marker-p-tags.html:
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::postTextStateChangeNotification):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::selectedTextMarkerRange):
* Source/WebCore/accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::selectedVisiblePositionRange const):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::selectedTextMarkerRange):
(WebCore::AXIsolatedObject::selectedVisiblePositionRange const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:
(WebCore::AXIsolatedTree::selectedTextMarkerRange):
(WebCore::AXIsolatedTree::setSelectedTextMarkerRange):
* Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:
(WebCore::AXObjectCache::onSelectedTextChanged):
* Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
(-[WebAccessibilityObjectWrapper selectedTextMarkerRange]):

Canonical link: https://commits.webkit.org/268821@main
  • Loading branch information
@hoffmanjoshua @fleizach
hoffmanjoshua authored and fleizach committed Oct 4, 2023
1 parent f399a5e commit a1746d7
Show file tree
Hide file tree
Showing 18 changed files with 224 additions and 140 deletions.
9 changes: 2 additions & 7 deletions LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash-expected.txt
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
text
This tests that creating a text marker range with a text marker that associated to a doctype node won't lead to crash.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


This tests that creating a text marker range with a text marker that is associated to a doctype node won't lead to crash.
PASS successfullyParsed is true

TEST COMPLETE

text
95 changes: 51 additions & 44 deletions LayoutTests/accessibility/mac/doctype-node-in-text-marker-crash.html
View file
Original file line number Diff line number Diff line change
@@ -1,59 +1,66 @@
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="../../resources/js-test-pre.js"></script>
<script src="../../resources/accessibility-helper.js"></script>
<script src="../../resources/js-test.js"></script>
</head>
<body id="body">

<div id="container" role="group" contenteditablt="true">text<iframe id="frame" marginwidth="0" marginheight="0" frameborder="0" scrolling="no" height="100%" width="100%" style="padding: 0px; margin: 0px; border: 0px; height: 100%; width: 100%;"></iframe></div>

<p id="description"></p>
<div id="console"></div>

<script>
var result = "This tests that creating a text marker range with a text marker that is associated to a doctype node won't lead to crash.";

function selectElementContents(el) {
var range = document.createRange();
range.selectNodeContents(el);
var sel = window.getSelection();
sel.removeAllRanges();
sel.addRange(range);
}

if (window.accessibilityController) {
window.jsTestIsAsync = true;

accessibilityController.enableEnhancedAccessibility(true);
var webArea = accessibilityController.rootElement.childAtIndex(0);
webArea.setBoolAttributeValue("AXCaretBrowsingEnabled", true);

// Make sure the iframe has a doctype node
var iframe = document.getElementById("frame");
var idocument = iframe.contentDocument;
idocument.open();
idocument.write("<!DOCTYPE html>");
idocument.write("<html>");
idocument.write("<head></head>");
idocument.write("<body></body>");
idocument.write("</html>");
idocument.close();

// Select all the contents in the container, and get the selection range.
var container = accessibilityController.accessibleElementById("container");
var containerElement = document.getElementById("container");
selectElementContents(containerElement);

setTimeout(async function() {
selectionRange = null;
var startMarker = null;
var endMarker = null;
await waitFor(() => {
selectionRange = container.selectedTextMarkerRange();
startMarker = container.startTextMarkerForTextMarkerRange(selectionRange);
endMarker = container.endTextMarkerForTextMarkerRange(selectionRange);
return startMarker != null && endMarker != null;
})

description("This tests that creating a text marker range with a text marker that associated to a doctype node won't lead to crash.");

function selectElementContents(el) {
var range = document.createRange();
range.selectNodeContents(el);
var sel = window.getSelection();
sel.removeAllRanges();
sel.addRange(range);
}

if (window.accessibilityController) {

accessibilityController.enableEnhancedAccessibility(true);
var webArea = accessibilityController.rootElement.childAtIndex(0);
webArea.setBoolAttributeValue("AXCaretBrowsingEnabled", true);

// Make sure the iframe has a doctype node
var iframe = document.getElementById("frame");
var idocument = iframe.contentDocument;
idocument.open();
idocument.write("<!DOCTYPE html>");
idocument.write("<html>");
idocument.write("<head></head>");
idocument.write("<body></body>");
idocument.write("</html>");
idocument.close();

// Select all the contents in the container, and get the selection range.
var container = accessibilityController.accessibleElementById("container");
var containerElement = document.getElementById("container");
selectElementContents(containerElement);
var selectionRange = container.selectedTextMarkerRange();

var startMarker = container.startTextMarkerForTextMarkerRange(selectionRange);
var endMarker = container.endTextMarkerForTextMarkerRange(selectionRange);

// endMarker is now associated with a doctype node, make sure it won't lead to a crash.
var markerRange = container.textMarkerRangeForMarkers(endMarker, startMarker);
}
// endMarker is now associated with a doctype node, make sure it won't lead to a crash.
var markerRange = container.textMarkerRangeForMarkers(endMarker, startMarker);
debug(result);
finishJSTest();
}, 0);

}
</script>

<script src="../../resources/js-test-post.js"></script>
</body>
</html>
18 changes: 8 additions & 10 deletions LayoutTests/accessibility/mac/replace-text-with-range-expected.txt
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
This tests that the replace with range API functions as expected.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS: contenteditable.replaceTextInRange("Blurb", 5, 5) === true
PASS: contenteditable.stringValue === 'AXValue: HelloBlurb'
PASS: contenteditable.replaceTextInRange("Blorg", 5, 5) === true
PASS: contenteditable.stringValue === 'AXValue: HelloBlorg'
PASS: text.replaceTextInRange("blurb", 6, 5) === true
PASS: text.stringValue === 'AXValue: Hello blurb'
PASS: textarea.replaceTextInRange("blurb", 6, 5) === true
PASS: textarea.stringValue === 'AXValue: Hello blurb'


PASS contenteditable.replaceTextInRange("Blurb", 5, 5) is true
PASS contenteditable.stringValue is 'AXValue: HelloBlurb'
PASS contenteditable.replaceTextInRange("Blorg", 5, 5) is true
PASS contenteditable.stringValue is 'AXValue: HelloBlorg'
PASS text.replaceTextInRange("blurb", 6, 5) is true
PASS text.stringValue is 'AXValue: Hello blurb'
PASS textarea.replaceTextInRange("blurb", 6, 5) is true
PASS textarea.stringValue is 'AXValue: Hello blurb'
PASS successfullyParsed is true

TEST COMPLETE
Expand Down
59 changes: 37 additions & 22 deletions LayoutTests/accessibility/mac/replace-text-with-range-value-change-notification.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<script src="../../resources/js-test.js"></script>
<script src="../../editing/editing.js"></script>
<script src="value-change/value-change-helpers.js"></script>
<script src="../../resources/accessibility-helper.js"></script>
</head>
<body id="body">

Expand All @@ -30,6 +31,8 @@

var expectedValues = [];

var finished = false;

function notificationCallback(notification, userInfo) {
if (notification == "AXValueChanged") {
actualChangeTypes[count] = userInfo["AXTextStateChangeType"];
Expand Down Expand Up @@ -57,7 +60,7 @@
shouldBeInsertReplace("Cat", "Dog");

webArea.removeNotificationListener();
finishJSTest();
finished = true;
}
}
}
Expand All @@ -71,35 +74,47 @@

var addedNotification = webArea.addNotificationListener(notificationCallback);
shouldBe("addedNotification", "true");

var axContentEditableDiv = accessibilityController.accessibleElementById("content-editable-div");
var contentEditableDiv = document.getElementById("content-editable-div");
contentEditableDiv.focus();
shouldBeTrue("axContentEditableDiv.replaceTextInRange('Apple', 0, 0)");
shouldBe("axContentEditableDiv.stringValue", "'AXValue: Apple'");
shouldBeTrue("axContentEditableDiv.replaceTextInRange('Pie', 0, 5)");
shouldBe("axContentEditableDiv.stringValue", "'AXValue: Pie'");
contentEditableDiv.blur();

var axText = accessibilityController.accessibleElementById("text");
var text = document.getElementById("text");
text.focus();
shouldBeTrue("axText.replaceTextInRange('Banana', 0, 0)");
shouldBe("axText.stringValue", "'AXValue: Banana'");
shouldBeTrue("axText.replaceTextInRange('Ice-cream', 0, 6)");
shouldBe("axText.stringValue", "'AXValue: Ice-cream'");
text.blur();

var axTextarea = accessibilityController.accessibleElementById("textarea");
var textarea = document.getElementById("textarea");
textarea.focus();
shouldBeTrue("axTextarea.replaceTextInRange('Cat', 0, 0)");
shouldBe("axTextarea.stringValue", "'AXValue: Cat'");
shouldBeTrue("axTextarea.replaceTextInRange('Dog', 0, 3)");
shouldBe("axTextarea.stringValue", "'AXValue: Dog'");
textarea.blur();

document.getElementById("content").style.visibility = "hidden";

setTimeout(async function() {
contentEditableDiv.focus();
shouldBeTrue("axContentEditableDiv.replaceTextInRange('Apple', 0, 0)");
await waitFor(() => axContentEditableDiv.stringValue == "AXValue: Apple");
shouldBe("axContentEditableDiv.stringValue", "'AXValue: Apple'");
shouldBeTrue("axContentEditableDiv.replaceTextInRange('Pie', 0, 5)");
await waitFor(() => axContentEditableDiv.stringValue == "AXValue: Pie");
shouldBe("axContentEditableDiv.stringValue", "'AXValue: Pie'");
contentEditableDiv.blur();

text.focus();
shouldBeTrue("axText.replaceTextInRange('Banana', 0, 0)");
await waitFor(() => axText.stringValue == "AXValue: Banana");
shouldBe("axText.stringValue", "'AXValue: Banana'");
shouldBeTrue("axText.replaceTextInRange('Ice-cream', 0, 6)");
await waitFor(() => axText.stringValue == "AXValue: Ice-cream");
shouldBe("axText.stringValue", "'AXValue: Ice-cream'");
text.blur();

textarea.focus();
shouldBeTrue("axTextarea.replaceTextInRange('Cat', 0, 0)");
await waitFor(() => axTextarea.stringValue == "AXValue: Cat");
shouldBe("axTextarea.stringValue", "'AXValue: Cat'");
shouldBeTrue("axTextarea.replaceTextInRange('Dog', 0, 3)");
await waitFor(() => axTextarea.stringValue == "AXValue: Dog");
shouldBe("axTextarea.stringValue", "'AXValue: Dog'");
textarea.blur();

document.getElementById("content").style.visibility = "hidden";
await waitFor(() => finished);
finishJSTest();
}, 0);
}
</script>

Expand Down
42 changes: 25 additions & 17 deletions LayoutTests/accessibility/mac/replace-text-with-range.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<html>
<head>
<script src="../../resources/js-test.js"></script>
<script src="../../resources/accessibility-helper.js"></script>
</head>
<body id="body">
<div id="content">
Expand All @@ -11,34 +12,41 @@
<textarea id="textarea">Hello world</textarea>
</div>

<p id="description"></p>
<div id="console"></div>

<script>
description("This tests that the replace with range API functions as expected.");
var result = "This tests that the replace with range API functions as expected.\n\n";

if (window.accessibilityController) {
window.jsTestIsAsync = true;
var contenteditable = accessibilityController.accessibleElementById("contenteditable");
var text = accessibilityController.accessibleElementById("text");
var textarea = accessibilityController.accessibleElementById("textarea");

if (window.accessibilityController) {
var contenteditable = accessibilityController.accessibleElementById("contenteditable");
shouldBeTrue('contenteditable.replaceTextInRange("Blurb", 5, 5)');
shouldBe("contenteditable.stringValue", "'AXValue: HelloBlurb'");
setTimeout(async function() {
result += expect('contenteditable.replaceTextInRange("Blurb", 5, 5)', "true");
await waitFor(() => contenteditable.stringValue == "AXValue: HelloBlurb");
result += expect("contenteditable.stringValue", "'AXValue: HelloBlurb'");

// now test what happens when a control has focus
document.getElementById("contenteditable").focus();
shouldBeTrue('contenteditable.replaceTextInRange("Blorg", 5, 5)');
shouldBe("contenteditable.stringValue", "'AXValue: HelloBlorg'");
result += expect('contenteditable.replaceTextInRange("Blorg", 5, 5)', "true");
await waitFor(() => contenteditable.stringValue == "AXValue: HelloBlorg");
result += expect("contenteditable.stringValue", "'AXValue: HelloBlorg'");
document.getElementById("contenteditable").blur();

var text = accessibilityController.accessibleElementById("text");
shouldBeTrue('text.replaceTextInRange("blurb", 6, 5)');
shouldBe("text.stringValue", "'AXValue: Hello blurb'");
result += expect('text.replaceTextInRange("blurb", 6, 5)', "true");
await waitFor(() => text.stringValue == "AXValue: Hello blurb");
result += expect("text.stringValue", "'AXValue: Hello blurb'");

var textarea = accessibilityController.accessibleElementById("textarea");
shouldBeTrue('textarea.replaceTextInRange("blurb", 6, 5)');
shouldBe("textarea.stringValue", "'AXValue: Hello blurb'");
result += expect('textarea.replaceTextInRange("blurb", 6, 5)', "true");
await waitFor(() => textarea.stringValue == "AXValue: Hello blurb");
result += expect("textarea.stringValue", "'AXValue: Hello blurb'");

document.getElementById("content").style.visibility = "hidden";
}

debug(result);
finishJSTest();
}, 0);
}
</script>

</body>
Expand Down
6 changes: 2 additions & 4 deletions LayoutTests/accessibility/mac/text-marker-p-tags-expected.txt
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
This tests that previous sentence/word/paragraph text marker calls work with p tag elements

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS selectedString is 'Where'
selectedString is Where
Previous sentence: Test sentence two


Expand All @@ -13,6 +10,7 @@ Previous word: two
Previous paragraph: Test sentence one. Test sentence two



PASS successfullyParsed is true

TEST COMPLETE
Expand Down
Loading

0 comments on commit a1746d7

Please sign in to comment.