Bug 270395 – AX: Add notification for VoiceOver to announce percentage of completion of isolated tree creation.

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

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/123951195>

[Andres Gonzalez]

rdar://122335605

[Andres Gonzalez]

Created attachment 470141 [details]
Patch

[chris fleizach]

Comment on attachment 470141 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:505
> +    String titleAttributeValue() const;

should this be final

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 470141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470141&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:505
> > +    String titleAttributeValue() const;
> 
> should this be final

The whole class is final, so I don't think we need to add final to each individual method.

[chris fleizach]

Comment on attachment 470141 [details]
Patch

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

>>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:505
>>> +    String titleAttributeValue() const;
>> 
>> should this be final
> 
> The whole class is final, so I don't think we need to add final to each individual method.

Maybe we should remove final from all the other methods then?

[Andres Gonzalez]

(In reply to chris fleizach from comment #6)
> Comment on attachment 470141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470141&action=review
> 
> >>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:505
> >>> +    String titleAttributeValue() const;
> >> 
> >> should this be final
> > 
> > The whole class is final, so I don't think we need to add final to each individual method.
> 
> Maybe we should remove final from all the other methods then?

I'll check whether there is a WebKit/C++ guideline about this, and address in a separate patch.

[Andres Gonzalez]

Created attachment 470162 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #6)
> Comment on attachment 470141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470141&action=review
> 
> >>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:505
> >>> +    String titleAttributeValue() const;
> >> 
> >> should this be final
> > 
> > The whole class is final, so I don't think we need to add final to each individual method.
> 
> Maybe we should remove final from all the other methods then?

There are some advantages in declaring each member function as final in addition to declaring the class final. So I'm correcting myself and made that change. Also changed it for a couple of other methods related to datetimes I added before. Thanks.

[Andres Gonzalez]

Created attachment 470212 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470212 [details]
Patch

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

> Source/WebCore/accessibility/AXLogger.cpp:58
> +    static NeverDestroyed nameFilter = Vector<String> { "AXIsolatedTree::updateLoadingProgress"_s, "AXIsolatedTree::reportLoadingProgress"_s, "AXIsolatedTree::create"_s };

Do we want to commit this? Or is it left over from debugging?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:146
> +    else
> +        AXLOG("no existing tree!");

Do we want to commit this? Or is it leftover from debugging?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:198
> +    String percent = String::number(std::ceil(loadingProgress() * 100)) + "%"_s;

I know this problem existed before this patch, but I realize now that this is not localized. For example, some locales expect the percentage sign to be first. Maybe we can address this in a follow-up patch.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:203
> +    if (RefPtr axWebarea = cache ? cache->rootWebArea() : nullptr) {

Most places seem to capitalize web area like axWebArea, rather than axWebarea.

[Andres Gonzalez]

Created attachment 470218 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #11)
> Comment on attachment 470212 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470212&action=review
> 
> > Source/WebCore/accessibility/AXLogger.cpp:58
> > +    static NeverDestroyed nameFilter = Vector<String> { "AXIsolatedTree::updateLoadingProgress"_s, "AXIsolatedTree::reportLoadingProgress"_s, "AXIsolatedTree::create"_s };
> 
> Do we want to commit this? Or is it left over from debugging?
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:146
> > +    else
> > +        AXLOG("no existing tree!");
> 
> Do we want to commit this? Or is it leftover from debugging?
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:198
> > +    String percent = String::number(std::ceil(loadingProgress() * 100)) + "%"_s;
> 
> I know this problem existed before this patch, but I realize now that this
> is not localized. For example, some locales expect the percentage sign to be
> first. Maybe we can address this in a follow-up patch.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:203
> > +    if (RefPtr axWebarea = cache ? cache->rootWebArea() : nullptr) {
> 
> Most places seem to capitalize web area like axWebArea, rather than
> axWebarea.

Addressed all comments except the localization issue which I'll fix in a separate follow up. Thanks!

[Tyler Wilcock]

Comment on attachment 470218 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:206
> +        if (cache)
> +            cache->postPlatformNotification(axWebArea.get(), AXObjectCache::AXNotification::AXLayoutComplete);

So we're posting layout complete notifications to report partial progress? That doesn't feel exactly right to me — there's no layout happening here, and it's not complete (presumably unless processingProgress == 1). Can you help me understand why we do it this way?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #14)
> Comment on attachment 470218 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470218&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:206
> > +        if (cache)
> > +            cache->postPlatformNotification(axWebArea.get(), AXObjectCache::AXNotification::AXLayoutComplete);
> 
> So we're posting layout complete notifications to report partial progress?
> That doesn't feel exactly right to me — there's no layout happening here,
> and it's not complete (presumably unless processingProgress == 1). Can you
> help me understand why we do it this way?

As I put in the commit comment, that's what VoiceOver expects and reacts to in order to announce loading progress. There is a long story about this workaround that is beyond the scope of this change, but I agree that this should be refactored in the future.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #15)
> (In reply to Tyler Wilcock from comment #14)
> > Comment on attachment 470218 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=470218&action=review
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:206
> > > +        if (cache)
> > > +            cache->postPlatformNotification(axWebArea.get(), AXObjectCache::AXNotification::AXLayoutComplete);
> > 
> > So we're posting layout complete notifications to report partial progress?
> > That doesn't feel exactly right to me — there's no layout happening here,
> > and it's not complete (presumably unless processingProgress == 1). Can you
> > help me understand why we do it this way?
> 
> As I put in the commit comment, that's what VoiceOver expects and reacts to
> in order to announce loading progress. There is a long story about this
> workaround that is beyond the scope of this change, but I agree that this
> should be refactored in the future.
OK, got it. Might be nice to have a FIXME comment here so we don't forget.

[EWS]

Committed 275788@main (44f5b661008a): <https://commits.webkit.org/275788@main>

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