Randall Barker [:rbarker] Assignee
	Description • 9 years ago

In B2G the function APZCCallbackHelper::ApplyCallbackTransform would remove the nsPresShell resolution from the event in the child process. This worked in B2G because the Root Document and Root Content Document were the same in the child process. In Fennec The Root Document and the Root Content Document are not the same. This caused the events to some times have the incorrect coordinate values in Fennec. Conceptually, the resolution applies to the Root Content Document but not to documents outside of it (such as the Root Document in Fennec, or documents in the parent process on B2G). Rather than remove the nsPresShell resolution in APZCCallbackHelper::ApplyCallbackTransform, it might be better to leave the event coordinates relative to the outer most Root Document (in B2G this is the Root Document in the parent process) and instead update the nsLayoutUtil functions to apply and remove the accumulative nsPresShell resolution as needed.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 9 years ago

I'm going to steal the browser.js changes from this patch for bug 1223296. It doesn't look like they're really relevant for this bug and I need to fiddle with those MOZ_ANDROID_APZ conditions further for that bug anyway.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 9 years ago

Comment on attachment 8686331 [details] [diff] [review]
0001-Bug-1122804-Update-nsLayoutUtils-to-account-for-resolution-when-converting-event-coordinate-15111116-e0b1833.patch

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

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +449,1 @@
>      void* property = content->GetProperty(nsGkAtoms::apzCallbackTransform);

Note that content can be null here, need to rearrange this code a bit...

	Randall Barker [:rbarker] Assignee
	Comment 4 • 9 years ago

Check that content is not null and remove browser.js changes from patch.

	Randall Barker [:rbarker] Assignee
	Comment 5 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 8686331 [details] [diff] [review]
> 0001-Bug-1122804-Update-nsLayoutUtils-to-account-for-resolution-when-
> converting-event-coordinate-15111116-e0b1833.patch
> 
> Review of attachment 8686331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +449,1 @@
> >      void* property = content->GetProperty(nsGkAtoms::apzCallbackTransform);
> 
> Note that content can be null here, need to rearrange this code a bit...

Good catch, fixed in updated patch.

	Botond Ballo [:botond]
	Comment 12 • 9 years ago

Without Part 3 (the ifdefs), we still have B2G test failures [1], and after some further investigation and discussion, I believe that we really have no reason to expect this to work on B2G, because there still is a conceptual problem.

I'm going to try to summarize my understanding of the situation in the hope that it will help us make progress towards a proper solution.

  - The RCD has a resolution, which is applied when its content is rendered
    to the screen. (Specifically, the resolution is stored on the RCD's root
    Layer in ContainerLayer::mScaleToResolution and applied by the compositor.)

  - Input events originate in screen coordinates, which means they have the
    resolution applied. Before being used by content inside the RCD, this
    resolution needs to be unapplied somewhere.

  - On e10s platforms, the RCD is the root document of the child process, so
    the boundary where the resolution needs to be unapplied lines up nicely
    with the process boundary. Accordingly, the un-application happens in
    each method of TabChild that processes a raw input event (like a touch) 
    or something derived from input events (like a tap gesture). 

       - More specifically, the un-application happens in the helper method
         ApplyCallbackTransform(), which is called by all relevant
         TabChild methods.

       - The PBrowser protocol (TabParent/TabChild) can be thought of as a 
         "bottleneck" through which all relevant communication between the 
         parent and child process passes, so this ensures the resolution is 
         always unapplied.

  - On non-e10s platforms - notably Fennec - the boundary where the resolution
    needs to be unapplied doesn't line up as nicely with anything.

  - To make APZ work on non-e10s platforms, the patches in this bug try to
    move the un-application of the resolution from the TabChild methods to 
    three places:

       1) nsDisplayResolution::HitTest, which is intended to cover cases where
          the coodinates cross into the RCD during hit testing.

       2) GetEventCoordinatesRelativeTo() and TranslateViewToWidget(), which 
          are intended to cover cases where the coordinates cross into the RCD
          during event dispatch.

       3) HandleDoubleTap(). This is a bit of a special case because, unlike
          HandleSingleTap() and HandleLongTap(), it doesn't create WidgetEvents
          for which the conversion is handled by (2), so it needs to unapply
          the resolution itself.

          (Note: I'm at this point convinced that the patch unapplying the
                 resolution in ChromeProcessController::HandleDoubleTap() but
                 not TabChild::RecvHandleDoubleTap() is a bug. I tested 
                 double-tap-to-zoom with these patches on B2G, and it's
                 busted.)

    I don't have a deep enough understanding of Gecko to be convinced that these
    three scenarios are exhaustive, but I'm prepared to take someone's word for it.

  - The conceptual problem we're facing is that (1) doesn't work on e10s platforms,
    where the RCD is the root document in the child process. nsDisplayResolution 
    display items are only built for subdocuments, so there is nothing to unapply
    the resolution during hit testing on B2G.

    A couple of possible solutions came up on IRC today:

       (a) As nsDisplayResolution - being an nsDisplaySubDocument - can be thought
           of as a container for the subdocument in the parent document, the
           equivalent display item for a remote document would be nsDisplayRemote.
           So, we could make a similar modification to nsDisplayRemote::HitTest,
           called during parent-process hit testing.

           (Importantly, we would want nsDisplayRemote to _mutate_ the point it's
           processing so that it's the mutated (resolution-unapplied) point that's 
           sent on to the child process. I don't know if HitTest() is set up to do 
           that sort of thing, but let's consider that a "detail" for now.)

           This would work for raw input events which pass through parent-process
           hit testing. It wouldn't work for things like tap gestures, so we'd
           have to unapply the resolution for those somewhere else, like in APZ.

       (b) Continue unapplying the resolution in TabChild methods.

    (I'll mention for the record that the original solution proposed in 
     bug 1122804 comment 0 was to unapply the resolution in APZ even for raw
     input events, but that was problematic because then it would be unapplied 
     when the events go through parent-process event dispatch, and that's 
     incorrect.)

    However, there's a problem with both of these approaches: while (1)
    (nsDisplayResolution::HitTest) doesn't happen for the RCD in e10s setups,
    (2) does, so for the purpose of event dispatch in the child process, we'd
    be unapplying the resolution twice.

    -----------------------------------------------------------------------

    This last point makes me wonder whether, perhaps, the cleanest solution
    would be to do (1)/(2)/(3) on non-e10s platforms *only*, while continuing to
    unapply the resolution in TabChild (and not elsewhere) for e10s platforms.

    This would essentially amount to the #ifdef approach, but it could be
    formulated and explained a bit more neatly, with the conceptual model being
    that we're deliberately performing mutually exclusive sets of resolution
    un-applications for the e10s and non-e10s casees.

    Thoughts?
    

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=309a6b73301b

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 13 • 9 years ago

For the record, I'm still in favour of landing these patches (with #ifdefs) because then we'll start getting feedback from Fennec users - the feedback may reveal additional bugs and use cases we haven't considered yet, and that will provide more guidance as to what the right solution is. It would suck to do big rewrite of the approach now, land it, and then have to do another big rewrite because we find more use cases that make us reconsider everything.

	Botond Ballo [:botond]
	Comment 14 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> For the record, I'm still in favour of landing these patches (with #ifdefs)
> because then we'll start getting feedback from Fennec users - the feedback
> may reveal additional bugs and use cases we haven't considered yet, and that
> will provide more guidance as to what the right solution is. It would suck
> to do big rewrite of the approach now, land it, and then have to do another
> big rewrite because we find more use cases that make us reconsider
> everything.

Note that the proposal at the end of comment 12 isn't a big rewrite - it's essentially the #ifdefs, with perhaps some light refactoring to make the conceptual model clearer.

That said, I agree that even the light refactoring can be done as a follow-up. I'm not suggesting blocking these patches at all :)

	Timothy Nikkel (:tnikkel)
	Comment 15 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #12)
>   - The conceptual problem we're facing is that (1) doesn't work on e10s
> platforms,
>     where the RCD is the root document in the child process.
> nsDisplayResolution 
>     display items are only built for subdocuments, so there is nothing to
> unapply
>     the resolution during hit testing on B2G.

We can wrap the display list in a nsDisplayResolution, or we can add resolution handling to nsLayoutUtils::GetFramesForArea like we do for painting nsDisplayList::PaintRoot at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=c13d0478272f#1570

> 
>     However, there's a problem with both of these approaches: while (1)
>     (nsDisplayResolution::HitTest) doesn't happen for the RCD in e10s setups,
>     (2) does, so for the purpose of event dispatch in the child process, we'd
>     be unapplying the resolution twice.

So if we landed these patches (without the ifdefs) then only the code in (2) would unapply the resolution and it would work?


Another solution would to be quit pretending that we are actually making resolution work in non-e10s, and recognize that fennec is a special case, where the parent chrome document is an empty husk and we can treat the RCD as the root document in most cases. So I think this would involve:
1) doing the document retargeting in PresShell::HandleEvent that happens on androind to GetTouchEventTargetDocument() for most (all?) events
2) in PresShell::HandleEvent, if we don't retarget to another doc, if we are on android, and we are the primary content shell, remove the resolution of the event point before proceeding

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 16 • 9 years ago

So I spent some time thinking about this and trying to formulate a consistent mental model of how it should work. It helps to think of the case where any presshell can have a resolution, not just the RCD. In this world, the RCD is not "special", it's just another document, and any resolution-specific handling has to be done as we navigate in the code from one document to another.

Fundamentally I think this means the nsDisplayResolution/GetEventCoordinatesRelativeTo/TranslateViewToWidget function changes are necessary. For resolution changes between two presshells inside a process, the above functions (plus maybe a few others that we might have missed) should handle all the cases. The other set of cases is when we have a resolution lining up with the process boundary. In these cases I think the corresponding resolution adjustment should be done in TabChild.

(I think the approach of introducing a new nsDisplayResolution to handle this case would get complicated because the nsDisplayResolution would have to live in the child process but nsDisplayResolution is a subclass of nsDisplaySubDocument, and we may not want to inherit that behaviour. We might have to create a separate display item for this. Also, GetEventCoordinatesRelativeTo/TranslateViewToWidget/etc would need corresponding changes, and in some cases those functions might not even be getting called for the root document in a child process but we would still need the transformation).

So my proposal is to make the following changes from what we have in m-c currently:
- Take Randall's changes to everything except APZCCallbackHelper
- Change APZCCallbackHelper to unapply the resolution from the RD rather than the RCD. This will automatically cause the resolution unapplication to continue happening in TabChild, but not affect the parent-process calls to ApplyCallbackTransform.
- Ensure that the ChromeProcessController/AndroidContentController Handle*Tap implementations are prepared to deal with coordinates that still have the resolution applied, and either divide out the resolutions manually or use code that does (e.g. by using existing hit-testing functions that go through nsDisplayResolution).

I'm going to try implementing this and see what happens.

	Botond Ballo [:botond]
	Comment 17 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #15)
> >     However, there's a problem with both of these approaches: while (1)
> >     (nsDisplayResolution::HitTest) doesn't happen for the RCD in e10s setups,
> >     (2) does, so for the purpose of event dispatch in the child process, we'd
> >     be unapplying the resolution twice.
> 
> So if we landed these patches (without the ifdefs) then only the code in (2)
> would unapply the resolution and it would work?

For event handling, I believe so. For hit testing, nothing would unapply the resolution on B2G unless we introduce an nsDisplayResolution for the RCD like you suggest, or solve the problem in another way.

	Botond Ballo [:botond]
	Comment 18 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> Fundamentally I think this means the
> nsDisplayResolution/GetEventCoordinatesRelativeTo/TranslateViewToWidget
> function changes are necessary. For resolution changes between two
> presshells inside a process, the above functions (plus maybe a few others
> that we might have missed) should handle all the cases.

If we go with this approach, we need to be careful that these functions _don't_ unapply a root document resolution. (I believe the way they are currently written they do.)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 19 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #18)
> If we go with this approach, we need to be careful that these functions
> _don't_ unapply a root document resolution. (I believe the way they are
> currently written they do.)

Oh! If they do then wrapping the child in a nsDisplayResolution is probably a better approach. I was under the impression that they didn't, but I didn't actually check.

I implemented what I was thinking, and it works on Fennec but tapping is broken on B2G, so either I screwed up something or my model is wrong. I'll dig into it but in the meantime I think we should still go ahead and land these patches with ifdefs.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 20 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> (In reply to Botond Ballo [:botond] from comment #18)
> > If we go with this approach, we need to be careful that these functions
> > _don't_ unapply a root document resolution. (I believe the way they are
> > currently written they do.)
> 
> Oh! If they do then wrapping the child in a nsDisplayResolution is probably
> a better approach. I was under the impression that they didn't, but I didn't
> actually check.
> 
> I implemented what I was thinking, and it works on Fennec but tapping is
> broken on B2G, so either I screwed up something or my model is wrong. I'll
> dig into it but in the meantime I think we should still go ahead and land
> these patches with ifdefs.

I think I got it working locally - but I did have to modify GetEventCoordinatesRelativeTo to not remove the resolution in the root document case. In a way that makes sense because now the resolution-removal in that function is parallel to the ScaleToOtherAppUnits code, which makes sense to me since nsDisplayResolution should work largely like nsDisplayZoom. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0971a5ff34c4

	Timothy Nikkel (:tnikkel)
	Comment 21 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> (I think the approach of introducing a new nsDisplayResolution to handle
> this case would get complicated because the nsDisplayResolution would have
> to live in the child process but nsDisplayResolution is a subclass of
> nsDisplaySubDocument, and we may not want to inherit that behaviour. We
> might have to create a separate display item for this.

That's not hard to do, so don't let that stop a potential solution.

	Timothy Nikkel (:tnikkel)
	Comment 25 • 9 years ago

Comment on attachment 8691623 [details] [diff] [review]
0003-Bug-1224015-Part-3-ifdef-changes-to-C-APZ-so-that-they-only-apply-to-single-process-APZ-15112415-f6a4a52.patch

I only skimmed this, going to assume that you got all the places you changed code.

	Timothy Nikkel (:tnikkel)
	Comment 26 • 9 years ago

Comment on attachment 8691622 [details] [diff] [review]
0002-Bug-1224015-Part-2-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15112415-190bacf.patch

>+nsRect::RemoveResolution(const float aResolution) const
>+  if (width == 1 && height == 1) {
>+    rect.width = rect.height = 1.0f;

These are ints, no need for floating point.

>@@ -409,16 +409,18 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>   if (subdocRootFrame) {
>     // get the dirty rect relative to the root frame of the subdoc
>     dirty = aDirtyRect + GetOffsetToCrossDoc(subdocRootFrame);
>     // and convert into the appunits of the subdoc
>     dirty = dirty.ScaleToOtherAppUnitsRoundOut(parentAPD, subdocAPD);
>+    // and remove nsPresShell resolution
>+    dirty = dirty.RemoveResolution(presShell->ScaleToResolution() ? presShell->GetResolution () : 1.0f);
> 
>     if (nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame()) {
>       nsIScrollableFrame* rootScrollableFrame = presShell->GetRootScrollFrameAsScrollable();
>       MOZ_ASSERT(rootScrollableFrame);
>       haveDisplayPort = rootScrollableFrame->DecideScrollableLayer(aBuilder,
>                           &dirty, /* aAllowCreateDisplayPort = */ true);

Did you find that this was necessary to fix something?

Something like this is likely necessary to fix bug 1225508.

However, we can't factor in the resolution at exactly this point because the DecideScrollableLayer call just below uses the passed in dirty rect to set the display port base rect, which needs to not include the resolution.

I'm still trying to figure out the best way to do this. Do you need this to land the rest of the patches?

	Timothy Nikkel (:tnikkel)
	Comment 27 • 9 years ago

Comment on attachment 8691621 [details] [diff] [review]
0001-Bug-1224015-Part-1-nsLayoutUtils-functions-do-not-account-for-nsPresShell-resolution-15112415-4213f11.patch

>@@ -79,18 +79,38 @@ MouseEvent::InitMouseEvent(const nsAString& aType,
>     case ePointerEventClass:
>     case eSimpleGestureEventClass: {
>       WidgetMouseEventBase* mouseEventBase = mEvent->AsMouseEventBase();
>       mouseEventBase->relatedTarget = aRelatedTarget;
>       mouseEventBase->button = aButton;
>       mouseEventBase->InitBasicModifiers(aCtrlKey, aAltKey, aShiftKey, aMetaKey);
>       mClientPoint.x = aClientX;
>       mClientPoint.y = aClientY;
>-      mouseEventBase->refPoint.x = aScreenX;
>-      mouseEventBase->refPoint.y = aScreenY;
>+      float resolution = 1.0;
>+      LayoutDeviceIntPoint offset;
>+      nscoord appUnitsToDevFullZoom = nsPresContext::AppUnitsPerCSSPixel();
>+      if (mPresContext) {
>+        nsIPresShell* shell = mPresContext->PresShell();
>+        if (shell) {
>+          resolution = shell->GetCumulativeScaleResolution();
>+          appUnitsToDevFullZoom = mPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
>+        }
>+      }
>+      WidgetGUIEvent* guiEvent = mEvent->AsGUIEvent();
>+      if (guiEvent && guiEvent->widget) {
>+        offset = guiEvent->widget->WidgetToScreenOffset();
>+      }
>+
>+      LayoutDevicePoint point =
>+          LayoutDevicePixel::FromAppUnits(
>+              CSSPixel::ToAppUnits(CSSPoint(aScreenX, aScreenY)),
>+              appUnitsToDevFullZoom);
>+      point -= offset;
>+      point = point * resolution;
>+      mouseEventBase->refPoint = RoundedToInt(point);
> 
>       WidgetMouseEvent* mouseEvent = mEvent->AsMouseEvent();
>       if (mouseEvent) {
>         mouseEvent->clickCount = aDetail;
>       }
>       break;
>     }

I don't think I understand this change. Why was it needed?

What's confusing is that the current code assigns aScreenX/Y to the refpoint of the event. Assuming aScreenX/Y are in screen coords, and refPoint is relative to the widget this seems wrong. But the code seems to go back to the CVS days, so I'm guessing there is a good reason why it is like that.

So that makes me think the new code that shifts by the widget to screen offset may be wrong?

	Botond Ballo [:botond]
	Comment 28 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #27)
> I don't think I understand this change. Why was it needed?
> 
> What's confusing is that the current code assigns aScreenX/Y to the refpoint
> of the event. Assuming aScreenX/Y are in screen coords, and refPoint is
> relative to the widget this seems wrong. But the code seems to go back to
> the CVS days, so I'm guessing there is a good reason why it is like that.
> 
> So that makes me think the new code that shifts by the widget to screen
> offset may be wrong?

When we compute ScreenX/Y from the refpoint, we add the widget offset [1]. Here, we're doing the corresponding subtraction. This was needed to make a test which tests round-tripping of the screen coordinates (that is, it sets them and then reads them back to see if it gets the same value) pass.

[1] https://dxr.mozilla.org/mozilla-central/rev/45273bbed8efaface6f5ec56d984cb9faf4fbb6a/dom/events/Event.cpp#938

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 29 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #26)
> >@@ -409,16 +409,18 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
> >   if (subdocRootFrame) {
> >     // get the dirty rect relative to the root frame of the subdoc
> >     dirty = aDirtyRect + GetOffsetToCrossDoc(subdocRootFrame);
> >     // and convert into the appunits of the subdoc
> >     dirty = dirty.ScaleToOtherAppUnitsRoundOut(parentAPD, subdocAPD);
> >+    // and remove nsPresShell resolution
> >+    dirty = dirty.RemoveResolution(presShell->ScaleToResolution() ? presShell->GetResolution () : 1.0f);
> > 
> >     if (nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame()) {
> >       nsIScrollableFrame* rootScrollableFrame = presShell->GetRootScrollFrameAsScrollable();
> >       MOZ_ASSERT(rootScrollableFrame);
> >       haveDisplayPort = rootScrollableFrame->DecideScrollableLayer(aBuilder,
> >                           &dirty, /* aAllowCreateDisplayPort = */ true);
> 
> Did you find that this was necessary to fix something?

IIRC this was needed to fix the hit-testing on http://bluemarvin.github.io/html/divscroll.html - without it you could only scroll the div by dragging in the bottom area of the div; dragging in the top wouldn't hit it and wouldn't layerize the div. We found that the code at [1] was finding the root <html> as the enclosing scrollable, turned on display list dumping for the hit test, and found that the dirty rect was wrong so it wasn't collecting the right display items.

[1] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/gfx/layers/apz/util/APZCCallbackHelper.cpp#598 

> Something like this is likely necessary to fix bug 1225508.

I checked to see if that fixed bug 1225508, and it didn't, unfortunately. But yeah I agree that something like this is probably what we need, just not sure where.

	Randall Barker [:rbarker] Assignee
	Comment 30 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #27)
> Comment on attachment 8691621 [details] [diff] [review]
> 0001-Bug-1224015-Part-1-nsLayoutUtils-functions-do-not-account-for-
> nsPresShell-resolution-15112415-4213f11.patch
>
> I don't think I understand this change. Why was it needed?
> 
> What's confusing is that the current code assigns aScreenX/Y to the refpoint
> of the event. Assuming aScreenX/Y are in screen coords, and refPoint is
> relative to the widget this seems wrong. But the code seems to go back to
> the CVS days, so I'm guessing there is a good reason why it is like that.
> 
> So that makes me think the new code that shifts by the widget to screen
> offset may be wrong?

This code was originally added to address try failures in B2G caused by events created in js code. After further investigation we found the reason try was not failing for Fennec is because dom/events/test/mochitest.ini is disabled for Fennec. :/ Enabling these tests for Fennec revealed the same failures. The try failures are caused because the DOM events are created in js code so the screen coordinates where being set on the event without modification. However, when the values of the event were read back, they were being transformed due to the code added here:

https://bugzilla.mozilla.org/attachment.cgi?id=8691621&action=diff#a/dom/events/Event.cpp_sec1

It was necessary to perform the reverse transformation for the values coming in from js code in order for them to be mostly the same when read back. I say mostly because unfortunately rounding errors cause off-by-one errors to manifest. Due to these off-by-one errors, the dom/events/test/ are still disabled for Fennec.

	Randall Barker [:rbarker] Assignee
	Comment 31 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #26)
> Comment on attachment 8691622 [details] [diff] [review] 
> >@@ -409,16 +409,18 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
> >   if (subdocRootFrame) {
> >     // get the dirty rect relative to the root frame of the subdoc
> >     dirty = aDirtyRect + GetOffsetToCrossDoc(subdocRootFrame);
> >     // and convert into the appunits of the subdoc
> >     dirty = dirty.ScaleToOtherAppUnitsRoundOut(parentAPD, subdocAPD);
> >+    // and remove nsPresShell resolution
> >+    dirty = dirty.RemoveResolution(presShell->ScaleToResolution() ? presShell->GetResolution () : 1.0f);
> > 
> >     if (nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame()) {
> >       nsIScrollableFrame* rootScrollableFrame = presShell->GetRootScrollFrameAsScrollable();
> >       MOZ_ASSERT(rootScrollableFrame);
> >       haveDisplayPort = rootScrollableFrame->DecideScrollableLayer(aBuilder,
> >                           &dirty, /* aAllowCreateDisplayPort = */ true);
> 
> Did you find that this was necessary to fix something?
> 
> Something like this is likely necessary to fix bug 1225508.
> 
> However, we can't factor in the resolution at exactly this point because the
> DecideScrollableLayer call just below uses the passed in dirty rect to set
> the display port base rect, which needs to not include the resolution.
> 
> I'm still trying to figure out the best way to do this. Do you need this to
> land the rest of the patches?

Without this fix, scrolling iframes and scrollable divs is broken.

	Randall Barker [:rbarker] Assignee
	Comment 32 • 9 years ago

Address review comment. Carry forward r+ from :tn

	Timothy Nikkel (:tnikkel)
	Comment 33 • 9 years ago

(In reply to Randall Barker [:rbarker] from comment #30)
> failures. The try failures are caused because the DOM events are created in
> js code so the screen coordinates where being set on the event without
> modification. However, when the values of the event were read back, they
> were being transformed due to the code added here:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8691621&action=diff#a/dom/
> events/Event.cpp_sec1
> 
> It was necessary to perform the reverse transformation for the values coming
> in from js code in order for them to be mostly the same when read back. I

So currently on m-c the code in InitMouseEvent sets the refpoint on the event to the passed in screen coords directly. And Event::GetScreenCoords adds the widget to screen offset to the refpoint. So how does this test pass on desktop?

	Randall Barker [:rbarker] Assignee
	Comment 34 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #33)
> (In reply to Randall Barker [:rbarker] from comment #30)
> > failures. The try failures are caused because the DOM events are created in
> > js code so the screen coordinates where being set on the event without
> > modification. However, when the values of the event were read back, they
> > were being transformed due to the code added here:
> > 
> > https://bugzilla.mozilla.org/attachment.cgi?id=8691621&action=diff#a/dom/
> > events/Event.cpp_sec1
> > 
> > It was necessary to perform the reverse transformation for the values coming
> > in from js code in order for them to be mostly the same when read back. I
> 
> So currently on m-c the code in InitMouseEvent sets the refpoint on the
> event to the passed in screen coords directly. And Event::GetScreenCoords
> adds the widget to screen offset to the refpoint. So how does this test pass
> on desktop?

Maybe the widget is NULL so the values are passed back unchanged?

	Timothy Nikkel (:tnikkel)
	Comment 35 • 9 years ago

(In reply to Randall Barker [:rbarker] from comment #34)
> > So currently on m-c the code in InitMouseEvent sets the refpoint on the
> > event to the passed in screen coords directly. And Event::GetScreenCoords
> > adds the widget to screen offset to the refpoint. So how does this test pass
> > on desktop?
> 
> Maybe the widget is NULL so the values are passed back unchanged?

If that's the case then why do we get a non-null widget on b2g and android? Doesn't seem like the widget presence should be changing between desktop and mobile.

	Timothy Nikkel (:tnikkel)
	Comment 36 • 9 years ago

Comment on attachment 8691621 [details] [diff] [review]
0001-Bug-1224015-Part-1-nsLayoutUtils-functions-do-not-account-for-nsPresShell-resolution-15112415-4213f11.patch

>@@ -79,18 +79,38 @@ MouseEvent::InitMouseEvent(const nsAString& aType,
>+      float resolution = 1.0;
>+      LayoutDeviceIntPoint offset;
>+      nscoord appUnitsToDevFullZoom = nsPresContext::AppUnitsPerCSSPixel();
>+      if (mPresContext) {
>+        nsIPresShell* shell = mPresContext->PresShell();
>+        if (shell) {
>+          resolution = shell->GetCumulativeScaleResolution();
>+          appUnitsToDevFullZoom = mPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
>+        }
>+      }

Also, why use AppUnitsPerDevPixelAtUnitFullZoom for this?

	Randall Barker [:rbarker] Assignee
	Comment 37 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #36)
> Comment on attachment 8691621 [details] [diff] [review]
> 0001-Bug-1224015-Part-1-nsLayoutUtils-functions-do-not-account-for-
> nsPresShell-resolution-15112415-4213f11.patch
> 
> >@@ -79,18 +79,38 @@ MouseEvent::InitMouseEvent(const nsAString& aType,
> >+      float resolution = 1.0;
> >+      LayoutDeviceIntPoint offset;
> >+      nscoord appUnitsToDevFullZoom = nsPresContext::AppUnitsPerCSSPixel();
> >+      if (mPresContext) {
> >+        nsIPresShell* shell = mPresContext->PresShell();
> >+        if (shell) {
> >+          resolution = shell->GetCumulativeScaleResolution();
> >+          appUnitsToDevFullZoom = mPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
> >+        }
> >+      }
> 
> Also, why use AppUnitsPerDevPixelAtUnitFullZoom for this?

Because that is what is used in Event::GetScreenCoords.

	Randall Barker [:rbarker] Assignee
	Comment 38 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #35)
> (In reply to Randall Barker [:rbarker] from comment #34)
> > > So currently on m-c the code in InitMouseEvent sets the refpoint on the
> > > event to the passed in screen coords directly. And Event::GetScreenCoords
> > > adds the widget to screen offset to the refpoint. So how does this test pass
> > > on desktop?
> > 
> > Maybe the widget is NULL so the values are passed back unchanged?
> 
> If that's the case then why do we get a non-null widget on b2g and android?
> Doesn't seem like the widget presence should be changing between desktop and
> mobile.

I don't know how the various test harnesses work. The intent was to make the js event constructors work as expected or at least as close as possible. We were not able to conceive of a system where rounding error did not affect the returned values.

	Timothy Nikkel (:tnikkel)
	Comment 39 • 9 years ago

(In reply to Randall Barker [:rbarker] from comment #38)
> I don't know how the various test harnesses work. The intent was to make the
> js event constructors work as expected or at least as close as possible. We
> were not able to conceive of a system where rounding error did not affect
> the returned values.

It's not the rounding error, it's that on desktop we seemingly don't add or subtract the widget to screen offset in the constructor, but we do in GetScreenCoords, and we still seem to pass? That makes me think we are missing some important piece of the puzzle here.

	Timothy Nikkel (:tnikkel)
	Comment 40 • 9 years ago

Comment on attachment 8692103 [details] [diff] [review]
0002-Bug-1224015-Part-2-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15112511-8a0d756.patch

>@@ -409,16 +409,18 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+    // and remove nsPresShell resolution
>+    dirty = dirty.RemoveResolution(presShell->ScaleToResolution() ? presShell->GetResolution () : 1.0f);
> 
>     if (nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame()) {
>       nsIScrollableFrame* rootScrollableFrame = presShell->GetRootScrollFrameAsScrollable();
>       MOZ_ASSERT(rootScrollableFrame);
>       haveDisplayPort = rootScrollableFrame->DecideScrollableLayer(aBuilder,
>                           &dirty, /* aAllowCreateDisplayPort = */ true);

To be clear, we don't want to pass dirty after we've removed resolution to DecideScrollableLayer. DecideScrollableLayer expects that not to have happened yet.

	Timothy Nikkel (:tnikkel)
	Comment 41 • 9 years ago

(In reply to Randall Barker [:rbarker] from comment #30)
> This code was originally added to address try failures in B2G caused by
> events created in js code. After further investigation we found the reason
> try was not failing for Fennec is because dom/events/test/mochitest.ini is
> disabled for Fennec. :/ Enabling these tests for Fennec revealed the same
> failures.

What tests were these? I'll try to look into why they are passing on desktop since that seems to be the only way forward here.

	Botond Ballo [:botond]
	Comment 42 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #41)
> (In reply to Randall Barker [:rbarker] from comment #30)
> > This code was originally added to address try failures in B2G caused by
> > events created in js code. After further investigation we found the reason
> > try was not failing for Fennec is because dom/events/test/mochitest.ini is
> > disabled for Fennec. :/ Enabling these tests for Fennec revealed the same
> > failures.
> 
> What tests were these? I'll try to look into why they are passing on desktop
> since that seems to be the only way forward here.

I believe it was dom/events/test/test_bug741666.html, and this was the failing assertion [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/35916735b8afc5b0732e00f9aeb56bf846bba7f4/dom/events/test/test_bug741666.html#61

	Randall Barker [:rbarker] Assignee
	Comment 43 • 9 years ago

removed code from MouseEvent::Init that was changing the screen coord values.

	Randall Barker [:rbarker] Assignee
	Comment 44 • 9 years ago

Address comments about when and where to remove resolution from dirty rect in hit test.

	Randall Barker [:rbarker] Assignee
	Comment 45 • 9 years ago

rebased

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 46 • 9 years ago

Try push with the patches from bug 1228216 and this bug, with apz-fennec enabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94cabe1b630c

Same patches, but apz-fennec disabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f005b7da4b97

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 47 • 9 years ago

Try pushes seem green. The only legit failure appears to be a C4 failure on Android which I'll fix by backing out the change I made at https://hg.mozilla.org/mozilla-central/rev/474012a19e10#l9.32 since apparently that assertion went away on its own.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 48 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47)
> I'll fix by backing out the change I made at
> https://hg.mozilla.org/mozilla-central/rev/474012a19e10#l9.32 since
> apparently that assertion went away on its own.

Did this in https://hg.mozilla.org/integration/mozilla-inbound/rev/748531bf0272. The patches on this bug should be good to go land they get tn's stamp of approval.

	Pulsebot
	Comment 49 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c0d6e4df2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa2e79b9252
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e9567751fd

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 50 • 9 years ago

Awesome, thanks for pushing these!

	Carsten Book [:Tomcat]
	Comment 51 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/22c0d6e4df2c
https://hg.mozilla.org/mozilla-central/rev/0aa2e79b9252
https://hg.mozilla.org/mozilla-central/rev/d1e9567751fd