AXPress event coordinates are always sent as (0, 0)

[Samuel White]

WebKit sends clientX and clientY coordinates as (0,0) when handling AXPress events.

[Samuel White]

Created attachment 123231 [details]
First pass patch

I've spun my wheels a bit attacking this problem from a few different directions. I've come to believe the most elegant solution is to handle the AXPress case directly in the event dispatcher. I've attached a rough patch in hopes that someone might have some good feedback. Thanks.

[chris fleizach]

Comment on attachment 123231 [details]
First pass patch

I think we want to modify all simulated clicks to send the mid point of the object in question. Just doing it when AX is enabled is not a great check, because then we have different behavior whether VO is on or off which leads to all kinds of confusion.
I think that SimulatedMouseClick that's already dispatched should take some coordinates. You can probably copy enough of the logic in the click point routine to get the point you want for an object

[chris fleizach]

I also like using the shouldBe paradigm from the js-test methods because it provides a standardized way for asserting things should be true or false

[Samuel White]

Created attachment 127783 [details]
Patch.

Thanks for the feedback. I think I've addressed your concerns in the following patch I'm proposing.

[chris fleizach]

This looks good to me. 

Beth, can you take a look at this to make sure it's OK as well

[Samuel White]

Created attachment 127831 [details]
Patch.

Removed screen coordinates from the test as these values will differ depending on where the test window is positioned and seem likely to cause this test to fail. For example, the test would have failed if you ran it manually since you could position your window anywhere. Since screen coordinates are based on client coordinates I think this change is ok and doesn't make the test any less valid.

[WebKit Commit Bot]

Attachment 127831 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/dom/MouseEvent.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/dom/MouseEvent.h:106:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:110:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 7 files

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/MouseEvent.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/dom/MouseEvent.h:106:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:110:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel White]

Created attachment 128771 [details]
Fixed style patch.

Fixed style errors found by bots.

[Eric Seidel (no email)]

How do simulated mouse events arrise?  What is the "underlyingEvent"?  I'm assuming there is no actual mouse coordinates on the event, which is why we use the middle of the target element?

[chris fleizach]

(In reply to comment #9)
> How do simulated mouse events arrise?  What is the "underlyingEvent"?  I'm assuming there is no actual mouse coordinates on the event, which is why we use the middle of the target element?

They arise from things like accessibility clients asking to "press" on an element. It will then simulate a mouse click in order to accomplish that.

There are no mouse coordinates otherwise available

[Simon Fraser (smfr)]

Comment on attachment 128771 [details]
Fixed style patch.

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

> Source/WebCore/dom/MouseEvent.cpp:184
> +        int clientX = rect.location().x() + rect.size().width() / 2;
> +        int clientY = rect.location().y() + rect.size().height() / 2;

absoluteBoundingBoxRect is document-relative. clientX and clientY should be viewport-relative, given their names.

This will break in subframes, CSS-transformed subframes, and scrolled documents. All these cases should be tested.

> Source/WebCore/dom/MouseEvent.cpp:186
> +        m_screenLocation = IntPoint(clientX + domWindow->screenX(), clientY + domWindow->screenY());

I don't think that's a good way to convert to screen coordinates.

It would be best to use the methods on Widget.h to convert to the containing window. Not sure how best to go from there to the screen.

> LayoutTests/ChangeLog:6
> +        Make sure AXPress events target an elements center point.

element's

> LayoutTests/platform/mac/accessibility/press-event-has-target-coordinates.html:8
> +<button id="button" style="height:30px; width:60px;">Submit</button>

It would be nice to test with a CSS-transformed element too.

[Samuel White]

(In reply to comment #9)
> How do simulated mouse events arrise?  What is the "underlyingEvent"?  I'm assuming there is no actual mouse coordinates on the event, which is why we use the middle of the target element?

The underlying event may be a mouse event, in which case we use the coords associated with that event. But as Chris mentioned screen reader press events have no associated underlying mouse event so in this case it is most appropriate to use the elements center.

[Samuel White]

Created attachment 130326 [details]
Patch with review changes.

I've addressed Simon's concerns in the following patch which is now CSS transform aware and includes a test for this. I also reevaluated how screenX & screenY are computed as Simon suggested but in the end decided to leave that part alone for two reasons. First, I couldn't find any case that wasn't supported by the current implementation and secondly, I tried to stay away from platform dependence where possible. Let me know what you think and thanks again for any feedback.

[WebKit Review Bot]

Comment on attachment 130326 [details]
Patch with review changes.

Attachment 130326 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11836243

[Samuel White]

Created attachment 130400 [details]
Build fix patch.

Should fix the issue EWS found above.

[WebKit Review Bot]

Comment on attachment 130400 [details]
Build fix patch.

Attachment 130400 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11805008

New failing tests:
fast/forms/mailto/formenctype-attribute-input-html.html
fast/events/stopPropagation-submit.html
fast/forms/mailto/formenctype-attribute-button-html.html

[Samuel White]

Looks like nothing is passing Chromium Linux EWS at the moment. I'll wait until things clear up and submit again in the future.

[Adam Barth]

> Looks like nothing is passing Chromium Linux EWS at the moment. I'll wait until things clear up and submit again in the future.

There are always some failing tests.  The ones posted in the comment should just be the ones that are new after your patch.

[Samuel White]

Created attachment 208759 [details]
Updated patch.

[Samuel White]

I've attached an updated patch (since this issue has been dormant for so long). I'm still having some trouble computing screen coordinates for the simulated mouse event. See the FIXME line in the patch. Any help here would be great! Thanks.

[chris fleizach]

Comment on attachment 208759 [details]
Updated patch.

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

> LayoutTests/platform/mac/accessibility/press-targets-center-point.html:10
> +<button id="button" style="-webkit-transform:translate(240px, 240px); height:30px; width:48px;">Press</button>

you should also have a non transformed case

> LayoutTests/platform/mac/accessibility/press-targets-center-point.html:21
> +    actual.innerHTML = "Waiting for press";

this seems unncessary

> LayoutTests/platform/mac/accessibility/press-targets-center-point.html:36
> +        button.focus();

you don't need to focus first, you can just call accessibilityController.accessibleElementById("id")

> Source/WebCore/dom/MouseEvent.cpp:263
> +SimulatedMouseEvent::SimulatedMouseEvent(const AtomicString& eventType, PassRefPtr<AbstractView> view, PassRefPtr<Event> underlyingEvent, PassRefPtr<Element> targetElement)

I don't think Element needs to be a pass ref ptr, because you don't need to maintain a reference to that object beyond its creation.

> Source/WebCore/dom/MouseEvent.cpp:286
> +        m_screenLocation = IntPoint(0, 0); // FIXME: m_screenLocation should be set here.

there's a screenRect() method on Element that might help

[Build Bot]

Comment on attachment 208759 [details]
Updated patch.

Attachment 208759 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1473438

New failing tests:
fast/forms/mailto/formenctype-attribute-input-html.html
fast/events/stopPropagation-submit.html
fast/forms/mailto/formenctype-attribute-button-html.html

[Build Bot]

Created attachment 208921 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 208759 [details]
Updated patch.

Attachment 208759 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1465853

New failing tests:
fast/forms/mailto/formenctype-attribute-input-html.html
fast/events/stopPropagation-submit.html
fast/forms/mailto/formenctype-attribute-button-html.html

[Build Bot]

Created attachment 208969 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Samuel White]

Created attachment 209117 [details]
Added screen coordinates to simulated click.

Also addressed feedback from Chris.

I believe two of the failing tests are false positives. Both "formenctype-attribute-input-html" and "formenctype-attribute-button-html" were previously listing the originator of 'form submitted' as "HTML > #document". This patch changes that to "INPUT > FORM > BODY > HTML > #document" which I would think is more accurate.

[Ryosuke Niwa]

Comment on attachment 209117 [details]
Added screen coordinates to simulated click.

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

> Source/WebCore/dom/MouseEvent.cpp:286
> +        IntRect targetScreenRect = targetElement->screenRect();
> +        int targetScreenX = static_cast<int>(targetScreenRect.x() + targetScreenRect.width() / 2);
> +        int targetScreenY = static_cast<int>(targetScreenRect.y() + targetScreenRect.height() / 2);

Can we add a test to make sure this code works in RTL and vertical writing mode?

[chris fleizach]

Comment on attachment 209117 [details]
Added screen coordinates to simulated click.

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

> LayoutTests/platform/mac/accessibility/press-targets-center-point.html:9
> +<button id="button1" style="height:30px; width:48px;">Press</button>

The name of the button 'press' can reflect what it's testing -- 'transformed element', 'right to left' etc, etc, for easier results reading when something fails in the future

[James Craig]

<rdar://problem/10467360>

[Build Bot]

Comment on attachment 209117 [details]
Added screen coordinates to simulated click.

Attachment 209117 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1503669

New failing tests:
fast/forms/mailto/formenctype-attribute-input-html.html
fast/events/stopPropagation-submit.html
fast/forms/mailto/formenctype-attribute-button-html.html

[Build Bot]

Created attachment 209133 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Samuel White]

Created attachment 209222 [details]
Updated patch.

Added vertical writing mode and right-to-left direction tests. Are there any other cases we should be testing? Thanks.

[chris fleizach]

Comment on attachment 209222 [details]
Updated patch.

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

> Source/WebCore/dom/MouseEvent.cpp:286
> +        int targetScreenY = static_cast<int>(targetScreenRect.y() + targetScreenRect.height() / 2);

There's a center() method on IntRect that you should be able to use directly

[Simon Fraser (smfr)]

Comment on attachment 209222 [details]
Updated patch.

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

> Source/WebCore/dom/MouseEvent.cpp:289
> +        // Handle flipped screen coordinates on Mac.
> +        targetScreenY = static_cast<int>(targetElement->document()->domWindow()->screen()->height() - targetScreenY);

For events, this mapping uses NSPoint flipScreenPoint(const NSPoint& screenPoint, NSScreen *screen)

I don't think your code handles screens whose Y offset is not zero.

I'd prefer this screen flipping code to be shared with the code that initializes events.

[James Craig]

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

[Samuel White]

Created attachment 214379 [details]
Updated patch.

Uploading revised patch to get an exhaustive list of failing tests that need to be updated.

[Samuel White]

(In reply to comment #34)
> (From update of attachment 209222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209222&action=review
> 
> > Source/WebCore/dom/MouseEvent.cpp:289
> > +        // Handle flipped screen coordinates on Mac.
> > +        targetScreenY = static_cast<int>(targetElement->document()->domWindow()->screen()->height() - targetScreenY);
> 
> For events, this mapping uses NSPoint flipScreenPoint(const NSPoint& screenPoint, NSScreen *screen)
> 
> I don't think your code handles screens whose Y offset is not zero.
> 
> I'd prefer this screen flipping code to be shared with the code that initializes events.

Newest patch uses a more robust coordinate flipping solution in the simulated event init as suggested. Thanks for the feedback.

[Simon Fraser (smfr)]

Comment on attachment 214379 [details]
Updated patch.

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

> Source/WebCore/dom/Element.cpp:1004
> +        return document().view()->contentsToRootView(renderer->absoluteBoundingBoxRect());

Does this do the right thing if the root view is scrolled?

> Source/WebCore/dom/Element.cpp:1011
> +    if (RenderObject* renderer = this->renderer())
> +        return document().view()->contentsToScreen(renderer->absoluteBoundingBoxRect());

Is this actually fixing a bug? Should you add a test for it?

> Source/WebCore/dom/Element.h:226
> +    

Whitespace

[Samuel White]

(In reply to comment #38)
> (From update of attachment 214379 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214379&action=review
> 
> > Source/WebCore/dom/Element.cpp:1004
> > +        return document().view()->contentsToRootView(renderer->absoluteBoundingBoxRect());
> 
> Does this do the right thing if the root view is scrolled?

Good thought, it does.

> 
> > Source/WebCore/dom/Element.cpp:1011
> > +    if (RenderObject* renderer = this->renderer())
> > +        return document().view()->contentsToScreen(renderer->absoluteBoundingBoxRect());
> 
> Is this actually fixing a bug? Should you add a test for it?

Not a bug fix per say, just FIXME cleanup (moving over to the more robust method). Surprisingly, we only have three existing call sites for this method and none of the associated layout tests suffer.

> 
> > Source/WebCore/dom/Element.h:226
> > +    
> 
> Whitespace

Fixed.

[Samuel White]

Created attachment 218317 [details]
Final patch.

Checking EWS again to be safe. Some time has passed since this patch was originally uploaded. Will commit manually if all goes well. Thanks.

[Samuel White]

Committed in http://trac.webkit.org/changeset/160032

Closing.