From efe397519e8f2793ca8cf1c30a8398ada75fafe6 Mon Sep 17 00:00:00 2001 From: James Teh Date: Fri, 27 May 2022 10:56:40 +0000 Subject: [PATCH] Bug 1737919 part 1: TextLeafPoint::FindTextAttrsStart: Don't allow the caller to supply origin attributes. r=morgan This was done for LocalAccessibles to avoid the need to fetch attributes twice when we're simultaneously fetching boundaries and returning the attributes to a client. However, GetTextAttributes will soon handle spelling errors, so it will no longer return the same data as GetTextAttributesLocalAcc (which doesn't handle spelling errors). This breaks the equality checks in FindTextAttrsStart. Aside from this, this argument was somewhat confusing. For now, just remove aOriginAttrs, which means there will be some redundant calls to GetTextAttributesLocalAcc. Parent process documents shouldn't be that large anyway. If this ends up being a real problem, we can either revert to the local HyperTextAccessible implementation of text attributes or implement a smarter temporary cache. Differential Revision: https://phabricator.services.mozilla.com/D147241 --- accessible/base/TextLeafRange.cpp | 37 ++++--------------- accessible/base/TextLeafRange.h | 11 +----- .../basetypes/HyperTextAccessibleBase.cpp | 11 +++--- 3 files changed, 14 insertions(+), 45 deletions(-) diff --git a/accessible/base/TextLeafRange.cpp b/accessible/base/TextLeafRange.cpp index 53aefa40bbbf..e0040b691178 100644 --- a/accessible/base/TextLeafRange.cpp +++ b/accessible/base/TextLeafRange.cpp @@ -1061,36 +1061,15 @@ already_AddRefed TextLeafPoint::GetTextAttributes( return attrs.forget(); } -TextLeafPoint TextLeafPoint::FindTextAttrsStart( - nsDirection aDirection, bool aIncludeOrigin, - const AccAttributes* aOriginAttrs, bool aIncludeDefaults) const { +TextLeafPoint TextLeafPoint::FindTextAttrsStart(nsDirection aDirection, + bool aIncludeOrigin) const { if (IsCaret()) { - return ActualizeCaret().FindTextAttrsStart(aDirection, aIncludeOrigin, - aOriginAttrs, aIncludeDefaults); + return ActualizeCaret().FindTextAttrsStart(aDirection, aIncludeOrigin); } - // XXX Add support for spelling errors. - RefPtr lastAttrs; const bool isRemote = mAcc->IsRemote(); - if (isRemote) { - // For RemoteAccessible, leaf attrs and default attrs are cached - // separately. To combine them, we have to copy. Since we're not walking - // outside the container, we don't care about defaults. Therefore, we - // always just fetch the leaf attrs. - // We ignore aOriginAttrs because it might include defaults. Fetching leaf - // attrs is very cheap anyway. - lastAttrs = mAcc->AsRemote()->GetCachedTextAttributes(); - } else { - // For LocalAccessible, we want to avoid calculating attrs more than - // necessary, so we want to use aOriginAttrs if provided. - if (aOriginAttrs) { - lastAttrs = aOriginAttrs; - // Whether we include defaults henceforth must match aOriginAttrs, which - // depends on aIncludeDefaults. Defaults are always calculated even if - // they aren't returned, so calculation cost isn't a concern. - } else { - lastAttrs = GetTextAttributesLocalAcc(aIncludeDefaults); - } - } + RefPtr lastAttrs = + isRemote ? mAcc->AsRemote()->GetCachedTextAttributes() + : GetTextAttributesLocalAcc(); if (aIncludeOrigin && aDirection == eDirNext && mOffset == 0) { // Even when searching forward, the only way to know whether the origin is // the start of a text attrs run is to compare with the previous sibling. @@ -1104,7 +1083,7 @@ TextLeafPoint TextLeafPoint::FindTextAttrsStart( // calculation or copying. RefPtr attrs = isRemote ? point.mAcc->AsRemote()->GetCachedTextAttributes() - : point.GetTextAttributesLocalAcc(aIncludeDefaults); + : point.GetTextAttributesLocalAcc(); if (attrs && lastAttrs && !attrs->Equal(lastAttrs)) { return *this; } @@ -1119,7 +1098,7 @@ TextLeafPoint TextLeafPoint::FindTextAttrsStart( } RefPtr attrs = isRemote ? point.mAcc->AsRemote()->GetCachedTextAttributes() - : point.GetTextAttributesLocalAcc(aIncludeDefaults); + : point.GetTextAttributesLocalAcc(); if (attrs && lastAttrs && !attrs->Equal(lastAttrs)) { // The attributes change here. If we're moving forward, we want to // return this point. If we're moving backward, we've now moved before diff --git a/accessible/base/TextLeafRange.h b/accessible/base/TextLeafRange.h index e3f67731df18..8d17a59b671c 100644 --- a/accessible/base/TextLeafRange.h +++ b/accessible/base/TextLeafRange.h @@ -135,18 +135,9 @@ class TextLeafPoint final { * (depending on the direction). * If aIncludeorigin is true and this is at a boundary, this will be * returned unchanged. - * aOriginAttrs allows the caller to supply the text attributes for this (as - * retrieved by GetTextAttributes). This can be used to avoid fetching the - * attributes twice if they are also to be used for something else; e.g. - * returning the attributes to a client. If aOriginAttrs is null, this method - * will fetch the attributes itself. - * aIncludeDefaults specifies whether aOriginAttrs includes default - * attributes. */ TextLeafPoint FindTextAttrsStart(nsDirection aDirection, - bool aIncludeOrigin = false, - const AccAttributes* aOriginAttrs = nullptr, - bool aIncludeDefaults = true) const; + bool aIncludeOrigin = false) const; /** * Returns a rect (in dev pixels) describing position and size of diff --git a/accessible/basetypes/HyperTextAccessibleBase.cpp b/accessible/basetypes/HyperTextAccessibleBase.cpp index 5a755c8b6a52..6cc51c4e50f9 100644 --- a/accessible/basetypes/HyperTextAccessibleBase.cpp +++ b/accessible/basetypes/HyperTextAccessibleBase.cpp @@ -616,17 +616,16 @@ already_AddRefed HyperTextAccessibleBase::TextAttributes( } TextLeafPoint origin = ToTextLeafPoint(static_cast(offset)); - RefPtr attributes = origin.GetTextAttributes(aIncludeDefAttrs); - TextLeafPoint start = origin.FindTextAttrsStart( - eDirPrevious, /* aIncludeOrigin */ true, attributes, aIncludeDefAttrs); + TextLeafPoint start = + origin.FindTextAttrsStart(eDirPrevious, /* aIncludeOrigin */ true); bool ok; std::tie(ok, *aStartOffset) = TransformOffset(start.mAcc, start.mOffset, /* aIsEndOffset */ false); - TextLeafPoint end = origin.FindTextAttrsStart( - eDirNext, /* aIncludeOrigin */ false, attributes, aIncludeDefAttrs); + TextLeafPoint end = + origin.FindTextAttrsStart(eDirNext, /* aIncludeOrigin */ false); std::tie(ok, *aEndOffset) = TransformOffset(end.mAcc, end.mOffset, /* aIsEndOffset */ true); - return attributes.forget(); + return origin.GetTextAttributes(aIncludeDefAttrs); } void HyperTextAccessibleBase::CroppedSelectionRanges(