Closed Bug 915213 Opened 11 years ago Closed 11 years ago

Transform all gecko input through the apz if zoom is applied

Categories

(Core Graveyard :: Widget: WinRT, defect)

26 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(8 files, 6 obsolete files)

3.55 KB, text/html
Details
216.76 KB, image/png
Details
35.45 KB, image/png
Details
2.79 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.25 KB, patch
kats
: review+
Details | Diff | Splinter Review
15.84 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
13.11 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.64 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Spin off from bug 914829 - all gecko input with a refPoint needs to be transformed by the apz before being dispatched to content. Pretty sure this includes simple gestures, mouse and wheel input, and touch.
Attached file draw.html (obsolete) —
Assignee: nobody → jmathies
Attached file test case (obsolete) —
added mouse input tracking
Attachment #803122 - Attachment is obsolete: true
Attached file test case
adjust for scroll
Attachment #803127 - Attachment is obsolete: true
Attachment #803131 - Attachment is patch: false
Attachment #803131 - Attachment mime type: text/plain → text/html
This is turning out to be harder than I expected. Touch input is covered by existing logic we have, but other events like mouse should only be transformed if the event is destined for content. So for example if I transform mousedown, mouseup, and mousemove unconditionally before delivery, chrome elements like the overlay nav bars and options panel don't get non-transformed ref points, and as such don't receive clicks.

It appears we'll need to do some sort of chrome/content hit testing, though I'm not sure of the best way to go about that.
Attached patch wip (obsolete) — Splinter Review
Attachment #803753 - Attachment is patch: true
Actually touch also has the same problem since we currently aren't using the transformed output from APZCTreeManager::ReceiveInputEvent. Just like mouse input we can't use those points unconditionally if the event is destined for chrome.
Blocks: 915724
No longer blocks: metro-apzc
Attached patch wip v.2 (obsolete) — Splinter Review
New rev that uses APZCTreeManager::ReceiveInputEvent instead of a new entry point. I'm not a fan of this honestly since we have to duplicate the event data just to get transformed ref point.

Also, afaict, the tree manager / controllers are not doing hit testing in the dom. No matter what's below the event coords ReceiveInputEvent always seems to transform the value.
Looks like the hit testing we use is based on visible rects - 

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.h#657

which won't work reliably for metro. I think we're going to have to send an event up to the dom to find out if the event should be transformed or not.
Can you turn on layer borders (layers.draw-borders config option) and take a screenshot of the screen where the hit detection is going wrong? I would expect the chrome bits to have separate layers that are overlaid on top of the scrollable content layer, but I would like to confirm that. We might need to adjust the hit testing code in APZCTreeManager (or more specifically, the mVisibleRegion stored on the APZC) to account for this. I used to have it store a region rather than a rect but I took that code out because I wasn't sure it would ever be used.
Attached image overlay button
Buttons like this won't get picked up right with rects. Not sure about regions. I think I can solve this with a bit of hit testing before I deliver the event to apz/dom.
I don't see any layer borders on that screenshot which is surprising if you turned it on.

But yeah I guess for now you can do some hit testing in the metro code before passing the event to APZC but eventually I would like that to all live in the APZC code. Will need to think about that more first.
Attached image layers.draw-borders
Hm, ok, thanks. So if we ever move to doing this in APZC we'll have to take into account transparent areas on layers which is probably non-trivial. :( Otherwise that entire middle bar will go to the overlay layer and that will make touch events starting in that region not go to content.
I have a better fix to this in the works which involves adding a new ReceiveInputEvent that takes a single non-const nsInputEvent and transforms the event in place. I need to clean up up since there's a lot of duplicate code. Also i ad to turn off the special processing of mouse events (#ifdef'd to unix only) so I could pass mouse events in for transformation. I like this approach the most, it avoids all the data copying the other ReceiveInputEvent does.

However using apz transformed coords for everything exposes a new problem with the composition rect bounds(?) where panning vertically causes the page to accumulate a horizontal movement that causes the page to jitter out of control after a short pan. If I hardcode the x displacement to 0 in TrackTouch the problem goes away.

Looking at MXR, this area of the apz recent went through some pretty big changes, so I'm going to do a pull / rebuild and see how things look on mc tip.
Blocks: 918288
Depends on: 918273
Attached patch scroll offset (obsolete) — Splinter Review
This addresses the horizontal pan jitter I've been seeing.
Attachment #803753 - Attachment is obsolete: true
Attachment #806090 - Attachment is obsolete: true
Useful from metro widget where we always call here on the main thread, and have no interest in duplicating touch events.
Blocks: 918273
No longer depends on: 918273
Comment on attachment 807255 [details] [diff] [review]
break up ReceiveInputEvent so it can be reused

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +295,5 @@
>    return result;
>  }
>  
> +AsyncPanZoomController*
> +APZCTreeManager::GetTouchInputBlockAPZ(const nsTouchEvent& aEvent, ScreenPoint aPoint)

s/APZ/APZC/, here and everywhere

@@ +299,5 @@
> +APZCTreeManager::GetTouchInputBlockAPZ(const nsTouchEvent& aEvent, ScreenPoint aPoint)
> +{
> +  nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aPoint);
> +  gfx3DMatrix transformToApzc, transformToScreen;
> +  // Reset the cahced apz transform

typo: cached

@@ +379,5 @@
> +APZCTreeManager::ProcessEvent(const nsInputEvent& aEvent,
> +                              nsInputEvent* aOutEvent)
> +{
> +  // Transform the refPoint
> +  nsInputEvent& oldEvent = const_cast<nsInputEvent&>(aEvent);

oldEvent doesn't appear to be used, remove it? I also don't like const_cast because it means we're doing something wrong somewhere.

@@ +380,5 @@
> +                              nsInputEvent* aOutEvent)
> +{
> +  // Transform the refPoint
> +  nsInputEvent& oldEvent = const_cast<nsInputEvent&>(aEvent);
> +  aOutEvent->AssignInputEventData(aEvent, true);

The assumption for touch and mouse events is that the "out event" is already a copy of the input event. This call to AssignInputEventData seems to imply that is not the case for the events coming in here. Is there a reason for this difference? I would rather be consistent for all the input events.

@@ +417,5 @@
> +      nsTouchEvent* outEvent = static_cast<nsTouchEvent*>(aOutEvent);
> +      return ProcessTouchEvent(touchEvent, outEvent);
> +    }
> +#ifdef XP_UNIX
> +    case NS_MOUSE_EVENT: {

why is this ifdef'd?
Attachment #807255 - Flags: feedback+
Comment on attachment 807256 [details] [diff] [review]
add inline transform ReceiveInputEvent

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +431,5 @@
>  }
>  
> +nsEventStatus
> +APZCTreeManager::ReceiveInputEvent(nsInputEvent& aEvent)
> +{

So.. you could just implement this by calling ReceiveInputEvent(aEvent, &aEvent), right? Less duplicated code that way. Although really I want to get rid of the two-param version entirely and just always use this one so maybe it's ok to leave it duplicated for now.
Attachment #807256 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> @@ +417,5 @@
> > +      nsTouchEvent* outEvent = static_cast<nsTouchEvent*>(aOutEvent);
> > +      return ProcessTouchEvent(touchEvent, outEvent);
> > +    }
> > +#ifdef XP_UNIX
> > +    case NS_MOUSE_EVENT: {
> 
> why is this ifdef'd?

I guess on unix we convert mouse input to touch, I think this is due to b2g not having real touch input? In any case we don't want to treat mouse as touch on Windows. When I pass a mouse even t in here, all I want is a transformed refPoint.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Comment on attachment 807256 [details] [diff] [review]
> add inline transform ReceiveInputEvent
> 
> Review of attachment 807256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/APZCTreeManager.cpp
> @@ +431,5 @@
> >  }
> >  
> > +nsEventStatus
> > +APZCTreeManager::ReceiveInputEvent(nsInputEvent& aEvent)
> > +{
> 
> So.. you could just implement this by calling ReceiveInputEvent(aEvent,
> &aEvent), right? Less duplicated code that way. Although really I want to
> get rid of the two-param version entirely and just always use this one so
> maybe it's ok to leave it duplicated for now.

Sounds good. We're not using the two param one in Windows to avoid the data copies.
Comment on attachment 807253 [details] [diff] [review]
scroll offset

This addressed a really bad horizontal jitter problem when scrolling a page that could not scroll horizontally. Not sure if it's correct, but does fix the problem.
Attachment #807253 - Flags: review?(bugmail.mozilla)
Comment on attachment 807257 [details] [diff] [review]
add an apz hittest method to apz tree manager

I use this down in widget to do a rough check to see if the apz wants input. It saves me the trouble of having to send a dom hit test to chrome when the browser is displaying a non-apz scrollable interface.
Attachment #807257 - Flags: review?(bugmail.mozilla)
Comment on attachment 807258 [details] [diff] [review]
transform events through the apz in widget

This patch runs all input through the apz if the event coords are in a scrollable view that might need transforming due to zoom. The tricky part is that the apz currently can't handle accurate hit testing for semi-transparent regions so I'm handling that in widget using a dom hit test event. Temporary but kats tells me better apz hit testing will take time to get right, which we don't have.
Attachment #807258 - Flags: review?(tabraldes)
Attachment #807255 - Attachment is obsolete: true
Comment on attachment 808720 [details] [diff] [review]
break up ReceiveInputEvent so it can be reused v.2

updated per review comments.
Attachment #808720 - Flags: review?(bugmail.mozilla)
Comment on attachment 807253 [details] [diff] [review]
scroll offset

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

This feels like a band-aid to me. I don't think the offset being passed in should ever result in the mScrollOffset going outside the scrollable rect. I suspect there's probably just some rounding mismatch in the Axis.cpp code that's causing this. Would you mind digging into where the scroll offset is coming from and why it's causing the scroll offset to jitter?
Attachment #807253 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> Comment on attachment 807253 [details] [diff] [review]
> scroll offset
> 
> Review of attachment 807253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This feels like a band-aid to me. I don't think the offset being passed in
> should ever result in the mScrollOffset going outside the scrollable rect. I
> suspect there's probably just some rounding mismatch in the Axis.cpp code
> that's causing this. Would you mind digging into where the scroll offset is
> coming from and why it's causing the scroll offset to jitter?

We never bounds check what we pass to ScrollBy. So for example if on a web page with no horizontal scroll the user drags diagonally down, mFrameMetrics.mScrollOffset.x will accumulate value. I'm not sure where this should get adjusted so that what gets painted to the screen isn't adjust in a direction it should go. But this check here fixed it.
I'm not sure where this should get adjusted so that what gets painted to the screen is not adjusted in a direction it *shouldn't* go.

I'm also confused why this doesn't show up on android or b2g, seems like those platforms would have the same problem.
(In reply to Jim Mathies [:jimm] from comment #22)
> > > +#ifdef XP_UNIX
> > > +    case NS_MOUSE_EVENT: {
> > 
> > why is this ifdef'd?
> 
> I guess on unix we convert mouse input to touch, I think this is due to b2g
> not having real touch input? In any case we don't want to treat mouse as
> touch on Windows. When I pass a mouse even t in here, all I want is a
> transformed refPoint.

So IIRC that code was put in for B2G emulation purposes. I'm not entirely sure if it was for Gaia on desktop, or B2G on the Android emulator. I'm not even sure if it's relevant any more. ni? to :drs to answer this if he can.
Flags: needinfo?(bugzilla)
(In reply to Jim Mathies [:jimm] from comment #30)
> We never bounds check what we pass to ScrollBy. So for example if on a web
> page with no horizontal scroll the user drags diagonally down,
> mFrameMetrics.mScrollOffset.x will accumulate value. I'm not sure where this
> should get adjusted so that what gets painted to the screen isn't adjust in
> a direction it should go. But this check here fixed it.

There is code that should be clamping the scroll offset already. AsyncPanZoomController::TrackTouch calls APZC::AttemptScroll, which gets the displacement by calling Axis::AdjustDisplacement. This in turn checks to see if the displacement will cause overscroll by calling Axis::DisplacementWillOverscroll, and removes it with Axis::DisplacementWillOverscrollAmount. So in effect the scroll offset should never go outside the scrollable rect, and if it is, then there's a problem somewhere earlier in the code.

(The above code references are for latest m-c. In slightly older code, prior to Botond landing overscroll handoff, the equivalent codepath when through AsyncPanZoomController::TrackTouch, Axis::GetDisplacementForDuration, Axis::DisplacementWillOverscroll, and Axis::DisplacementWillOverscrollAmount)
Comment on attachment 808720 [details] [diff] [review]
break up ReceiveInputEvent so it can be reused v.2

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

r=me with comments addressed, and maybe update the XP_UNIX ifdef depending on what doug says.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +299,5 @@
> +APZCTreeManager::GetTouchInputBlockAPZC(const nsTouchEvent& aEvent, ScreenPoint aPoint)
> +{
> +  nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aPoint);
> +  gfx3DMatrix transformToApzc, transformToScreen;
> +  // Reset the cached apz transform

s/apz/apzc/

@@ +316,5 @@
> +    apzc = RootAPZCForLayersId(apzc);
> +    APZC_LOG("Using APZC %p as the root APZC for multi-touch\n", apzc.get());
> +  }
> +  if (apzc) {
> +    // Cache apz transform so it can be used for future events in this block.

ditto

::: gfx/layers/composite/APZCTreeManager.h
@@ +155,5 @@
>     * NOTE: Be careful of invoking the nsInputEvent variant. This can only be
>     * called on the main thread. See widget/InputData.h for more information on
>     * why we have InputData and nsInputEvent separated.
> +   * NOTE: On unix, mouse events are treated as touch and are forwarded
> +   * to the appropriate apz as such.

ditto
Attachment #808720 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> So IIRC that code was put in for B2G emulation purposes. I'm not entirely
> sure if it was for Gaia on desktop, or B2G on the Android emulator. I'm not
> even sure if it's relevant any more. ni? to :drs to answer this if he can.

I believe it was for the emulator. Though I don't think this is the section you're talking about. Maybe this is it: http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/InputData.cpp#69

Either way, I'm not sure why this has to be platform-specific. To me, it makes sense to handle it on a case-by-case basis (i.e. if it's a mouse event, convert it to a touch event, then handle it). I can see the issue with not wanting to do this conversion on Windows, but why are mouse events being sent to APZC on Windows in the first place, then?
Comment on attachment 807257 [details] [diff] [review]
add an apz hittest method to apz tree manager

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

r=me. For posterity, I want to note that this function is only needed as a workaround because currently APZC hit testing does not account for non-rectangular visible regions on layers. Metro has circular chrome buttons overlaid on top of the content, and so ideally APZC would return the content layer for when the user touches on the content area and the chrome layer for when the user touches on the chrome. But since it currently does not we expose this hit test function so that the metro widget code can handle this case specially. A proper solution would be rather complex, with the APZC having to track all the non-transparent portions of a layer so that the user input gets directed to the layer that the user visually touched on rather than the bounding rect from a different layer.
Attachment #807257 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> (In reply to Jim Mathies [:jimm] from comment #30)
> > We never bounds check what we pass to ScrollBy. So for example if on a web
> > page with no horizontal scroll the user drags diagonally down,
> > mFrameMetrics.mScrollOffset.x will accumulate value. I'm not sure where this
> > should get adjusted so that what gets painted to the screen isn't adjust in
> > a direction it should go. But this check here fixed it.
> 
> There is code that should be clamping the scroll offset already.
> AsyncPanZoomController::TrackTouch calls APZC::AttemptScroll, which gets the
> displacement by calling Axis::AdjustDisplacement. This in turn checks to see
> if the displacement will cause overscroll by calling
> Axis::DisplacementWillOverscroll, and removes it with
> Axis::DisplacementWillOverscrollAmount. So in effect the scroll offset
> should never go outside the scrollable rect, and if it is, then there's a
> problem somewhere earlier in the code.

Ok so this definitely wasn't working in practice for me then. I can try to debug further.
(In reply to Doug Sherk (:drs) (:dRdR) from comment #35)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> I believe it was for the emulator. Though I don't think this is the section
> you're talking about. Maybe this is it:
> http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/InputData.
> cpp#69

That's more or less the same code. The constructor you reference is invoked by the code we're discussing.

> Either way, I'm not sure why this has to be platform-specific. To me, it
> makes sense to handle it on a case-by-case basis (i.e. if it's a mouse
> event, convert it to a touch event, then handle it). I can see the issue
> with not wanting to do this conversion on Windows, but why are mouse events
> being sent to APZC on Windows in the first place, then?

I would actually flip that around, and do the mouse -> touch event conversion in widget/gonk code, so that APZC can just deal with touch inputs, which is what it's designed to do. That would remove the mouse handling from APZCTreeManager entirely, but leave it in widget/gonk, which should satisfy all the requirements.
Flags: needinfo?(bugzilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38)
> I would actually flip that around, and do the mouse -> touch event
> conversion in widget/gonk code, so that APZC can just deal with touch
> inputs, which is what it's designed to do. That would remove the mouse
> handling from APZCTreeManager entirely, but leave it in widget/gonk, which
> should satisfy all the requirements.

But there's nothing inherently platform-specific about converting from non-platform-specific mouse events to non-platform-specific touch events. Putting that conversion in platform-specific code, just because right now it's the only platform that needs it, doesn't make sense to me.
(In reply to Doug Sherk (:drs) (:dRdR) from comment #35)
> Either way, I'm not sure why this has to be platform-specific. To me, it
> makes sense to handle it on a case-by-case basis (i.e. if it's a mouse
> event, convert it to a touch event, then handle it). I can see the issue
> with not wanting to do this conversion on Windows, but why are mouse events
> being sent to APZC on Windows in the first place, then?

So the refPoint can be transformed. For example, clicking a link using the mouse after zooming content via touch. Somewhat corner case, but needs to work right.
(In reply to Jim Mathies [:jimm] from comment #40)
> So the refPoint can be transformed. For example, clicking a link using the
> mouse after zooming content via touch. Somewhat corner case, but needs to
> work right.

Is that not something that has to be done on all platforms and for all input types?
(In reply to Doug Sherk (:drs) (:dRdR) from comment #39)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38)
> > I would actually flip that around, and do the mouse -> touch event
> > conversion in widget/gonk code, so that APZC can just deal with touch
> > inputs, which is what it's designed to do. That would remove the mouse
> > handling from APZCTreeManager entirely, but leave it in widget/gonk, which
> > should satisfy all the requirements.
> 
> But there's nothing inherently platform-specific about converting from
> non-platform-specific mouse events to non-platform-specific touch events.
> Putting that conversion in platform-specific code, just because right now
> it's the only platform that needs it, doesn't make sense to me.

Sorry, I meant that only the mouse -> touch conversion be moved. APZC would still handle and untransform mouse events, but as a general nsInputEvent rather than a nsMouseEvent specifically. I might be mistaken but I think the mRefPoint that needs to be transformed lives on nsInputEvent so there's no need to handle mouse inputs specially from other non-touch inputs.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42)
> Sorry, I meant that only the mouse -> touch conversion be moved. APZC would
> still handle and untransform mouse events, but as a general nsInputEvent
> rather than a nsMouseEvent specifically. I might be mistaken but I think the
> mRefPoint that needs to be transformed lives on nsInputEvent so there's no
> need to handle mouse inputs specially from other non-touch inputs.

Yeah, that makes sense to me, and I think you're right about mRefPoint.
Comment on attachment 807253 [details] [diff] [review]
scroll offset

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

Btw, the correct fix here is probably very similar to the second patch on bug 915831. Except I think that one only affects zooming.
After the discussion here I'm not clear on what the decision was for the special cased NS_MOUSE_EVENT handling, should I keep that or remove it or ?
Attachment #807253 - Attachment is obsolete: true
FWIW I pushed these to try without the special handling. I'm curious to see if this breaks one of the emulators we have running tests.
(In reply to Jim Mathies [:jimm] from comment #45)
> After the discussion here I'm not clear on what the decision was for the
> special cased NS_MOUSE_EVENT handling, should I keep that or remove it or ?

I think you should leave it in your patches here. I can clean it up as part of the other input flow reorganizing I'm about to start doing.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47)
> (In reply to Jim Mathies [:jimm] from comment #45)
> > After the discussion here I'm not clear on what the decision was for the
> > special cased NS_MOUSE_EVENT handling, should I keep that or remove it or ?
> 
> I think you should leave it in your patches here. I can clean it up as part
> of the other input flow reorganizing I'm about to start doing.

I believe we still support desktop emulators so maybe the right thing to do for now is keep it for all platforms except winrt.

Will push one more try build with those changes and assuming everything comes up green go ahead and land this work.
Ok, thanks. I filed bug 920036 for cleaning up the rest of the input event flow.
Attachment #809183 - Flags: review?(tabraldes)
Attachment #807258 - Flags: review?(tabraldes) → review?(netzen)
Attachment #809183 - Flags: review?(tabraldes) → review?(netzen)
Comment on attachment 807258 [details] [diff] [review]
transform events through the apz in widget

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

ugh

::: widget/windows/winrt/MetroInput.cpp
@@ +1180,5 @@
>  
> +  // Test for chrome vs. content target. To do this we only use the first touch
> +  // point since that will be the input batch target. Cache this for touch events
> +  // since HitTestChrome has to send a dom event.
> +  if (event->message == NS_TOUCH_START) {

not sure if this can happen but maybe also check for && event->touches.size() > 0
Attachment #807258 - Flags: review?(netzen) → review+
Attachment #809183 - Flags: review?(netzen) → review+
OS: Windows 8 Metro → Windows 8.1
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: