Bug 235646 – AX ITM: Defer to the tree when determining AX object loading progress

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

Currently, we set AXPropertyName::IsLoaded and AXPropertyName::EstimatedLoadingProgress once in AXIsolatedObject::initializeAttributeData and never update it.

This means that if an object is created while the page is at some non-100% load percentage, AX clients will always think the object isn't loaded.

[Radar WebKit Bug Importer]

<rdar://problem/88078376>

[Tyler Wilcock]

Created attachment 450031 [details]
Patch

[Tyler Wilcock]

Created attachment 450034 [details]
Patch

[Tyler Wilcock]

Created attachment 450038 [details]
Patch

[Tyler Wilcock]

Created attachment 450042 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 450034 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ a/Source/WebCore/accessibility/AXObjectCache.cpp

+        m_estimatedLoadingProgress = newProgressValue;

Why estimated? This is the best we are going to know, right? So for us is the truth.

--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ a/Source/WebCore/accessibility/AXObjectCache.h

+    void updateEstimatedLoadingProgress(double);
+    void loadingProgressFinished() { updateEstimatedLoadingProgress(1); }
+    double estimatedLoadingProgress() const { return m_estimatedLoadingProgress; }

consider updateLoadingProgress, loadingFinished and loadingProgress instead.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

-    double estimatedLoadingProgress() const override { return doubleAttributeValue(AXPropertyName::EstimatedLoadingProgress); }
+    double estimatedLoadingProgress() const override { return tree()->estimatedLoadingProgress(); }

Would get rid of "estimated". We are not estimating anything.

--- a/Source/WebCore/page/Page.cpp
+++ a/Source/WebCore/page/Page.cpp

+void Page::progressEstimateChanged(Frame& frameWithProgressUpdate) const
+{
+    if (auto* document = frameWithProgressUpdate.document()) {
+        if (auto* axObjectCache = document->axObjectCache())
+            axObjectCache->updateEstimatedLoadingProgress(progress().estimatedProgress());
+    }
+}
+
+void Page::progressFinished(Frame& frameWithCompletedProgress) const
+{
+    if (auto* document = frameWithCompletedProgress.document()) {
+        if (auto* axObjectCache = document->axObjectCache())
+            axObjectCache->loadingProgressFinished();
+    }
+}
+
Perhaps we want to do document->axObjectCacheIfExist() instead? Because I think document->axObjectCache() creates it if it doesn't exist, but I'm going by memory here.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 450034 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> 
> +        m_estimatedLoadingProgress = newProgressValue;
> 
> Why estimated? This is the best we are going to know, right? So for us is
> the truth.
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.h
> +++ a/Source/WebCore/accessibility/AXObjectCache.h
> 
> +    void updateEstimatedLoadingProgress(double);
> +    void loadingProgressFinished() { updateEstimatedLoadingProgress(1); }
> +    double estimatedLoadingProgress() const { return
> m_estimatedLoadingProgress; }
> 
> consider updateLoadingProgress, loadingFinished and loadingProgress instead.
> 
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> 
> -    double estimatedLoadingProgress() const override { return
> doubleAttributeValue(AXPropertyName::EstimatedLoadingProgress); }
> +    double estimatedLoadingProgress() const override { return
> tree()->estimatedLoadingProgress(); }
> 
> Would get rid of "estimated". We are not estimating anything.
> 
> --- a/Source/WebCore/page/Page.cpp
> +++ a/Source/WebCore/page/Page.cpp
> 
> +void Page::progressEstimateChanged(Frame& frameWithProgressUpdate) const
> +{
> +    if (auto* document = frameWithProgressUpdate.document()) {
> +        if (auto* axObjectCache = document->axObjectCache())
> +           
> axObjectCache->updateEstimatedLoadingProgress(progress().
> estimatedProgress());
> +    }
> +}
> +
> +void Page::progressFinished(Frame& frameWithCompletedProgress) const
> +{
> +    if (auto* document = frameWithCompletedProgress.document()) {
> +        if (auto* axObjectCache = document->axObjectCache())
> +            axObjectCache->loadingProgressFinished();
> +    }
> +}
> +
> Perhaps we want to do document->axObjectCacheIfExist() instead? Because I
> think document->axObjectCache() creates it if it doesn't exist, but I'm
> going by memory here.
Ah, axObjectCacheIfExists() is definitely what we want. Thanks!

I chose to name things like "estimatedLoadingProgress" instead of "loadingProgress" to be consistent with that naming in ProgressTracker, and in the AXCoreObject interface, which has a method called estimatedLoadingProgress.

I do agree that "loading progress" is better naming than "estimated loading progress", so I'm going to fix this by renaming the AXCoreObject interface method.

[Tyler Wilcock]

Created attachment 450054 [details]
Patch

[Tyler Wilcock]

Created attachment 450071 [details]
Patch

[Tyler Wilcock]

Created attachment 450076 [details]
Patch

[chris fleizach]

Comment on attachment 450076 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.h:529
> +    double m_loadingProgress { 0.25 };

how come default is .25

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363
> +    void updateLoadingProgress(double newProgress);

not necessary to name newProgress here

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416
> +    double m_loadingProgress { 0.25 };

defaults .25?

[Tyler Wilcock]

(In reply to chris fleizach from comment #11)
> Comment on attachment 450076 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450076&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.h:529
> > +    double m_loadingProgress { 0.25 };
> 
> how come default is .25
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363
> > +    void updateLoadingProgress(double newProgress);
> 
> not necessary to name newProgress here
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416
> > +    double m_loadingProgress { 0.25 };
> 
> defaults .25?
The default is mostly arbitrary so long as it's not 100% since it will be overwritten as the page loads / reloads. I figured that by the time the cache / tree is created, we can guess the page will be at least 25% loaded, but no science there. In my testing this default is never actually used, but we should default initialize the value to something if not this.

[chris fleizach]

(In reply to Tyler Wilcock from comment #12)
> (In reply to chris fleizach from comment #11)
> > Comment on attachment 450076 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=450076&action=review
> > 
> > > Source/WebCore/accessibility/AXObjectCache.h:529
> > > +    double m_loadingProgress { 0.25 };
> > 
> > how come default is .25
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363
> > > +    void updateLoadingProgress(double newProgress);
> > 
> > not necessary to name newProgress here
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416
> > > +    double m_loadingProgress { 0.25 };
> > 
> > defaults .25?
> The default is mostly arbitrary so long as it's not 100% since it will be
> overwritten as the page loads / reloads. I figured that by the time the
> cache / tree is created, we can guess the page will be at least 25% loaded,
> but no science there. In my testing this default is never actually used, but
> we should default initialize the value to something if not this.

Ok if it doesn't matter, might be less confusing to just leave as zero (for other programmers looking at this)

[Tyler Wilcock]

Created attachment 450085 [details]
Patch

[Tyler Wilcock]

Created attachment 450086 [details]
Patch

[Tyler Wilcock]

> Ok if it doesn't matter, might be less confusing to just leave as zero (for
> other programmers looking at this)
OK, done.

[EWS]

Committed r288674 (246480@main): <https://commits.webkit.org/246480@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450086 [details].

[Don Olmstead]

This broke !ENABLE(ACCESSIBILITY) builds because updateLoadingProgress wasn't added to in the block of inlined methods below.

Following up in with a patch in https://bugs.webkit.org/show_bug.cgi?id=235727

[Tyler Wilcock]

Sorry about that. Thanks for fixing it.

[Don Olmstead]

(In reply to Tyler Wilcock from comment #19)
> Sorry about that. Thanks for fixing it.

No worries it happens since none of the EWS bots are !ENABLE(ACCESSIBILITY).