AX: Give user feedback while the isolated tree is being built.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Especially needed for large pages where building the isolated tree can take several seconds.

[Radar WebKit Bug Importer]

<rdar://problem/117857438>

[Andres Gonzalez]

Created attachment 468462 [details]
Patch

[Andres Gonzalez]

Created attachment 468468 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468468 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:-155
> -    tree->updatingSubtree(nullptr);

Is this removing the only place we set updatingSubtree to nullptr?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:181
> +    String percent = String::number(percentComplete) + "%"_s;
> +    String title = AXProcessingPage() + " "_s + percent;

I think using makeString will be more efficient here (less String allocations).

String title = makeString(AXProcessingPage(), " ", percentComplete, "%");

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:190
> +            { AXPropertyName::TitleAttributeValue, title },

I think we can WTFMove(title) here.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:389
> +    double i = 0;

Is there a more descriptive name that could be used? appendsResolved? Something else?

> Source/WebCore/platform/LocalizedStrings.cpp:955
> +    return WEB_UI_STRING("Processing page", "Title for the webarea while accessibility isolated tree is being built.");

I think this note:

"Title for the webarea while accessibility isolated tree is being built."

is only used but folks doing localization, who might not know what "isolated tree" is (and probably don't need to). Maybe we just call it the accessibility tree?

[Andres Gonzalez]

Created attachment 468477 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 468468 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468468&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:-155
> > -    tree->updatingSubtree(nullptr);
> 
> Is this removing the only place we set updatingSubtree to nullptr?

AG: At the moment, this was superfluous, it was always null for a full tree. The only time it is not null is for the empty content tree, and for that tree it remains not null until the tree is destroyed. This will have a role when we implement some sort of feedback during updates of isolated subtrees or to avoid processing notifications while a subtree is being re-built.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:181
> > +    String percent = String::number(percentComplete) + "%"_s;
> > +    String title = AXProcessingPage() + " "_s + percent;
> 
> I think using makeString will be more efficient here (less String
> allocations).
> 
> String title = makeString(AXProcessingPage(), " ", percentComplete, "%");

AG: we need both Strings, the percent and the title since they are used for different purposes. Also makeString and the operator+ resolve to invoking the same method tryMakeString in StringConcatenate.h, so I don't think there is significant advantage of one over the other performance wise, and + is a lot more readable.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:190
> > +            { AXPropertyName::TitleAttributeValue, title },
> 
> I think we can WTFMove(title) here.


Fixed.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:389
> > +    double i = 0;
> 
> Is there a more descriptive name that could be used? appendsResolved?
> Something else?

counter since that's what it is.
> 
> > Source/WebCore/platform/LocalizedStrings.cpp:955
> > +    return WEB_UI_STRING("Processing page", "Title for the webarea while accessibility isolated tree is being built.");
> 
> I think this note:
> 
> "Title for the webarea while accessibility isolated tree is being built."
> 
> is only used but folks doing localization, who might not know what "isolated
> tree" is (and probably don't need to). Maybe we just call it the
> accessibility tree?

Removed "isolated" and left it as "accessibility tree".

[EWS]

Committed 270280@main (16714dac0820): <https://commits.webkit.org/270280@main>

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