From 455f71d2fda82bf70f2522f3573162286f09c62b Mon Sep 17 00:00:00 2001 From: Morgan Rae Reschenberg Date: Thu, 12 Jan 2023 19:10:12 +0000 Subject: [PATCH] Bug 1808828: Compute ancestor transform for continuations that span multiple subtrees r=Jamie Differential Revision: https://phabricator.services.mozilla.com/D166485 --- accessible/generic/LocalAccessible.cpp | 19 +++++ .../e10s/browser_caching_text_bounds.js | 81 ++++++++++++++++++- accessible/tests/mochitest/layout.js | 2 +- 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/accessible/generic/LocalAccessible.cpp b/accessible/generic/LocalAccessible.cpp index 5800bd4e92ef..96cc7ae6ca9d 100644 --- a/accessible/generic/LocalAccessible.cpp +++ b/accessible/generic/LocalAccessible.cpp @@ -3399,9 +3399,27 @@ already_AddRefed LocalAccessible::BundleFieldsForCache( nsTArray charData; if (nsTextFrame* currTextFrame = do_QueryFrame(frame)) { + nsTextFrame* prevTextFrame = currTextFrame; nsRect frameRect = currTextFrame->GetRect(); + nsIFrame* nearestAccAncestorFrame = + LocalParent() ? LocalParent()->GetFrame() : nullptr; while (currTextFrame) { nsRect contRect = currTextFrame->GetRect(); + if (prevTextFrame->GetParent() != currTextFrame->GetParent() && + nearestAccAncestorFrame) { + // Continuations can span multiple frame tree subtrees, + // particularly when multiline text is nested within both block + // and inline elements. In addition to using the position of this + // continuation to offset our char rects, we'll need to offset + // this continuation from the continuations that occurred before + // it. We don't know how many there are or what subtrees they're + // in, so we use a transform here. This also ensures our offset is + // accurate even if the intervening inline elements are not + // present in the a11y tree. + contRect = frameRect; + nsLayoutUtils::TransformRect(currTextFrame, + nearestAccAncestorFrame, contRect); + } nsTArray charBounds; currTextFrame->GetCharacterRectsInRange( currTextFrame->GetContentOffset(), @@ -3422,6 +3440,7 @@ already_AddRefed LocalAccessible::BundleFieldsForCache( charData.AppendElement(charRect.width); charData.AppendElement(charRect.height); } + prevTextFrame = currTextFrame; currTextFrame = currTextFrame->GetNextContinuation(); } } diff --git a/accessible/tests/browser/e10s/browser_caching_text_bounds.js b/accessible/tests/browser/e10s/browser_caching_text_bounds.js index e16e10d31d7b..0f505992938f 100644 --- a/accessible/tests/browser/e10s/browser_caching_text_bounds.js +++ b/accessible/tests/browser/e10s/browser_caching_text_bounds.js @@ -34,13 +34,24 @@ async function testTextRange(accDoc, browser, id, start, end) { // ignore whitespace, but not embedded elements let isEmbeddedElement = false; if (element.length == undefined) { - if (!element.firstChild) { - continue; - } else { + let potentialTextContainer = element; + while ( + potentialTextContainer && + potentialTextContainer.length == undefined + ) { + potentialTextContainer = element.firstChild; + } + if (potentialTextContainer && potentialTextContainer.length) { + // If we can reach some text from this container, use that as part + // of our range. This is important when testing with intervening inline + // elements. ie.
ab%0acd
+            element = potentialTextContainer;
+          } else if (element.firstChild) {
             isEmbeddedElement = true;
+          } else {
+            continue;
           }
         }
-
         if (element.length + traversed < _start) {
           // If our start index is not within this
           // node, keep looking.
@@ -431,6 +442,68 @@ addAccessibleTask(
   { chrome: true, topLevel: !isWinNoCache }
 );
 
+/**
+ * Test character bounds in an intervening inline element with non-br line breaks
+ */
+addAccessibleTask(
+  `
+  
+  
XX
+XXX
+XX
+X
`, + async function(browser, docAcc) { + await testChar(docAcc, browser, "t", 0); + await testChar(docAcc, browser, "t", 3); + await testChar(docAcc, browser, "t", 7); + await testChar(docAcc, browser, "t", 10); + }, + { + chrome: true, + topLevel: !isWinNoCache, + iframe: !isWinNoCache, + } +); + +// XXX: There's a fuzziness here of about 8 pixels, implying we aren't taking into +// account some kind of margin or padding. See bug 1809695. +// /** +// * Test character bounds in an intervening inline element with margins +// * and with non-br line breaks +// */ +// addAccessibleTask( +// ` +// +//
hello
XX
+// XXX
+// XX
+// X
`, +// async function(browser, docAcc) { +// await testChar(docAcc, browser, "t", 0); +// await testChar(docAcc, browser, "t", 3); +// await testChar(docAcc, browser, "t", 7); +// await testChar(docAcc, browser, "t", 10); +// }, +// { +// chrome: true, +// topLevel: !isWinNoCache, +// iframe: !isWinNoCache, +// } +// ); + /** * Test text bounds in a textarea after scrolling. */ diff --git a/accessible/tests/mochitest/layout.js b/accessible/tests/mochitest/layout.js index f10c14bad10e..1467d5fe7fa8 100644 --- a/accessible/tests/mochitest/layout.js +++ b/accessible/tests/mochitest/layout.js @@ -170,7 +170,7 @@ function testTextPos(aID, aOffset, aPoint, aCoordOrigin) { "Wrong x coordinate at offset " + aOffset + " for " + prettyName(aID) ); ok( - yObj.value - expectedY < 2 && expectedY - yObj.value < 2, + yObj.value - expectedY <= 2 && expectedY - yObj.value <= 2, "Wrong y coordinate at offset " + aOffset + " for " +