Closed Bug 1232338 Opened 4 years ago Closed 4 years ago

[parity] Hover from input devices does not get passed on to page CSS/javascript

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
fennec + ---

People

(Reporter: bugs, Assigned: rbarker, Mentored)

References

Details

(Whiteboard: [mentor project][lang=java])

Attachments

(2 files, 2 obsolete files)

In Chrome, hovering over parts of the page with the samsung s-pen (stylus) causes hover events to be fired for example on dropdown menus.  Firefox has always ignored this.  Bug #1230838 was related to hover issues, and restored the old Firefox lack-of-hover - it seemed a good time to request better hover support.  Hovering over some parts of the Firefox UI does cause hover stuff to happen (like revealing tab count) - it just doesn't do anything when hovering over the web page.

This presumably impacts:
* samsung s-pen (galaxy note tablets/smartphones)
* bluetooth/USB mice
* samsung airview (bug #1013899)

This would just be basic pass-the-events-on-to-the-page that would be nice to have.
Other things that would be nice though would be gestures (bug #808591) and not using as large a touch area for other input devices (bug #1213756). non-capacitive styluses and mice are much more precise.
tracking-fennec: --- → ?
One odd thing I did notice... when hovering over links on  Hacker News with the stylus in nightly and stable, an underline appeared - some hover stuff does make it to the page?  Although in nightly, unlike stable, it underlined offset by a url bar's worth of screen from where the point was if the url bar was visible.  

Curious, I tried some other stuff.

http://ianlunn.github.io/Hover/  kinda worked in stable.  There seemed to be lag, and stuff would get stuck in a particular state even after the pointer had moved.  By contrast, in Nightly nothing seemed to happen at all although occasionally on of the effects seemed to randomly wiggle a bit.

I then retested http://csswizardry.com/demos/css-dropdown/ which I'd used in writing up the other bug and it worked in stable! so did some other pure css menus I'd tried.  They do *not* work in Nightly though,  so this is a regression.

I eventually figured out why I thought they didn't work in Firefox.  I had been testing in landscape mode before with usual zoomed out sites.  I think Firefox' less-precise stylus pointer (previously mentioned bug on that already) was resulting in the events not being quite where I thought they were.  Even though I saw the stylus point exactly over the menu, it did not trigger until I zoomed in.  Nightly doesn't work at all though.

So this is probably more of a regression, although kind of also parity since the hover never worked that reliably.
Ok. I see. The pure CSS menu demo *does* work in Nightly if I kinda work around it by zooming in until scrolling is possible, then swiping down until the URL bar vanishes.  Must be another manifestation of the offset bug.  Was kinda hard to trigger the hover since the zoomed in page was triggering the scroll up arrow whn I got close to the edge where the menu was.  Curiously, Nightly rendered a URL bar's worth of black space if the scroll triggered.

 The other hover effects demo page still fails in Nightly though - maybe due to using javascript instead of CSS?
Seems like there may be a few things going on here:
  * We had a tap offset issue (bug 1232893)
  * I wonder if the hover changes are related to APZ (bug 1206872) – NI kats

Kevin, do you have anything to add here?
Flags: needinfo?(kbrosnan)
Flags: needinfo?(bugmail.mozilla)
It's not directly related to APZ - there was some hover-related breakage which we fixed in bug 1230838; this bug is a spinoff from that. However, APZ being enabled could only have made this worse, and it's definitely something I think we should fix. If we have a device with a stylus we can ship it to rbarker, since he's was planning on taking bug 1225936 which is in the same general vicinity.

That being said, I wouldn't block APZ riding the trains on this bug. As APZ is the higher priority project right now and we have limited resources, this will probably have to wait unless somebody finds some spare cycles to pick it up.
Flags: needinfo?(bugmail.mozilla)
See Also: → 1225936
This is reproducible using a bluetooth mouse and Fennec. While working on wheel events I noticed that the hover events were off by a certain offset.
Say, rbarker, can you reproduce the other issues I had?
That is:
http://ianlunn.github.io/Hover/  doesn't seem to work in nightly even if I scroll down to make the url bar vanish to avoid the offset problem.  Or even if I position the pointer with the url visible by a compensated distance.

Also, pretty much any hover menu out there doesn't work reliably in nightly or stable if zoomed out - I'm guessing that's possibly due to bug #1213756 .
Not much to add here. I thought we had an older bug about supporting the s-pen hover state but I cannot find it.
Flags: needinfo?(kbrosnan)
Mentor: snorp
tracking-fennec: ? → +
Whiteboard: [mentor project][lang=java]
Assignee: nobody → rbarker
Duplicate of this bug: 1247121
Depends on: 1242690
Please note that this patch will not work without bug 1242690
Blocks: 1248090
Attachment #8720559 - Flags: review?(bugmail.mozilla)
Attachment #8720560 - Flags: review?(nchen)
Comment on attachment 8720559 [details] [diff] [review]
0001-Bug-1232338-part-1-Add-better-mouse-support-to-InputData-MouseInput-16021716-ad3417d.patch

Review of attachment 8720559 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/InputData.cpp
@@ +32,5 @@
>    : InputData(MOUSE_INPUT, aMouseEvent.time, aMouseEvent.timeStamp,
>                aMouseEvent.modifiers)
> +  , mType(MOUSE_NONE)
> +  , mButtonType(NONE)
> +  , mButtons(aMouseEvent.buttons)

As far as I can tell this constructor is never actually used anywhere (not even in your part 2 patch) and the constructor you *do* use always uses 0 for the "buttons" parameter, so I'm not sure why that's needed. Can we just leave it out? It seems redundant with the mButtonType anyway.

::: widget/InputData.h
@@ +271,5 @@
>      RIGHT_BUTTON,
>      NONE
>    };
>  
> +  MouseInput(MouseType aType, ButtonType aButtonType, int16_t aButtons, ScreenPoint& aPoint,

make |aPoint| a const ScreenPoint&
Comment on attachment 8720560 [details] [diff] [review]
0002-Bug-1232338-part-2-Fix-mouse-hoover-events-to-have-correct-refPoint-value-when-C-APZ-is-enabled-16021716-f4e1cfb.patch

Review of attachment 8720560 [details] [diff] [review]:
-----------------------------------------------------------------

s/hoover/hover/ in the commit message.

This looks ok to me, but it might break the accessibility code, as that relies on special handling of hover events. I'm not entirely sure how it works but it would be worth testing to make sure that basic functionality doesn't regress with accessibility turned on.

::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
@@ +272,5 @@
>              // native code because it's just going to crash
>              return true;
>          }
> +
> +        event.offsetLocation(0, -mSurfaceTranslation);

This should probably move up to before the AndroidGamepadManager call.
Comment on attachment 8720559 [details] [diff] [review]
0001-Bug-1232338-part-1-Add-better-mouse-support-to-InputData-MouseInput-16021716-ad3417d.patch

Review of attachment 8720559 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping review pending response to comment 14.
Attachment #8720559 - Flags: review?(bugmail.mozilla)
Attachment #8720560 - Flags: review?(nchen) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Comment on attachment 8720560 [details] [diff] [review]
> 0002-Bug-1232338-part-2-Fix-mouse-hoover-events-to-have-correct-refPoint-
> value-when-C-APZ-is-enabled-16021716-f4e1cfb.patch
> 
> Review of attachment 8720560 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks ok to me, but it might break the accessibility code, as that
> relies on special handling of hover events. I'm not entirely sure how it
> works but it would be worth testing to make sure that basic functionality
> doesn't regress with accessibility turned on.
> 

Are you referring to Talk Back? I tested it and Talk Back appears to still work.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 8720559 [details] [diff] [review]
> 0001-Bug-1232338-part-1-Add-better-mouse-support-to-InputData-MouseInput-
> 16021716-ad3417d.patch
> 
> Review of attachment 8720559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/InputData.cpp
> @@ +32,5 @@
> >    : InputData(MOUSE_INPUT, aMouseEvent.time, aMouseEvent.timeStamp,
> >                aMouseEvent.modifiers)
> > +  , mType(MOUSE_NONE)
> > +  , mButtonType(NONE)
> > +  , mButtons(aMouseEvent.buttons)
> 
> As far as I can tell this constructor is never actually used anywhere (not
> even in your part 2 patch) and the constructor you *do* use always uses 0
> for the "buttons" parameter, so I'm not sure why that's needed. Can we just
> leave it out? It seems redundant with the mButtonType anyway.
> 

I didn't add the constructor from a WidgetMouseBaseEvent, it just bothered me that not all of the member variables were being initialized so I fixed it, but if it isn't being used I can remove the constructor.

As for the button stuff, I added it because we were forcing a left button click for widget enter and exit mouse move events. I asked :snorp why and he wasn't sure. I looked at the history and it was there when the nsWindow.cpp was added. So, I left it out and it didn't seem to break anything that I could see. I can remove the button stuff if you prefer.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Comment on attachment 8720560 [details] [diff] [review]
> 0002-Bug-1232338-part-2-Fix-mouse-hoover-events-to-have-correct-refPoint-
> value-when-C-APZ-is-enabled-16021716-f4e1cfb.patch
> 
> Review of attachment 8720560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> s/hoover/hover/ in the commit message.
> 
> This looks ok to me, but it might break the accessibility code, as that
> relies on special handling of hover events. I'm not entirely sure how it
> works but it would be worth testing to make sure that basic functionality
> doesn't regress with accessibility turned on.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
> @@ +272,5 @@
> >              // native code because it's just going to crash
> >              return true;
> >          }
> > +
> > +        event.offsetLocation(0, -mSurfaceTranslation);
> 
> This should probably move up to before the AndroidGamepadManager call.

I wasn't sure, Gamepad events don't actually have a location do they?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 8720559 [details] [diff] [review]
> 0001-Bug-1232338-part-1-Add-better-mouse-support-to-InputData-MouseInput-
> 16021716-ad3417d.patch
> 
> Review of attachment 8720559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/InputData.cpp
> @@ +32,5 @@
> >    : InputData(MOUSE_INPUT, aMouseEvent.time, aMouseEvent.timeStamp,
> >                aMouseEvent.modifiers)
> > +  , mType(MOUSE_NONE)
> > +  , mButtonType(NONE)
> > +  , mButtons(aMouseEvent.buttons)
> 
> As far as I can tell this constructor is never actually used anywhere (not
> even in your part 2 patch) and the constructor you *do* use always uses 0
> for the "buttons" parameter, so I'm not sure why that's needed. Can we just
> leave it out? It seems redundant with the mButtonType anyway.

Sorry, I misunderstood what you were asking earlier. So mButtonType is set when a particular button changes state, while mButtons is a mask that indicates which buttons are currently pressed, so it isn't redundant and actually without it, MouseInput potentially creates a WidgetMouseEvent with contradictory information. I can remove it but then we should remove mButtonType as well.
I really appreciate you guys looking into this, even if I'm currently on 38 again due to issues with touch navigation/stability.
Just thought I'd mention one more thing that you might already be looking into.  Clicking the button on the stylus acts as a right click in things like XSDL or bVNC.  It can be handy, even if it is a bit finicky due to clicking when not touching the screen triggering the air click menu.  Bluetooth mice would have this anyway.  I'm not sure if it would act any differently from longtouch, or could just be hooked into it.  Right click context menu.
(In reply to Randall Barker [:rbarker] from comment #17)
> Are you referring to Talk Back? I tested it and Talk Back appears to still
> work.

Yeah, that's what I was referring to. Thanks for checking!

(In reply to Randall Barker [:rbarker] from comment #18)
> I didn't add the constructor from a WidgetMouseBaseEvent, it just bothered
> me that not all of the member variables were being initialized so I fixed
> it, but if it isn't being used I can remove the constructor.

No, it's fine to leave that constructor, and thanks for initializing the member variables. That's not what I was complaining about, but I realize now that I misread the code. I thought the MouseInput was getting created for non-hover events as well and so passing in 0 for mButtons would be wrong but in fact it's ok since the only events you create are hover/enter/exit events. So yeah the patch is fine in that respect.

(In reply to Randall Barker [:rbarker] from comment #19)
> > 
> > This should probably move up to before the AndroidGamepadManager call.
> 
> I wasn't sure, Gamepad events don't actually have a location do they?

I'm not sure - but they might get one in the future even if they don't know. It's safer to just move the call now.
https://hg.mozilla.org/mozilla-central/rev/ec3026e8cd6f
https://hg.mozilla.org/mozilla-central/rev/d297ed97eaa1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.