Bug 187824 - AX: Press tab to highlight items on a webpage is not working with voiceover enabled
Summary: AX: Press tab to highlight items on a webpage is not working with voiceover e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-19 15:14 PDT by Nan Wang
Modified: 2018-07-23 14:24 PDT (History)
16 users (show)

See Also:


Attachments
patch (4.61 KB, patch)
2018-07-19 15:21 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (7.85 KB, patch)
2018-07-19 22:42 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (9.61 KB, patch)
2018-07-23 10:57 PDT, Nan Wang
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2018-07-19 15:14:14 PDT
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>
Comment 1 Nan Wang 2018-07-19 15:21:32 PDT
Created attachment 345390 [details]
patch
Comment 2 zalan 2018-07-19 19:02:28 PDT
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.
Comment 3 Nan Wang 2018-07-19 21:30:30 PDT
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.
Comment 4 zalan 2018-07-19 21:43:56 PDT
(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.
Comment 5 Nan Wang 2018-07-19 21:52:13 PDT
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?
Comment 6 Nan Wang 2018-07-19 22:42:34 PDT
Created attachment 345429 [details]
patch

update from review
Comment 7 Nan Wang 2018-07-23 10:00:14 PDT
Zalan, can you take another look at my patch?
Comment 8 zalan 2018-07-23 10:10:05 PDT
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.
Comment 9 Nan Wang 2018-07-23 10:57:56 PDT
Created attachment 345583 [details]
patch

update from review.
Comment 10 zalan 2018-07-23 13:45:50 PDT
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).
Comment 11 Nan Wang 2018-07-23 14:24:22 PDT
Committed r234112: <https://trac.webkit.org/changeset/234112>