Closed
Bug 969512
Opened 10 years ago
Closed 10 years ago
Don't use NS_MOUSEENTER/LEAVE in widget level code
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: smaug, Assigned: eeejay)
References
Details
Attachments
(2 files)
1.12 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
15.92 KB,
patch
|
kats
:
review+
yzen
:
review+
|
Details | Diff | Splinter Review |
http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=5a9badd6db00&mark=767-768,770-771#758 NS_MOUSEENTER/LEAVE are mapped to DOM mouseenter/leave which are closer to mouseover/out. NS_MOUSE_ENTER/EXIT are widget level stuff. I know, that is confusing.
Comment 1•10 years ago
|
||
CC'ing maxli who might want to work on this, based on wesj's comment on IRC that these events are used for accessibility purposes.
Reporter | ||
Comment 2•10 years ago
|
||
Ping about this. The bug is rather horrible from event handling point of view.
Comment 3•10 years ago
|
||
Is this all you're looking for? https://tbpl.mozilla.org/?tree=Try&rev=ff1e3544ea78
Assignee: nobody → bugmail.mozilla
Attachment #8487654 -
Flags: review?(bugs)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8487654 [details] [diff] [review] Patch Assuming ACTION_HOVER_ENTER means that pointer moves to be on top of the view/widget and ACTION_HOVER_EXIT means pointer leaving the view/widget, r+.
Attachment #8487654 -
Flags: review?(bugs) → review+
Comment 5•10 years ago
|
||
Yup. The try push was green but i want to check with accessibility folks before landing to make sure if any manual testing is required then we do it. eeejay, do you know if this code would be adequately tested by automated testing?
Flags: needinfo?(eitan)
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > Yup. The try push was green but i want to check with accessibility folks > before landing to make sure if any manual testing is required then we do it. > eeejay, do you know if this code would be adequately tested by automated > testing? It won't. let me give it a try on a device.
Flags: needinfo?(eitan)
Assignee | ||
Comment 7•10 years ago
|
||
From a quick check with the try build above, this does indeed break some accessibility features. The ACTION_HOVER motion events are emitted during explore by touch. ACTION_HOVER_ENTER/EXIT are emitted when a user puts their finger on the screen when TalkBack is active. We (partially) do our own gesture detection in our a11y JS stack, so we need a way to detect HOVER enter/exit/move, and translate that to one finger gestures. Right now we listen for mouseenter/mouseleave. I'll need to actually build this and play with it to figure this out. NIing myself so I won't forget.
Flags: needinfo?(eitan)
Reporter | ||
Comment 8•10 years ago
|
||
listening mouseenter/leave from JS is very much wrong. What is wrong with mouseover/out?
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #8) > listening mouseenter/leave from JS is very much wrong. > What is wrong with mouseover/out? I don't see those events being dispatched on a consistent basis with the current patch. A hover enter/exit could happen at any coordinate, it is not like a mouse that never leaves the screen and has a deterministic behavior of mouseover/out on each target. So when I test this patch, I first see mouseover for the XUL element, but there will never be a mouseout. There are mouseover events for different elements in the page, but there will only be a mouseout if the user actually moves their finger off of the element, not if they lift their finger.
Flags: needinfo?(eitan)
Comment 10•10 years ago
|
||
Not sure if you're still looking into comment 7 and accidentally removed the NI in comment 9.
Flags: needinfo?(eitan)
Assignee | ||
Comment 11•10 years ago
|
||
Make accessibility explore by touch hover events touch events. Flagged Yura for accessibility gesture detection changes, and kats for adnroid widget changes.
Attachment #8493232 -
Flags: review?(yzenevich)
Attachment #8493232 -
Flags: review?(bugmail.mozilla)
Comment 12•10 years ago
|
||
Comment on attachment 8493232 [details] [diff] [review] Don't use NS_MOUSEENTER/LEAVE in widget level code. Review of attachment 8493232 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: accessible/jsat/PointerAdapter.jsm @@ +122,5 @@ > + screenX: changedTouches[0].screenX + 5, > + screenY: changedTouches[0].screenY + 5, > + target: changedTouches[0].target > + }, changedTouches[0]]; > + } This looks good and seems to work. The only concern I have is that, we were only adding second point in case of rejection to Swipe... ::: accessible/jsat/content-script.js @@ -86,5 @@ > sendAsyncMessage('AccessFu:DoScroll', > { bounds: Utils.getBounds(position, true), > page: aMessage.json.page, > horizontal: aMessage.json.horizontal }); > - sendScrollCoordinates(position); Good catch.
Attachment #8493232 -
Flags: review?(yzenevich) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8493232 [details] [diff] [review] Don't use NS_MOUSEENTER/LEAVE in widget level code. Review of attachment 8493232 [details] [diff] [review]: ----------------------------------------------------------------- mobile/ stuff looks ok to me except for the comment below ::: mobile/android/base/GeckoEvent.java @@ +514,5 @@ > mPointRadii[index].x /= zoom; > mPointRadii[index].y /= zoom; > } > mPressures[index] = event.getPressure(eventIndex); > + mToolTypes[index] = event.getToolType(index); getToolType is only available in API version >= 14, we would need a fallback or use an API that is compatible with older versions
Attachment #8493232 -
Flags: review?(bugmail.mozilla) → review+
Updated•10 years ago
|
Assignee: bugmail.mozilla → eitan
Updated•10 years ago
|
Flags: needinfo?(eitan)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Comment on attachment 8493232 [details] [diff] [review] > Don't use NS_MOUSEENTER/LEAVE in widget level code. > > Review of attachment 8493232 [details] [diff] [review]: > ----------------------------------------------------------------- > > mobile/ stuff looks ok to me except for the comment below > > ::: mobile/android/base/GeckoEvent.java > @@ +514,5 @@ > > mPointRadii[index].x /= zoom; > > mPointRadii[index].y /= zoom; > > } > > mPressures[index] = event.getPressure(eventIndex); > > + mToolTypes[index] = event.getToolType(index); > > getToolType is only available in API version >= 14, we would need a fallback > or use an API that is compatible with older versions There are no HOVER events before API 14, so it shouldn't be an issues. I'll just not touch mToolTypes on older versions.
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b81989aa76
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/84b81989aa76
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•