Summary: | AXPress event coordinates are always sent as (0, 0) | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Samuel White <samuel_white> | ||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Samuel White <samuel_white> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, aboxhall, apinheiro, bdakin, buildbot, cfleizach, cmarcelo, commit-queue, dglazkov, dmazzoni, eric.slosser, eric, esprehn+autocc, jcraig, jdiggs, kangil.han, mario, rniwa, simon.fraser, webkit-bug-importer, webkit.review.bot | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||||
OS: | OS X 10.6 | ||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=163648 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Samuel White
2012-01-19 17:32: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 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
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 Created attachment 127783 [details]
Patch.
Thanks for the feedback. I think I've addressed your concerns in the following patch I'm proposing.
This looks good to me. Beth, can you take a look at this to make sure it's OK as well 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.
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.
Created attachment 128771 [details]
Fixed style patch.
Fixed style errors found by bots.
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? (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 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. (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. 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 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 Created attachment 130400 [details]
Build fix patch.
Should fix the issue EWS found above.
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 Looks like nothing is passing Chromium Linux EWS at the moment. I'll wait until things clear up and submit again in the future. > 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.
Created attachment 208759 [details]
Updated patch.
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 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 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 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 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 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
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 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 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 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 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
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 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 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. *** Bug 13659 has been marked as a duplicate of this bug. *** Created attachment 214379 [details]
Updated patch.
Uploading revised patch to get an exhaustive list of failing tests that need to be updated.
(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 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 (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. 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.
Committed in http://trac.webkit.org/changeset/160032 Closing. |