Bug 257406

Summary: AX: Cache AXPropertyName::RelativeFrame for AccessibilityScrollbars
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, ross.kirsling, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Tyler Wilcock 2023-05-26 17:28:55 PDT
...
Comment 1 Radar WebKit Bug Importer 2023-05-26 17:29:05 PDT
<rdar://problem/109912633>
Comment 2 Tyler Wilcock 2023-05-26 17:34:40 PDT
Created attachment 466509 [details]
Patch
Comment 3 Tyler Wilcock 2023-05-26 17:38:00 PDT
Created attachment 466510 [details]
Patch
Comment 4 Andres Gonzalez 2023-05-30 16:43:37 PDT
(In reply to Tyler Wilcock from comment #3)
> Created attachment 466510 [details]
> Patch

Looks good, just a couple of minor comments.
--- a/Source/WebCore/accessibility/AXGeometryManager.cpp
+++ b/Source/WebCore/accessibility/AXGeometryManager.cpp

-std::optional<IntRect> AXGeometryManager::paintRectForID(AXID axID)
+std::optional<IntRect> AXGeometryManager::cachedRectForID(AXID axID)

Maybe just rectForID ?

-    if (!paintRectChanged)
+    if (!rectChanged)
         return;

     RefPtr tree = AXIsolatedTree::treeForPageID(*m_cache->pageID());

m_cache->pageID() can be nullopt, couldn't it?

+void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar& scrollbar)

I guess we haven't standardized on using "Change" (as a noun) or Changed (as a past tense verb) in these functions. I can go either way but would be good to be consistent.
Comment 5 Tyler Wilcock 2023-05-30 16:53:33 PDT
(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 466510 [details]
> > Patch
> 
> Looks good, just a couple of minor comments.
> --- a/Source/WebCore/accessibility/AXGeometryManager.cpp
> +++ b/Source/WebCore/accessibility/AXGeometryManager.cpp
> 
> -std::optional<IntRect> AXGeometryManager::paintRectForID(AXID axID)
> +std::optional<IntRect> AXGeometryManager::cachedRectForID(AXID axID)
> 
> Maybe just rectForID ?
I did consider rectForID, but decided against it because in my opinion that name implies that it is the way to get geometry for any object, and in reality that is not the case (only things with cacheable geometry). Plus, having "cached" in the name lends more credence to the fact that the function returns an optional (std::nullopt logically means that there was no "cached" rect).

> -    if (!paintRectChanged)
> +    if (!rectChanged)
>          return;
> 
>      RefPtr tree = AXIsolatedTree::treeForPageID(*m_cache->pageID());
> 
> m_cache->pageID() can be nullopt, couldn't it?
This method should never be called (and is not called) on a cache with no page ID -- we null check m_pageID before calling this in all places. There is an ASSERT at the top of the method verifying / documenting this expectation.

> +void AXObjectCache::onScrollbarFrameRectChange(const Scrollbar& scrollbar)
> 
> I guess we haven't standardized on using "Change" (as a noun) or Changed (as
> a past tense verb) in these functions. I can go either way but would be good
> to be consistent.
Agreed. I prefer onFooChange vs. onFooChanged. Let's do that cleanup in a separate patch.
Comment 6 Tyler Wilcock 2023-05-31 10:36:09 PDT
Created attachment 466550 [details]
Patch
Comment 7 EWS 2023-05-31 16:08:47 PDT
Committed 264762@main (1a3a54995b6f): <https://commits.webkit.org/264762@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 466550 [details].
Comment 8 Ross Kirsling 2023-06-01 02:58:42 PDT
Fixed !ENABLE(ACCESSIBILITY) build break in https://github.com/WebKit/WebKit/pull/14582.