[webkit-reviews] review granted: [Bug 269628] AX: Update cached text runs when line layout changes : [Attachment 469939] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Feb 17 13:53:24 PST 2024

Tyler Wilcock <tyler_w at apple.com> has granted  review:
Bug 269628: AX: Update cached text runs when line layout changes

Attachment 469939: Patch


--- Comment #3 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 469939
  --> https://bugs.webkit.org/attachment.cgi?id=469939

View in context: https://bugs.webkit.org/attachment.cgi?id=469939&action=review

> +* LayoutTests/accessibility/ax-thread-text-apis/changing-text-runs.html:

I wonder if something like "dynamic-text-line-wrapping.html" would be a better
name. I imagine we'll want to add lots more tests where we dynamically change
the underlying text runs data structure and test the result, which would make
the current name of this test confusing.

> Source/WebCore/accessibility/AXObjectCache.h:221
> +    void onTextRunsChanged(RenderObject&);

Can this be const RenderObject&?

> Source/WebCore/rendering/RenderBlockFlow.cpp:4042
> +	   if (auto* axObjectCache = document().axObjectCache())

This should use Document::existingAXObjectCache() instead of axObjectCache().
The latter creates a cache if one doesn't already exist, which is probably not
what we want here?

Maybe just `cache` instead of `axObjectCache`?

> LayoutTests/accessibility/ax-thread-text-apis/changing-text-runs.html:9
> +<p id="line" style="word-break: break-word;">Hello world.</p>

Maybe id="paragraph"?

> LayoutTests/accessibility/ax-thread-text-apis/changing-text-runs.html:12
> +<script>
> +    var output = "This tests that text runs are updated when a container is
resized and text wraps.\n";

Everything inside the script tag can be indented one less level.

More information about the webkit-reviews mailing list