Bug 187824 – AX: Press tab to highlight items on a webpage is not working with voiceover enabled

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

[Nan Wang]

1.  Ensure that press tab to highlight items on a webpage is enabled (Safari>preferences>advanceed>press-ta to highlight items on a webpage)
2. visit a website like:
http://www.apple.com, and attempt to press tab to move back and forth through various navigation elements on the webpage.
3.  Ensure this with voiceover enabled.
Expected: pressing tab would allow the cursor to highlight individual elements.
Actual, the cursor does not move.

<rdar://problem/41448721>

[Nan Wang]

Created attachment 345390 [details]
patch

[alan]

Comment on attachment 345390 [details]
patch

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

> Source/WebCore/dom/Document.cpp:1871
> +            }

Any style mutation with an associated renderer will trigger a RenderElement::setStyle() call. Even mutations that don't need layout. Consulting StyleDifference in setStyle() might be a better way to get the AX cache updated.  
Also you may want to limit the performDeferredCacheUpdate() calls to focusable elements since calling AX cache on every repaint could be a performance hit.

[Nan Wang]

Comment on attachment 345390 [details]
patch

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

>> Source/WebCore/dom/Document.cpp:1871
>> +            }
> 
> Any style mutation with an associated renderer will trigger a RenderElement::setStyle() call. Even mutations that don't need layout. Consulting StyleDifference in setStyle() might be a better way to get the AX cache updated.  
> Also you may want to limit the performDeferredCacheUpdate() calls to focusable elements since calling AX cache on every repaint could be a performance hit.

Here we are doing the AX cache update on the document level and it's under the styleUpdate check. Are you saying:
1. frameView.needsLayout() check is not enough, we should use the StyleDifference in renderElement?
2. We should move the AX cache update to a RenderElement level, so for each RenderElement we check and update the cache in setStyle()? I think performDeferredCacheUpdate() is designed as a batch update mechanism. We are calling that in FrameView performPostLayoutTasks(). So I'm looking for some place similar, that indicates the FrameView;s style is resolved.

[alan]

(In reply to Nan Wang from comment #3)
> Comment on attachment 345390 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345390&action=review
> 
> >> Source/WebCore/dom/Document.cpp:1871
> >> +            }
> > 
> > Any style mutation with an associated renderer will trigger a RenderElement::setStyle() call. Even mutations that don't need layout. Consulting StyleDifference in setStyle() might be a better way to get the AX cache updated.  
> > Also you may want to limit the performDeferredCacheUpdate() calls to focusable elements since calling AX cache on every repaint could be a performance hit.
> 
> Here we are doing the AX cache update on the document level and it's under
> the styleUpdate check. Are you saying:
> 1. frameView.needsLayout() check is not enough, we should use the
> StyleDifference in renderElement?
> 2. We should move the AX cache update to a RenderElement level, so for each
> RenderElement we check and update the cache in setStyle()? I think
> performDeferredCacheUpdate() is designed as a batch update mechanism. We are
> calling that in FrameView performPostLayoutTasks(). So I'm looking for some
> place similar, that indicates the FrameView;s style is resolved.

Oh right, this is the "perform" and not the "dirtying" part. So who is dirtying the AX tree when non-layout type of mutation happens? I would assume that if AX tree sees that the mutation is not going to trigger layout, it could just issue a timer and update the tree in the next runloop.

[Nan Wang]

Comment on attachment 345390 [details]
patch

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

>>>> Source/WebCore/dom/Document.cpp:1871
>>>> +            }
>>> 
>>> Any style mutation with an associated renderer will trigger a RenderElement::setStyle() call. Even mutations that don't need layout. Consulting StyleDifference in setStyle() might be a better way to get the AX cache updated.  
>>> Also you may want to limit the performDeferredCacheUpdate() calls to focusable elements since calling AX cache on every repaint could be a performance hit.
>> 
>> Here we are doing the AX cache update on the document level and it's under the styleUpdate check. Are you saying:
>> 1. frameView.needsLayout() check is not enough, we should use the StyleDifference in renderElement?
>> 2. We should move the AX cache update to a RenderElement level, so for each RenderElement we check and update the cache in setStyle()? I think performDeferredCacheUpdate() is designed as a batch update mechanism. We are calling that in FrameView performPostLayoutTasks(). So I'm looking for some place similar, that indicates the FrameView;s style is resolved.
> 
> Oh right, this is the "perform" and not the "dirtying" part. So who is dirtying the AX tree when non-layout type of mutation happens? I would assume that if AX tree sees that the mutation is not going to trigger layout, it could just issue a timer and update the tree in the next runloop.

This one is triggered in Document::setFocusedElement()
if (!focusChangeBlocked && m_focusedElement) {
    // Create the AXObject cache in a focus change because GTK relies on it.
    if (AXObjectCache* cache = axObjectCache()) 
        cache->deferFocusedUIElementChangeIfNeeded(oldFocusedElement.get(), newFocusedElement.get());
 }

Should we just do not defer in this case?

[Nan Wang]

Created attachment 345429 [details]
patch

update from review

[Nan Wang]

Zalan, can you take another look at my patch?

[alan]

Comment on attachment 345429 [details]
patch

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

> Source/WebCore/ChangeLog:13
> +        happens.

When the timer fires, you should check if there's a pending layout and let the layout trigger the AX update. This is for the cases when you have different type of mutations coming the same time with the focus change. 
1. focus change
2. focus change
3. tree mutation
4. some other style mutation
In this case your timer will fire before the layout timer and you should really just not do anything at this point.

[Nan Wang]

Created attachment 345583 [details]
patch

update from review.

[alan]

Comment on attachment 345583 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2861
> +    RefPtr<FrameView> frameView = document().view();
> +    RenderView* renderView = document().renderView();
> +    bool needsLayout = frameView && renderView && (frameView->layoutContext().isLayoutPending() || renderView->needsLayout());

if (!document().view() || document().view()->needsLayout())
    return;
This should do (when the tree is dirty there must be a pending layout scheduled).

[Nan Wang]

Committed r234112: <https://trac.webkit.org/changeset/234112>