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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: smaug, Assigned: eeejay)

References

Details

Attachments

(2 files)

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.
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.
Ping about this. The bug is rather horrible from event handling point of view.
Attached patch PatchSplinter Review
Is this all you're looking for?

https://tbpl.mozilla.org/?tree=Try&rev=ff1e3544ea78
Assignee: nobody → bugmail.mozilla
Attachment #8487654 - Flags: review?(bugs)
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+
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)
(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)
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)
listening mouseenter/leave from JS is very much wrong.
What is wrong with mouseover/out?
(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)
Not sure if you're still looking into comment 7 and accidentally removed the NI in comment 9.
Flags: needinfo?(eitan)
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 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 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+
Assignee: bugmail.mozilla → eitan
Flags: needinfo?(eitan)
(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.
https://hg.mozilla.org/mozilla-central/rev/84b81989aa76
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: