Bug 242268 – AX ITM: Fix for accessibility/auto-fill-types.html in isolated tree mode.

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

[Andres Gonzalez]

This test is currently timing out in ITM.

[Radar WebKit Bug Importer]

<rdar://problem/96315434>

[Andres Gonzalez]

Created attachment 460625 [details]
Patch

[chris fleizach]

Comment on attachment 460625 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:-401
> -- (NSString *)baseAccessibilityDescription

are we using this on iOS?
generally we want to move in direction where iOS and Mac can share same implementations

[Tyler Wilcock]

Comment on attachment 460625 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:383
> +    AXLOG(makeString("property: ", property));

Do we want to keep this logging? If so, could we make it more succinct by replacing these two log lines with something like:

AXLOG(makeString("updateNodeProperty of ", property, " for objectID ", axObject.objectID().loggingString());

> Source/WebCore/html/HTMLInputElement.cpp:1399
> +    if (auto* renderer = this->renderer()) {

Do we need a renderer here, or can we just use the node to get the document().existingAXObjectCache()? Sometimes we don't have a renderer (e.g. display:contents, which I doubt is valid for input elements...but still).

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 460625 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460625&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:-401
> > -- (NSString *)baseAccessibilityDescription
> 
> are we using this on iOS?
> generally we want to move in direction where iOS and Mac can share same
> implementations

Not used on iOS.

[Andres Gonzalez]

Created attachment 460760 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 460625 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460625&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:383
> > +    AXLOG(makeString("property: ", property));
> 
> Do we want to keep this logging? If so, could we make it more succinct by
> replacing these two log lines with something like:
> 
> AXLOG(makeString("updateNodeProperty of ", property, " for objectID ",
> axObject.objectID().loggingString());

Done in latest revision.
> 
> > Source/WebCore/html/HTMLInputElement.cpp:1399
> > +    if (auto* renderer = this->renderer()) {
> 
> Do we need a renderer here, or can we just use the node to get the
> document().existingAXObjectCache()? Sometimes we don't have a renderer (e.g.
> display:contents, which I doubt is valid for input elements...but still).

Done.

[Tyler Wilcock]

Comment on attachment 460760 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1899
> +        AXLOG("Deferred handling");

If we want to keep this log, could it include more information? I know it's nested inside an AXTRACE, but I find sometimes a lot of logs happen after an AXTRACE opening, making it hard to reference again because it's far away. Maybe we could include the same makeString from above?

makeString("Deferring the handling of attribute ", attrName.localName().string(), " for element ", element->debugDescription())

What do you think?

> Source/WebCore/accessibility/AXObjectCache.cpp:3405
> +    AXLOG(makeString("ChildrenChangedList", m_deferredChildrenChangedList.size()));

Is this log missing a space between the size and ChildrenChangedList string? Maybe we could also include the word "size" to indicate what the number means.

> Source/WebCore/accessibility/AXObjectCache.cpp:3415
> +        AXLOG("Baling out due to reentrant call.");

Baling misspelled, should be bailing.

> Source/WebCore/accessibility/AXObjectCache.cpp:3420
> +    AXLOG(makeString("RecomputeTableIsExposedList ", m_deferredRecomputeTableIsExposedList.computeSize()));

Should this include the word "size"? Same for the other logs in this function.

[Andres Gonzalez]

Created attachment 460767 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> Comment on attachment 460760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460760&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1899
> > +        AXLOG("Deferred handling");
> 
> If we want to keep this log, could it include more information? I know it's
> nested inside an AXTRACE, but I find sometimes a lot of logs happen after an
> AXTRACE opening, making it hard to reference again because it's far away.
> Maybe we could include the same makeString from above?
> 
> makeString("Deferring the handling of attribute ",
> attrName.localName().string(), " for element ", element->debugDescription())
> 
> What do you think?

Replaced with suggestion and removed the one below AXTRACE to avoid duplication.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:3405
> > +    AXLOG(makeString("ChildrenChangedList", m_deferredChildrenChangedList.size()));
> 
> Is this log missing a space between the size and ChildrenChangedList string?
> Maybe we could also include the word "size" to indicate what the number
> means.
> 
Done.
> > Source/WebCore/accessibility/AXObjectCache.cpp:3415
> > +        AXLOG("Baling out due to reentrant call.");
> 
> Baling misspelled, should be bailing.
> 
Fixed.
> > Source/WebCore/accessibility/AXObjectCache.cpp:3420
> > +    AXLOG(makeString("RecomputeTableIsExposedList ", m_deferredRecomputeTableIsExposedList.computeSize()));
> 
> Should this include the word "size"? Same for the other logs in this
> function.

Done.

[EWS]

Committed 252332@main (755db3c9b12c): <https://commits.webkit.org/252332@main>

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