Bug 70075 – AX: Regression: Contextual menu not following with VO cursor in HTML content when item is scrolled

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

10.7.2 11C41 Safari 5.1

When moving the VoiceOver cursor through HTML content, contextual menus are not activating with the proper items.

Steps to Reproduce:
1) Visit a site such as news.google.com.
2) As VoiceOver moves around the page, press Control-Option-Shift-m to activate contextual menus.
2) Find a link and activate contextual menu, check the contents.
3) Find a header that is also a link, activate the contextual menu and check the contents of the menu.
4) Repeat using the mouse.

Actual Results: VoiceOver does not open the contextual menu properly, rather it displays a generic list of options found when a mouse right clicks on empty HTML space.

Expected Results: Contextual menu should display items relative to the object within the VO cursor, such as Open in New Window for links, etc.

[chris fleizach]

The problem appears because when we send our event into the context menu handler, we were assuming that it was a WK1 point and allowing the contentsToWIndow calls to transform it. On WK2 that doesn't work as expected because those methods are non-operations.

[chris fleizach]

Created attachment 110936 [details]
patch

[Darin Adler]

Comment on attachment 110936 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2581
> +    // To generate the correct point on WK2 we must account for sendContextMenuEvent transforming the point
> +    // from windowToContents (which adds the scroll offset). By pre-calling contentsToWindow (and removing the scroll offset)
> +    // the point sent through the WK2 event chain will be accurate.
> +    if (!frameView->platformWidget())
> +        clickPoint = frameView->contentsToWindow(clickPoint);

This doesn’t make sense. The sendContextMenuEvent function always transforms the point using windowToContents. That's not different between WebKit1 and WebKit2. So why would the code here need to be different between WebKit1 and WebKit2.

[chris fleizach]

(In reply to comment #3)
> (From update of attachment 110936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110936&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2581
> > +    // To generate the correct point on WK2 we must account for sendContextMenuEvent transforming the point
> > +    // from windowToContents (which adds the scroll offset). By pre-calling contentsToWindow (and removing the scroll offset)
> > +    // the point sent through the WK2 event chain will be accurate.
> > +    if (!frameView->platformWidget())
> > +        clickPoint = frameView->contentsToWindow(clickPoint);
> 
> This doesn’t make sense. The sendContextMenuEvent function always transforms the point using windowToContents. That's not different between WebKit1 and WebKit2. So why would the code here need to be different between WebKit1 and WebKit2.

I think the problem is that sendContextMenuEvent() DOES transform the point always, but on WK2 that transform is not needed. In order to continue using the stock sendContextMenuEvent (with the fake generated event from within accessibility) we need to undo that transformation so the rest of the machinery works as intended. 

I don't entirely understand the whole chain, but it appears that's what's happening. Note that AX is the only client of sendContextMenuEvent from within WebCore. The rest of clients come in through a different way

[Darin Adler]

Comment on attachment 110936 [details]
patch

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

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2581
>>> +        clickPoint = frameView->contentsToWindow(clickPoint);
>> 
>> This doesn’t make sense. The sendContextMenuEvent function always transforms the point using windowToContents. That's not different between WebKit1 and WebKit2. So why would the code here need to be different between WebKit1 and WebKit2.
> 
> I think the problem is that sendContextMenuEvent() DOES transform the point always, but on WK2 that transform is not needed. In order to continue using the stock sendContextMenuEvent (with the fake generated event from within accessibility) we need to undo that transformation so the rest of the machinery works as intended. 
> 
> I don't entirely understand the whole chain, but it appears that's what's happening. Note that AX is the only client of sendContextMenuEvent from within WebCore. The rest of clients come in through a different way

So then it seems we don’t need the if statement.

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 110936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110936&action=review
> 
> >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2581
> >>> +        clickPoint = frameView->contentsToWindow(clickPoint);
> >> 
> >> This doesn’t make sense. The sendContextMenuEvent function always transforms the point using windowToContents. That's not different between WebKit1 and WebKit2. So why would the code here need to be different between WebKit1 and WebKit2.
> > 
> > I think the problem is that sendContextMenuEvent() DOES transform the point always, but on WK2 that transform is not needed. In order to continue using the stock sendContextMenuEvent (with the fake generated event from within accessibility) we need to undo that transformation so the rest of the machinery works as intended. 
> > 
> > I don't entirely understand the whole chain, but it appears that's what's happening. Note that AX is the only client of sendContextMenuEvent from within WebCore. The rest of clients come in through a different way
> 
> So then it seems we don’t need the if statement.

I think we do, but I haven't been able to explain what I think is happening well.

When we call sendContextMenuEvent(), it does a windowToContents() call and then sends it off along its way. It seems on WK1 this is expected that the point for the context menu will be transformed thusly. That transform consists of doing a [view convertRect:] and then adding scroll offset.

On WK2, the point that the context menu hit test is assuming will be used does not expect the scroll offset to be applied. So this if check is basically subtracting the scroll offset (by using contentsToWindow()) before windowToContents() adds it for us.

In other words we could also do
if (isWK2()) 
 clickPoint -= scrollOffset();

[Darin Adler]

(In reply to comment #6)
> It seems on WK1 this is expected that the point for the context menu will be transformed thusly.

This is where you lost me. Either the point is in window coordinates or its in content coordinates. I don’t know what it means that “this is expect that the point will be transformed thusly”.

> On WK2, the point that the context menu hit test is assuming will be used does not expect the scroll offset to be applied.

Sorry, still not clear. You are saying that the “expectations” different in WebKit1 and WebKit2, and I understand that. But you are not saying why the expectations are different.

[chris fleizach]

*** Bug 69232 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 110936 [details]
patch

Anders, Sam, Simon, can one of you help figure out why the coordinates end up different between WebKit1 and WebKit2? There must be something obvious.

[Anders Carlsson]

(In reply to comment #9)
> (From update of attachment 110936 [details])
> Anders, Sam, Simon, can one of you help figure out why the coordinates end up different between WebKit1 and WebKit2? There must be something obvious.

Sure, I can take a look at it when I get back!

[Samuel White]

Are we sure this is a regression? Do we have evidence to suggest this wasn't present in WK1? The only AX layout test I see that deals with context menus is accessibility/editable-webarea-context-menu-point.html which wasn't designed to test the functionality being discussed.

I believe the problem is that the mouseEvent created in accessibilityShowContextMenu() is given coords to click using the AXObject clickPoint() function. These AXObject coords are provided with respect to the FrameView, so if the document has been scrolled clickPoint() can return y values larger than the height of the ViewPort. These coords makes sense in sendContextMenuEvent() when windowToContents() is called because this ensures that our target is inside our ViewPort. However, the actual mouseEvent that we fire needs to be updated with coords relative to the ViewPort before being fired.

I propose we add a function to AXObject that converts a clickPoint to be relative to the ViewPort. We can use the converted point as the x,y pos of the mouse event while keeping the original clickPoint intact as is needed by windowToContents() and other functionality. If others agree with this solution I'll work up a patch and a layout test to keep this functionality in check moving forward.

[chris fleizach]

(In reply to comment #11)
> Are we sure this is a regression? Do we have evidence to suggest this wasn't present in WK1? The only AX layout test I see that deals with context menus is accessibility/editable-webarea-context-menu-point.html which wasn't designed to test the functionality being discussed.
> 

definitely a regression

> I believe the problem is that the mouseEvent created in accessibilityShowContextMenu() is given coords to click using the AXObject clickPoint() function. These AXObject coords are provided with respect to the FrameView, so if the document has been scrolled clickPoint() can return y values larger than the height of the ViewPort. These coords makes sense in sendContextMenuEvent() when windowToContents() is called because this ensures that our target is inside our ViewPort. However, the actual mouseEvent that we fire needs to be updated with coords relative to the ViewPort before being fired.
> 
> I propose we add a function to AXObject that converts a clickPoint to be relative to the ViewPort. We can use the converted point as the x,y pos of the mouse event while keeping the original clickPoint intact as is needed by windowToContents() and other functionality. If others agree with this solution I'll work up a patch and a layout test to keep this functionality in check moving forward.

[chris fleizach]

Created attachment 178571 [details]
patch

Started over on this one. This is a problem that's similar to what we faced when calculating the position of elements in WK2

Essentially, we need to manually find a scroll view and covert the position. The attachment views in WK1 take care of this conversion, but without them on WK2, we need to convert manually.

[Ryosuke Niwa]

Comment on attachment 178571 [details]
patch

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

> Source/WebCore/ChangeLog:11
> +      

Nit: whitespaces.

[chris fleizach]

rdar://10015987

[chris fleizach]

http://trac.webkit.org/changeset/138989