Bug 76677 - AXPress event coordinates are always sent as (0, 0)
Summary: AXPress event coordinates are always sent as (0, 0)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Samuel White
URL:
Keywords: InRadar
: 13659 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-19 17:32 PST by Samuel White
Modified: 2016-10-18 22:25 PDT (History)
21 users (show)

See Also:


Attachments
First pass patch (6.55 KB, patch)
2012-01-19 18:01 PST, Samuel White
no flags Details | Formatted Diff | Diff
Patch. (9.17 KB, patch)
2012-02-20 01:48 PST, Samuel White
no flags Details | Formatted Diff | Diff
Patch. (8.97 KB, patch)
2012-02-20 10:29 PST, Samuel White
no flags Details | Formatted Diff | Diff
Fixed style patch. (9.48 KB, patch)
2012-02-24 11:12 PST, Samuel White
simon.fraser: review-
Details | Formatted Diff | Diff
Patch with review changes. (9.85 KB, patch)
2012-03-06 00:57 PST, Samuel White
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Build fix patch. (9.92 KB, patch)
2012-03-06 10:20 PST, Samuel White
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (9.40 KB, patch)
2013-08-14 13:57 PDT, Samuel White
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (816.03 KB, application/zip)
2013-08-16 06:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (463.81 KB, application/zip)
2013-08-16 18:02 PDT, Build Bot
no flags Details
Added screen coordinates to simulated click. (10.78 KB, patch)
2013-08-19 13:41 PDT, Samuel White
cfleizach: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (710.18 KB, application/zip)
2013-08-19 16:08 PDT, Build Bot
no flags Details
Updated patch. (11.70 KB, patch)
2013-08-20 13:12 PDT, Samuel White
simon.fraser: review-
Details | Formatted Diff | Diff
Updated patch. (11.55 KB, patch)
2013-10-16 11:42 PDT, Samuel White
simon.fraser: review+
Details | Formatted Diff | Diff
Final patch. (11.48 KB, patch)
2013-12-03 11:32 PST, Samuel White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel White 2012-01-19 17:32:08 PST
WebKit sends clientX and clientY coordinates as (0,0) when handling AXPress events.
Comment 1 Samuel White 2012-01-19 18:01:08 PST
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.
Comment 2 chris fleizach 2012-01-20 09:03:12 PST
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
Comment 3 chris fleizach 2012-01-20 09:03:56 PST
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
Comment 4 Samuel White 2012-02-20 01:48:53 PST
Created attachment 127783 [details]
Patch.

Thanks for the feedback. I think I've addressed your concerns in the following patch I'm proposing.
Comment 5 chris fleizach 2012-02-20 08:05:46 PST
This looks good to me. 

Beth, can you take a look at this to make sure it's OK as well
Comment 6 Samuel White 2012-02-20 10:29:42 PST
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.
Comment 7 WebKit Commit Bot 2012-02-23 19:05:14 PST
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.
Comment 8 Samuel White 2012-02-24 11:12:02 PST
Created attachment 128771 [details]
Fixed style patch.

Fixed style errors found by bots.
Comment 9 Eric Seidel (no email) 2012-02-24 11:30:05 PST
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?
Comment 10 chris fleizach 2012-02-24 11:32:16 PST
(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
Comment 11 Simon Fraser (smfr) 2012-02-24 11:43:53 PST
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.
Comment 12 Samuel White 2012-02-24 12:37:30 PST
(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.
Comment 13 Samuel White 2012-03-06 00:57:10 PST
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.
Comment 14 WebKit Review Bot 2012-03-06 02:01:23 PST
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
Comment 15 Samuel White 2012-03-06 10:20:54 PST
Created attachment 130400 [details]
Build fix patch.

Should fix the issue EWS found above.
Comment 16 WebKit Review Bot 2012-03-06 11:23:23 PST
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
Comment 17 Samuel White 2012-03-06 11:48:55 PST
Looks like nothing is passing Chromium Linux EWS at the moment. I'll wait until things clear up and submit again in the future.
Comment 18 Adam Barth 2012-03-06 13:33:11 PST
> 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.
Comment 19 Samuel White 2013-08-14 13:57:52 PDT
Created attachment 208759 [details]
Updated patch.
Comment 20 Samuel White 2013-08-14 14:00:03 PDT
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.
Comment 21 chris fleizach 2013-08-14 14:49:21 PDT
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
Comment 22 Build Bot 2013-08-16 06:16:25 PDT
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
Comment 23 Build Bot 2013-08-16 06:16:28 PDT
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
Comment 24 Build Bot 2013-08-16 18:02:49 PDT
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
Comment 25 Build Bot 2013-08-16 18:02:54 PDT
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
Comment 26 Samuel White 2013-08-19 13:41:29 PDT
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.
Comment 27 Ryosuke Niwa 2013-08-19 14:08:32 PDT
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?
Comment 28 chris fleizach 2013-08-19 15:25:17 PDT
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
Comment 29 James Craig 2013-08-19 16:00:22 PDT
<rdar://problem/10467360>
Comment 30 Build Bot 2013-08-19 16:08:49 PDT
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
Comment 31 Build Bot 2013-08-19 16:08:53 PDT
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
Comment 32 Samuel White 2013-08-20 13:12:59 PDT
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.
Comment 33 chris fleizach 2013-08-21 11:28:56 PDT
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
Comment 34 Simon Fraser (smfr) 2013-08-21 11:41:50 PDT
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.
Comment 35 James Craig 2013-09-30 11:18:29 PDT
*** Bug 13659 has been marked as a duplicate of this bug. ***
Comment 36 Samuel White 2013-10-16 11:42:09 PDT
Created attachment 214379 [details]
Updated patch.

Uploading revised patch to get an exhaustive list of failing tests that need to be updated.
Comment 37 Samuel White 2013-10-16 19:27:50 PDT
(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.
Comment 38 Simon Fraser (smfr) 2013-11-20 12:41:47 PST
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
Comment 39 Samuel White 2013-12-03 11:24:47 PST
(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.
Comment 40 Samuel White 2013-12-03 11:32:52 PST
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.
Comment 41 Samuel White 2013-12-03 12:58:56 PST
Committed in http://trac.webkit.org/changeset/160032

Closing.