Closed Bug 1556556 Opened 3 years ago Closed 2 years ago

Improve main thread hit testing in the presence of zooming (allowing e.g. for scrollbars to be hit)

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox69 --- wontfix
firefox78 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 3 open bugs)

Details

(Whiteboard: apz-planning)

Attachments

(28 files, 7 obsolete files)

Bug 1556556 - Remove some cruft related to handling the resolution in non-e10s setups. r?tnikkel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Clarify the documentation of APZCCallbackHelper::ApplyCallbackTransform(). r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Factor out an APZCCallbackHelper::GetCallbackTransform() helper. r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Remove APZCCallbackHelper::ApplyCallbackTransform(). r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Have GetCallbackTransform take just a scroll id rather than an entire guid. r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Move GetCallbackTransform() into a new ViewportUtils class. r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Rename GetCallbackTransform() to GetVisualToLayoutTransform(). r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Remove applications of the visual-to-layout transform at the process boundary (and equivalent places for non-e10s). r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Add some VisualToLayout() and LayoutToVisual() wrappers to ViewportUtils. r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Support both CSS and LayoutDevice units in GetVisualToLayoutTransform(). r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Add ViewportUtils::IsZoomedContentRoot(). r?kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Apply the visual-to-layout transform during display list building and display list based hit testing. r?kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Remove many uses of IgnoreRootScrollFrame. r=mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Add a "relative to layout viewport" option for display list building. r=mstange!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Use the "relative to layout viewport" flag to determine when to divide the composition bounds clip by the resolution. r=mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Introduce ViewportType and RelativeTo. r=kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Document WidgetEvent::mRefPoint as being in visual coordinates. r=kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Propagate RelativeTo far and wide. r=kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Include the layout-to-visual transform for a zoomed content root in GetTransformMatrix(). r=kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Handle visual/layout coordinate conversions correctly in GetEventCoordinatesRelativeTo(). r=kats!,mattwoodrow!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Perform app unit to milimetre conversion correctly in PositionedEventTargeting. r=tnikkel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Miscellaneous fixes related to event coordinate transformations. r=kats,tnikkel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Convert mRefPoint to visual coordinates for synthesized events. r=kats,tnikkel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Update test expectations for elementTiming.html. r=mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Update test expectations for tests which are now passing. r=mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Run helper_drag_root_scrollbar.html with zooming enabled as well. r=kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Use FrameForPointOption::IsRelativeToLayoutViewport when passing layout-relative coordinates to GetFrameForPoint(). r=kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1556556 - Disable zoom-to-focused-input behaviour in dom/html/test/forms. r=kats
47 bytes, text/x-phabricator-request
Details | Review

STR

  1. Run Nightly with apz.allow_zooming=true.
  2. Load planet.mozilla.org.
  3. Drag the vertical scrollbar with the mouse.

Expected results

Dragging the scrollbar works just as with apz.allow_zooming=false.

Actual results

The scroll position jumps back and forth

and/or

the page scrolls in the horizontal direction as well.

The main thread seems to be getting into the "select-drag content" codepath rather than the "drag scroll thumb" codepath.

Probably due to the IGNORE_ROOT_SCROLL_FRAME stuff that we need to Actually Fix this time.

(In reply to Botond Ballo [:botond] from comment #1)

Probably due to the IGNORE_ROOT_SCROLL_FRAME stuff that we need to Actually Fix this time.

Indeed: if I comment out this line, scrollbar dragging works great (but main-thread hit testing will break when zoomed out).

Summary: Scrollbar dragging misbehaves with apz.allow_zooming=true when not zoomed in or out → Scrollbar dragging of root scrollbars misbehaves with apz.allow_zooming=true when not zoomed in or out

Dragging the viewport scrollbar is accomplished by passing in a window
rather than an element.

Note that we can't just pass in the window's document.documentElement,
because coordinatesRelativeToScreen() would not give the correct result
for it. This is turn is because for a scrollable <div>, getBoundingClientRect()
returns the scroll frame's outer rect, but for the <html> element,
getBoundingClientRect() returns the root scroll frame's inner rect.

Depends on D34257

The patches posted so far just add a test case for this (and infrastructure required for the test case to work).

The next step is to actually fix the problem and make the testcase pass.

(In reply to Botond Ballo [:botond] from comment #2)

Indeed: if I comment out this line, scrollbar dragging works great (but main-thread hit testing will break when zoomed out).

In particular, commenting out that line makes the test case I wrote pass, but it makes helper_fixed_position_scroll_hittest.html fail. A proper fix will have both of them passing.

The fix for this may fix bug 1557160 as well.

See Also: → 1557160
Blocks: 1557160
See Also: 1557160 →
Blocks: 1556962

Updating bug title to reflect that the patches here will fix a category of hit-testing related issues with zooming, with hitting scrollbars being one of them.

Summary: Scrollbar dragging of root scrollbars misbehaves with apz.allow_zooming=true when not zoomed in or out → Improve main thread hit testing in the presence of zooming (allowing e.g. for scrollbars to be hit)

I'm some ways away from a fix here, so I'm going to get the patches posted so far landed in a dependent bug, with the test annotated as failing for now.

Depends on: 1561726

Comment on attachment 9070748 [details]
Bug 1556556 - Add an nsIContent overload of nsLayoutUtils::FindScrollableFrameFor. r=tnikkel

Revision D34256 was moved to bug 1561726. Setting attachment 9070748 [details] to obsolete.

Attachment #9070748 - Attachment is obsolete: true

Comment on attachment 9070749 [details]
Bug 1556556 - Add an nsIDOMWindowUtils API for querying the size of layout scrollbars. r=tnikkel

Revision D34257 was moved to bug 1561726. Setting attachment 9070749 [details] to obsolete.

Attachment #9070749 - Attachment is obsolete: true

Comment on attachment 9070750 [details]
Bug 1556556 - Modify dragVerticalScrollbar() to support dragging the viewport scrollbar as well. r=kats

Revision D34258 was moved to bug 1561726. Setting attachment 9070750 [details] to obsolete.

Attachment #9070750 - Attachment is obsolete: true

Comment on attachment 9070752 [details]
Bug 1556556 - Add a test to exercise dragging the viewport scrollbar. r=kats

Revision D34259 was moved to bug 1561726. Setting attachment 9070752 [details] to obsolete.

Attachment #9070752 - Attachment is obsolete: true
Depends on: 1575093
Blocks: desktop-zoom-xp

Just to provide an update here, I have a WIP patch series for this and I've been working on greening it up on Try. Latest Try push is here:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=1aa83d314ab433af32620a45d844a723ea398ccf

As you can see there are still a lot of failures, especially on Android, but it's greener than previous iterations (like this one).

Some of the failures in the most recent Try push have convinced me that the approach I've been pursuing here may not be workable.

Let me summarize the current approach, why it may not be workable, and what some possible alternatives are.

First, some background:

  • Gecko code that runs on the main thread (e.g. that works with the frame tree) typically works with coordinates that do not include the pinch-zoom resolution. For example, this is the case for the main thread hit-testing code (nsLayoutUtils::GetFrameForPoint()), and code that exposes values to web content (e.g. window.scrollX).
    • Such coordinates also do not include any offset of the visual viewport relative to the layout viewport that may have been introduced by pinch-zooming. (For example, getBoundingClientRect() returns coordinates relative to the layout viewport.)
  • Prior to rendering, coordinates in e.g. the frame tree need these transforms (the resolution, and the offset between the visual and layout viewports) applied to get coordinates in screen space.
  • Conversely, when handling input events, we start with coordinates in screen space, and then want to do things like main thread hit-testing or dispatching DOM events, so we have to apply the reverse of these transforms. (The reverse of these transforms is often referred to as the "callback transform" in the code, because historically it was applied by code in APZCCallbackHelper.)
  • Currently, we apply the callback transform when events cross the process boundary from the chrome process to a content process.
  • This is problematic because scrollbars are drawn by the content process, but we don't want the resolution to apply to them.
  • So far, we've dealt with this by fixing up the size and position of scrollbars for rendering purposes in the compositor. Notably, we've not had to deal with this for hit testing purposes, because scrollbars cannot be interacted with on mobile.
  • Now that we are trying to bring pinch-zooming to desktop, where scrollbars are draggable, we need to solve this problem for hit-testing purposes.

The current envisioned solution, as discussed during the Whistler All Hands, is to move the place where apply the callback transform, from the process boundary, to the async zoom container. The async zoom container is a display item added during the containerless scrolling refactor, which wraps the display items for scrolled content and fixed content, but not the display items for scrollbars. This way, scrolled and fixed content are subject to the resolution, but not scrollbars are not, as desired.

In this bug, I've been trying to implement this move. Unfortunately, it hasn't been straightforward. One of the reasons for this is that coordinates are often expressed as being relative to a frame, but the async zoom container is a notion in the display list, not in the frame tree. I've been trying to map the notion of the resolution being "at the level of the async zoom container" onto the frame tree as follows:

  • the ViewportFrame is outside the resolution
  • the root scroll frame and its contents are inside the resolution
  • frames which are fixed to the viewport are inside the resolution

In particular, I've had to make things so that whenever a coordinate is expressed relative to the ViewportFrame, it has the resolution applied, but whenever it's expressed relative to a frame representing scrolled or fixed content, it does not have the resolution applied. This has necessitated adding code to apply or unapply the resolution in at least the following places:

  • nsDisplayAsyncZoomContainer::HitTest
  • ScrollFrameHelper::BuildDisplayList
  • elsewhere in display list building code where we enter fixed content
  • nsLayoutUtils::GetEventCoordinatesRelativeTo
  • most recently, nsLayoutUtils::TransformFrameRectToAncestor

With the most recent change to TransformFrameRectToAncestor, I've run into a significant snag: there is code like Element::GetBoundingClientRect() which computes coordinates relative to an element's containing block -- which, for e.g. the document element is the viewport frame -- and then exposes them to web content. With my current changes, asking for coordinates relative to the viewport frame will give you coordinates that have the resolution applied, but we definitely don't want that for e.g. documentElement.getBoundingClientRect().

It seems likely that this is one of many places where we compute coordinates relative to the viewport frame and then make the result available to web content, making the fact that the viewport frame is outside the resolution problematic.

So, I'd like to take a step back, and evaluate our options / consider possible alternatives:

  1. Press on with the current approach. This will mean modifying Element::GetBoundingClientRect(), and other places that ask for coordinates relative to the viewport frame in the process of computing a quantity exposed to web content. It's not clear what these modifications would look like, and there's a good chance that a lot of places will need to be modified.

  2. Represent the async zoom container as its own frame in the frame tree. This sounds like a very scary change to me, because it's a pretty fundamental modification to the frame tree model. It would also likely mean changing many places in the code to replace uses of the viewport frame with uses of the new frame type where appropriate.

  3. Continue unapplying the resolution at the process boundary, but re-apply it when entering scrollbars. This would largely keep things as-is prior to this bug, and solve the problem of hit-testing scrollbars in a more targeted way.

I'm leaning towards the third option; while it's the hackiest of the three, it's also the least likely to involve large-surface-area modifications. It's not entirely clear to me yet how re-applying the resolution when entering scrollbars would work, but it at least seems like a change localized to scrollbar frames rather than mucking with the viewport frame.

Matt / Timothy / Markus, I would appreciate any thoughts you have on this!

Flags: needinfo?(tnikkel)
Flags: needinfo?(mstange)
Flags: needinfo?(matt.woodrow)

The visual viewport is ultimately a post-layout painting effect, so I don't think it makes sense to try tie specific layout frame types to be inside/outside the resolution.

I think instead we want to distinguish between layout coordinates and painting/visual coordinates. Coordinates for the ViewportFrame could be either, depending on where they came from (or where we're sending them to).

I think ideally, all conversions using the frame tree would exclusively be in layout coordinates, and all using the display list would be visual. I don't know if that's possible though, since we implement getElementFromPoint using a display list, despite it wanting layout coordinates. I think we also use TransformFrameRectToAncestor (which is frame-based) for things that want visual coordinates (but maybe shouldn't?).

Is it possible to add a flag (or maybe a proper strongly typed coordinate and lots more templates), so that the entry points can specify which coordinate space they're intending to work in?

For display list building, this might be a flag on the builder (or implied from the existing painting/hit-testing flags?), which instructs ScrollFrameHelper::BuildDisplayList to skip building an nsDisplayAsyncZoomContainer when we're wanting to work in layout coordinates.

TransformFrameRectToAncestor would use the flag to determine if it should account for the resolution when crossing the scroll frame boundary.

Hopefully a lot of the code and functions are exclusively used for one or the other, so won't need the flag (but might benefit from naming clarifications and/or comments).

Flags: needinfo?(matt.woodrow)

Note that display list building already has the the concept of 'ignore viewport scrolling' (used for screenshots) which effectively bypasses the layout viewport (and visual viewport) and builds a display list relative to the top-left of the document (and doesn't try to draw scrollbars or the layout viewport clip). It also affects the code in ScrollFrameHelper::BuildDisplayList to change the resulting display list.

I think we could unify these concepts such that we have three top-level coordinate spaces (views? presentations? other name that isn't already overloaded?): document, layout-viewport, visual-viewport.

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)

For display list building, this might be a flag on the builder (or implied from the existing painting/hit-testing flags?), which instructs ScrollFrameHelper::BuildDisplayList to skip building an nsDisplayAsyncZoomContainer when we're wanting to work in layout coordinates.

I'm not sure that hit testing falls into either bucket, since hitting the viewport scrollbars requires working with visual coordinates (viewport scrollbars are positioned relative to the visual viewport, and their size relative to the screen is independent of the resolution), while hitting page content next to the viewport scrollbars requires working with layout coordinates (whose size relative to the screen is dependent of the resolution). There needs to be a resolution (un)application "in the middle of" the hit test procedure.

(In reply to Botond Ballo [:botond] from comment #22)

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)

For display list building, this might be a flag on the builder (or implied from the existing painting/hit-testing flags?), which instructs ScrollFrameHelper::BuildDisplayList to skip building an nsDisplayAsyncZoomContainer when we're wanting to work in layout coordinates.

I'm not sure that hit testing falls into either bucket, since hitting the viewport scrollbars requires working with visual coordinates (viewport scrollbars are positioned relative to the visual viewport, and their size relative to the screen is independent of the resolution), while hitting page content next to the viewport scrollbars requires working with layout coordinates (whose size relative to the screen is dependent of the resolution). There needs to be a resolution (un)application "in the middle of" the hit test procedure.

That's a good point! We have two types of hit testing: from JS (getElementFromPoint) and from user interaction (the main one you're dealing with here).

For the former, I think the solution I described earlier should work (no nsDisplayAsyncZoomContainer, everything in layout viewport coords). The latter should just want the same display list as we use for painting (page content within an nsDisplayAsyncZoomContainer, scrollbars outside).

I guess we will need some new flags to tell the DL builder exactly what setup we want.

I've started to rework my patch series to implement Matt's suggestions. It's still pretty incomplete, but it should compile and pass some tests, including hopefully some that have been failing before:

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

Flags: needinfo?(tnikkel)
Flags: needinfo?(mstange)

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=8b026a87235f781460294434c72d5f55026171ab

^ Starting to look better. Android gv-junit is now green, and there are much fewer web platform test failures. Still more to be done though.

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=8d56660c38323a300c4634f736417e873fcb8177

^ Even fewer wpt failures. I also have a fix for wpt8 locally which is not reflected in this push.

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=115b510c14d02cff36bec0d883210e12be349a13

^ All wpt failures fixed now except for a debug-only assertion wpt8.

The timeout of test_group_zoom.html on desktop seems to be related to the fact that we're getting into a reflow cycle. Here's a stack trace showing how processing one reflow causes us to request another one:

#0  mozilla::PresShell::FrameNeedsReflow (this=0x7f2f09687000, aFrame=0x7f2f096966d0, aIntrinsicDirty=mozilla::IntrinsicDirty::Resize, aBitToAdd=nsFrameState::NS_FRAME_IS_DIRTY, 
    aRootHandling=mozilla::ReflowRootHandling::InferFromBitToAdd) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:2682
#1  0x00007f2f1df03793 in mozilla::ScrollFrameHelper::MarkScrollbarsDirtyForReflow (this=0x7f2f09696248) at /home/botond/dev/mozilla/central/layout/generic/nsGfxScrollFrame.cpp:2418
#2  0x00007f2f1df6e2ac in nsHTMLScrollFrame::MarkScrollbarsDirtyForReflow (this=0x7f2f09696198) at /home/botond/dev/mozilla/central/layout/generic/nsGfxScrollFrame.h:1034
#3  0x00007f2f1dcf0198 in mozilla::PresShell::CompleteChangeToVisualViewportSize (this=0x7f2f09687000) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:10607
#4  0x00007f2f1dcf02a8 in mozilla::PresShell::SetVisualViewportSize (this=0x7f2f09687000, aWidth=125951, aHeight=84378) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:10628
#5  0x00007f2f1dd95612 in nsLayoutUtils::SetVisualViewportSize (aPresShell=0x7f2f09687000, aSize=...) at /home/botond/dev/mozilla/central/layout/base/nsLayoutUtils.cpp:9054
#6  0x00007f2f1dcb9143 in mozilla::GeckoMVMContext::SetVisualViewportSize (this=0x7f2f0d7b82e0, aSize=...) at /home/botond/dev/mozilla/central/layout/base/GeckoMVMContext.cpp:150
#7  0x00007f2f1dcbe558 in MobileViewportManager::UpdateVisualViewportSize (this=0x7f2f073f78e0, aDisplaySize=..., aZoom=...)
    at /home/botond/dev/mozilla/central/layout/base/MobileViewportManager.cpp:496
#8  0x00007f2f1dcbd016 in MobileViewportManager::RefreshVisualViewportSize (this=0x7f2f073f78e0) at /home/botond/dev/mozilla/central/layout/base/MobileViewportManager.cpp:541
#9  0x00007f2f1dcbcf96 in MobileViewportManager::ResolutionUpdated (this=0x7f2f073f78e0, aOrigin=mozilla::ResolutionChangeOrigin::MainThreadAdjustment)
    at /home/botond/dev/mozilla/central/layout/base/MobileViewportManager.cpp:122
#10 0x00007f2f1dcb8fea in mozilla::PresShell::SetResolutionAndScaleTo (this=0x7f2f09687000, aResolution=0.438742042, aOrigin=mozilla::ResolutionChangeOrigin::MainThreadAdjustment)
    at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:5136
#11 0x00007f2f1dcb8e66 in mozilla::GeckoMVMContext::SetResolutionAndScaleTo (this=0x7f2f0d7b82e0, aResolution=0.438742042, aOrigin=mozilla::ResolutionChangeOrigin::MainThreadAdjustment)
    at /home/botond/dev/mozilla/central/layout/base/GeckoMVMContext.cpp:145
#12 0x00007f2f1dcbe126 in MobileViewportManager::UpdateResolution (this=0x7f2f073f78e0, aViewportInfo=..., aDisplaySize=..., aViewportOrContentSize=..., aDisplayWidthChangeRatio=..., 
    aType=MobileViewportManager::UpdateType::ContentSize) at /home/botond/dev/mozilla/central/layout/base/MobileViewportManager.cpp:449
#13 0x00007f2f1dcbe934 in MobileViewportManager::ShrinkToDisplaySizeIfNeeded (this=0x7f2f073f78e0, aViewportInfo=..., aDisplaySize=...)
    at /home/botond/dev/mozilla/central/layout/base/MobileViewportManager.cpp:656
#14 0x00007f2f1df0d272 in mozilla::ScrollFrameHelper::ReflowFinished (this=0x7f2f09696248) at /home/botond/dev/mozilla/central/layout/generic/nsGfxScrollFrame.cpp:5905
#15 0x00007f2f1dcd3df4 in mozilla::PresShell::HandlePostedReflowCallbacks (this=0x7f2f09687000, aInterruptible=true) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:3848
#16 0x00007f2f1dccbfd9 in mozilla::PresShell::DidDoReflow (this=0x7f2f09687000, aInterruptible=true) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:8969
#17 0x00007f2f1dcd54e9 in mozilla::PresShell::ProcessReflowCommands (this=0x7f2f09687000, aInterruptible=true) at /home/botond/dev/mozilla/central/layout/base/PresShell.cpp:9357
Duplicate of this bug: 1601693

(In reply to Botond Ballo [:botond] from comment #29)

The timeout of test_group_zoom.html on desktop seems to be related to the fact that we're getting into a reflow cycle.

The reflow cycle is caused by the fact that some layout code is not factoring the resolution into the size of the scrollbar.

I have a candidate fix which is almost certainly wrong / incomplete. I'm pushing it to Try mostly so that I can debug it using Pernosco:

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

Duplicate of this bug: 1605477
Duplicate of this bug: 1607258

(In reply to Botond Ballo [:botond] from comment #31)

The reflow cycle is caused by the fact that some layout code is not factoring the resolution into the size of the scrollbar.

The code I had in mind was this code, which subtracts the size of the scrollbars from the total size available to lay out a scroll frame (layoutSize), to calculate the size available for the viewport.

I figured that this code needs to factor in the resolution because layoutSize is conceptually inside the resolution (since it is used to derive the size of the layout viewport in CSS pixels, which does not change as you zoom) while the scrollbars are conceptually outside the resolution.

However, thinking more about it, I am realizing that we do not want to size the layout viewport in such a way as to leave space for the scrollbars at the current resolution. (If we did that, the layout viewport would have a slightly different aspect ratio at every zoom level, as each time we are making space for a quantity that is fixed-width in outside-the-resolution units and therefore variable-width in inside-the-resolution units.) Rather, we want to size the layout viewport so that it coincides with the visual viewport at unit zoom (leaving space for the scrollbars at that zoom), and henceforth leave its size in CSS pixels / its aspect ratio alone.

So, I now believe the subtraction is correct as written, and the changes in the Try push from comment 31 (and this later Try push) are misguided.

Unfortunately, I don't remember what the causal relationship of this subtraction to the reflow cycle was, so I'll have to debug it again, and hopefully, with a better understanding of what the various quantities should be, come up with a proper fix.

After debugging the reflow cycle again, I now believe that it's a "legitimate" reflow cycle. What I mean by that is, the page layout and viewport sizing are such that the page fluctuates back and forth between needing a vertical scrollbar vs. not needing one.

The page in question (helper_basic_doubletap_zoom.html) contains content that is just a little too tall to fit into the viewport at the intrinsic scale computed based on the full viewport width (with no space set aside for a vertical scrollbar), thereby necessitating the placement of a vertical scrollbar. However, setting aside space for the vertical scrollbar requires us to zoom out a bit more to fit the content into the narrower viewport width, and that zooming out makes the content fit into the viewport completely, therefore making the vertical scrollbar unnecessary.

This circumstance comes from the interaction between mobile viewport sizing (which is causing us to change the zoom level when the viewport width changes) and layout scrollbars (which cause the viewport width to change depending on whether a vertical scrollbar needs to be shown).

Importantly, this combination never exists in practice -- mobile viewport sizing is only used in mobile, and layout scrollbars are only used on desktop -- and therefore it's not a combination we need to, or intend to, support at this time.

It does appear that some tests that run on desktop -- including the one I'm debugging -- have been using this combination. I believe we've just been lucky not to run into reflow cycles so far. I think the correct approach here is to modify our tests not to exercise this combination of features that's never used in production.

More concretely, I propose the following path forward:

  • Identify zooming-related tests that run on desktop and use mobile viewport sizing.
  • To maxime zooming-related test coverage on desktop: where possible, revise the tests not to use mobile viewport sizing. (Generally, this would mean using reftest-resolution or nsIDOMWindowUtils.setResolutionAndScaleTo() to trigger zooming rather than initial-scale in a meta-viewport tag.)
  • For the remaining test cases, split them out into a new test group which only runs on mobile.

(In reply to Botond Ballo [:botond] from comment #35)

  • For the remaining test cases, split them out into a new test group which only runs on mobile.

(In addition to mobile, we could also run this test group on macOS which uses overlay scrollbars.)

Depends on: 1608506

(In reply to Botond Ballo [:botond] from comment #35)

More concretely, I propose the following path forward: [...]

The interaction between mobile viewport sizing and layout scrollbars is a pre-existing issue, not directly related to this bug. We just happened to be lucky not to run into problems so far, and the coordinate calculation changes in this bug happen to trigger a problem.

So, the planned test changes can be made independently of this bug, i.e. they can land without waiting for the other patches in this bug. I will track them in bug 1608506.

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=f9f39ac338c704b368ac9646dc0ef3033271673e

^ With the patches from bug 1608506 and one additional fix, my patches for this bug are now passing all tests on desktop!

There are a handful of remaining test failures on Android. My investigation of them is currently blocked by bug 1608932.

Depends on: 1608932

Just FYI: you can workaround bug 1608932 by running the tests with --disable-e10s.

(In reply to Kris Taeleman (:kris/kris) from comment #39)

Just FYI: you can workaround bug 1608932 by running the tests with --disable-e10s.

Thanks for the suggestion! That does indeed seem to work around bug 1608932.

Of the 7 test failures I'm seeing on Android, I am able to reproduce 2 locally (dom/tests/mochitest/general/test_bug1012662_editor.html and dom/tests/mochitest/general/test_bug1012662_noeditor.html). I'm currently investigating them.

(In reply to Botond Ballo [:botond] from comment #40)

Of the 7 test failures I'm seeing on Android, I am able to reproduce 2 locally (dom/tests/mochitest/general/test_bug1012662_editor.html and dom/tests/mochitest/general/test_bug1012662_noeditor.html). I'm currently investigating them.

I was able to get test_bug1012662_editor.html passing locally, but it's still failing in automation (albeit with different failures?):

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=1a321ce4dd45a87c8ecd824f854f1fea99117419

(In reply to Botond Ballo [:botond] from comment #41)

I was able to get test_bug1012662_editor.html passing locally, but it's still failing in automation (albeit with different failures?):

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=1a321ce4dd45a87c8ecd824f854f1fea99117419&selectedJob=285687657

This push also introduces some other failures, including in the mochitest harness sanity test, which I was able to repro locally and fix. Now down to 6 failures, 1 of which I can repro locally:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=2cd882e6da17b514b9187034cf6b5349fe28effc

I have a fix for the failure I could repro locally. Now down to 5 failures:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=c2844907c36e7783140912832ba69bb9d55d8b38

Moreover, I found a way to repro one of these locally, by running the entire directory rather than just the individual test.

Down to 4 failures:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=7f6519123f452c6116308cef12a0d80cf0e5ca77

Unfortunately, I can't repro any of these locally, so I'll have to debug them over Try.

Blocks: 1619186
Blocks: desktop-zoom-nightly

Just wanted to provide an update on progress here.

The Android test failure that I've been debugging is test_clipboard_editor.html. This is a monstrosity of a test, with many cases and sub-cases, so there may well be multiple issues with it, but I'll describe the one I've been investigating so far.

The test synthesizes a click on various types of input elements, synthesizes key events to trigger clipboard actions (cut/copy/paste), and then checks to see if the expected contents have been transferred to the clipboard. The usual failure mode is related to the synthesized click targeting the wrong element, and other failures cascade from there.

The targeting seems to fail when the targeted element is not inside the visual viewport. (That in turn usually occurs because a previous sub-case transferred focus to another input element, which triggered "zoom to focused input" logic.) In such cases, the synthesized event coordinates (which start in layout coordinates, as they come from getBoundingClientRect() and such) are transformed into visual coordinates prior to being stored on the widget event. The visual coordinates can be negative (if e.g. the target element is to the left of the visual viewport). Now, the idea is the hit test will transform them back into layout coordinates, but this occurs when we cross the async zoom container (AZC) on our way down into the display list. However, the negative coordinates are discarded as "out of bounds" before we reach the AZC (in fact, the "for event delivery" display list that's built doesn't even include the AZC, because the negative coordinate doesn't intersect with any display item (the relevant check appears to be this one)).

I made some attempts to work around this in the display list building code, but it started looking pretty hairy. I thought about it some more today, and it occurred to me that perhaps hit tests outside the visual viewport shouldn't be expected to succeed in the first place. Real user interaction wouldn't trigger them, and the purpose of tests is to emulate real user interaction, not exercise scenarios which can't actually occur.

So, I'm now attempting to work around this in the test, by using scrollIntoView() to bring input elements into the visual viewport (which, importantly, scrollIntoView() now handles) before synthesizing clicks that target them. Let's see how that works out on Try:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=d1abe9cf19e7e908902e0d72395404b6cfd88c2b

(In reply to Botond Ballo [:botond] from comment #36)

(In reply to Botond Ballo [:botond] from comment #35)

  • For the remaining test cases, split them out into a new test group which only runs on mobile.

(In addition to mobile, we could also run this test group on macOS which uses overlay scrollbars.)

Overlay scrollbars are disabled on the macOS test machines via a system setting. But they could be enabled with a Gecko pref.

(In reply to Botond Ballo [:botond] from comment #46)

So, I'm now attempting to work around this in the test, by using scrollIntoView() to bring input elements into the visual viewport (which, importantly, scrollIntoView() now handles) before synthesizing clicks that target them. Let's see how that works out on Try:

Adding scrollIntoView() made things better, but some sub-cases were still failing. Further investigation made me realize that the scrolling into view is actually supposed to be handled by the "zoom to focused input" logic, but the test isn't waiting for such zooming to complete; in fact, we were getting artifacts like the "zoom to focused input" operation for a previous sub-case interfering with the current sub-case.

Making the test wait properly for a "zoom to focused input" operation to complete doesn't look to be straightforward, because there are multiple points of asynchronicity, including in GeckoView Java / JS code. Instead, I opted for just disabling "zoom to focused input" entirely for this test, which seems to make the entire test pass.

I also fixed one more test failure which required a code change. New Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=2dd4694cb3e06017cb0444342d4f5532d44e224e

We are now down to the following test failures:

  • dom/html/test/forms/test_input_file_picker.html
  • dom/html/test/forms/test_input_range_mouse_and_touch_events.html
  • dom/html/test/test_bug514856.html

(I'm aware that's more than the 2 we had in comment 45; some of these failures are intermittent / seem to come and go.)

I'm not able to reproduce any of these locally, so I'll likely have to debug them over Try.

(In reply to Botond Ballo [:botond] from comment #48)

Adding scrollIntoView() made things better, but some sub-cases were still failing. Further investigation made me realize that the scrolling into view is actually supposed to be handled by the "zoom to focused input" logic, but the test isn't waiting for such zooming to complete; in fact, we were getting artifacts like the "zoom to focused input" operation for a previous sub-case interfering with the current sub-case.

Making the test wait properly for a "zoom to focused input" operation to complete doesn't look to be straightforward, because there are multiple points of asynchronicity, including in GeckoView Java / JS code. Instead, I opted for just disabling "zoom to focused input" entirely for this test, which seems to make the entire test pass.

test_input_file_picker.html also appears to be using the same tactic to get things to be visible:

https://searchfox.org/mozilla-central/rev/5a52cec97c41ae1eda9412dfe6f4099a9af4c7dd/dom/html/test/forms/test_input_file_picker.html#153

I'll try disabling via that pref for this test as well. Failing that I'll just try manually scrolling the page (via setting scrollTop or something).

(In reply to Timothy Nikkel (:tnikkel) from comment #49)

I'll try disabling via that pref for this test as well. Failing that I'll just try manually scrolling the page (via setting scrollTop or something).

Not that. The reason is that the test calls synthesizeMouse(element, x, y) for an element that is split between two lines, and synthesizeMouse uses getBoundingClientRect to determine where to send the event, but that doesn't work if an element is split between lines. Using the rect from element.getClientRects with the lowest y coord should fix it.

(In reply to Timothy Nikkel (:tnikkel) from comment #50)

Not that. The reason is that the test calls synthesizeMouse(element, x, y) for an element that is split between two lines, and synthesizeMouse uses getBoundingClientRect to determine where to send the event, but that doesn't work if an element is split between lines. Using the rect from element.getClientRects with the lowest y coord should fix it.

Wouldn't that cause the test to fail even before these changes?

(In reply to Botond Ballo [:botond] from comment #51)

(In reply to Timothy Nikkel (:tnikkel) from comment #50)

Not that. The reason is that the test calls synthesizeMouse(element, x, y) for an element that is split between two lines, and synthesizeMouse uses getBoundingClientRect to determine where to send the event, but that doesn't work if an element is split between lines. Using the rect from element.getClientRects with the lowest y coord should fix it.

Wouldn't that cause the test to fail even before these changes?

I don't have an explanation for that. But I'm pretty certain it is at least one of the problems as I dumped a window snapshot in the log, the coords of the click, and the result of elementFromPoint for those coords.

That said, with my fixes it only passes some of the time (even though elementFromPoint returns the right element) so there must be more going on.

Duplicate of this bug: 1621367

(In reply to Timothy Nikkel (:tnikkel) from comment #52)

(In reply to Botond Ballo [:botond] from comment #51)

(In reply to Timothy Nikkel (:tnikkel) from comment #50)

Not that. The reason is that the test calls synthesizeMouse(element, x, y) for an element that is split between two lines, and synthesizeMouse uses getBoundingClientRect to determine where to send the event, but that doesn't work if an element is split between lines. Using the rect from element.getClientRects with the lowest y coord should fix it.

Wouldn't that cause the test to fail even before these changes?

I know more on this now. I dumped the id of the nearest ancestor node with an id in HTMLInputElement::MaybeInitPickers and it is being called for label-2 when we are on label-3. This makes sense, that's where we would expect the click to go with the erroneous coords. So then why doesn't the same thing happen after the changes in this bug? Not sure yet.

(In reply to Timothy Nikkel (:tnikkel) from comment #54)

I know more on this now. I dumped the id of the nearest ancestor node with an id in HTMLInputElement::MaybeInitPickers and it is being called for label-2 when we are on label-3. This makes sense, that's where we would expect the click to go with the erroneous coords. So then why doesn't the same thing happen after the changes in this bug? Not sure yet.

With this bug patches the mouse down from the synthesized mouse click goes to label-2, but the coords (when they get to GetFramesForArea) are different for the mouse up and so we hit the <html> element and no mouse click is generated.

(In reply to Timothy Nikkel (:tnikkel) from comment #55)

With this bug patches the mouse down from the synthesized mouse click goes to label-2, but the coords (when they get to GetFramesForArea) are different for the mouse up and so we hit the <html> element and no mouse click is generated.

It looks like the coords are different because we are using the iframe's viewport frame as the aFrame parameter to GetFramesForArea. It's expected that the coordinates will be different than if we are using the root document's viewport as the aFrame parameter (however, the conversion may be numerically wrong for some reason).

Blocks: 1621740
Depends on: 1621803

fresh try with bug 1621803 fixed based on m-c from today and patches I pulled from botond's push in comment 48
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1fa3295a14b6cd219967f3c4ee69f471e5382d

It looks like bug 1621803 fixed the failures of all three tests mentioned in comment 48. However, other failures have appeared:

  • dom/html/test/test_bug500885.html is failing intermittently. That's likely always been the case, and I just happened to get lucky and not get any failures in my push from comment 48.
  • layout/style/test/test_pseudoelement_state.html seems to have started failing as a consequence of bug 1621803. Possibly that bug was covering up another issue in that test which is now revealed.
  • layout/base/tests/test_bug977003.html and layout/base/tests/test_event_target_radius.html are new failures that appeared when rebasing from the baseline of the comment 48 push to a more recent m-c. I suspect they are related to the enablement of event retargeting in that interval (bug 1618532).

I can repro all of the above locally except for dom/html/test/test_bug500885.html.

(In reply to Botond Ballo [:botond] from comment #58)

  • layout/style/test/test_pseudoelement_state.html seems to have started failing as a consequence of bug 1621803. Possibly that bug was covering up another issue in that test which is now revealed.

I have failures on my try in this test without the matrix fix. Perhaps caused by something between your comment 48 push and when I rebased your patches onto m-c last weekend?

(In reply to Timothy Nikkel (:tnikkel) from comment #60)

I have failures on my try in this test without the matrix fix. Perhaps caused by something between your comment 48 push and when I rebased your patches onto m-c last weekend?

Oh, interesting.

The reason I thought it was caused by bug 1621803, is that it was showing up in a Try push with the bug 1621803 patch but before any rebase compared to the comment 48 baseline.

However, I've now done some retriggers on the original comment 48 push and I'm seeing it there too. I guess it's just an intermittent whose frequency varies somewhat (possibly became more frequent with the rebase).

(In reply to Botond Ballo [:botond] from comment #58)

  • layout/base/tests/test_bug977003.html and layout/base/tests/test_event_target_radius.html are new failures that appeared when rebasing from the baseline of the comment 48 push to a more recent m-c. I suspect they are related to the enablement of event retargeting in that interval (bug 1618532).

FWIW, disabling the pref from bug 1618532 only makes the test_bug977003.html failure go away, the others are unaffected.

I have a fix for layout/style/test/test_pseudoelement_state.html:

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

I think the problem with test_event_target_radius.html might be that the coords that FindFrameTargetedByInputEvent deals with go from inside the resolution to outside of it when applying botond's patch series, but when we expand to the rect based on the prefs we factor in the resolution when converting from mm to app units. The test also includes the resolution, not sure if that is kosher or not.

Yeah, that was the issue, the test's use of resolution seems to be fine (in that the test passes without changing that).

There is one remaining issue with test_event_target_radius.html: this line

https://searchfox.org/mozilla-central/rev/3f8c67d7fd836d559491e3fe497bc739f707c1a6/layout/base/tests/test_event_target_radius.html#192

places a click exactly halfway between two elements, and then relies on one element being above the other to break the tie. This starts to fail with Botond's patches because of rounding. The wrong element ends up being 2 app units closer than the other. There are at least two places where we have potential rounding: we have to remove resolution from the click point, and then we have to remove the resolution from the border box of the potential elements. Suggestions for fixing this in the test to be more robust? Should we just force the resolution of the test to 1? Or a nice even number of 2 or 0.5 (so that we are still testing resolution!=1?

(In reply to Timothy Nikkel (:tnikkel) from comment #65)

Created attachment 9133476 [details] [diff] [review]
Ignore resolution when computing size of mm

Although that patch definitely fixes the test for me when run locally it does not do anything for the test on try server. I'm going to move on and look at test_bug977003.html for now.

I think it's valuable to test resolution != 1 for this particular test. But yeah we can set the resolution to 0.5 or some other "round" number via a meta tag and that would be fine.

That might be harder than I thought: the resolution is on the root document which holds the test harness, the actual test is in an iframe, I don't think we support more than one zoomed document? So then we'd have to change the resolution for the test harness and therefore every single mochitest, which seems like it will tease out a bunch more problems.

The problem with test_bug977003.html happens here

https://searchfox.org/mozilla-central/rev/3f8c67d7fd836d559491e3fe497bc739f707c1a6/layout/base/PositionedEventTargeting.cpp#487

Note that nsLayoutUtils::TransformPoint takes raw frames and not RelativeTo (frame, viewport type) pairs. So the point gets transformed incorrectly. We need a new function to do the transform, I just pretended the svg text frame case didn't exist for now.

I also noticed what seems like a bug in nsLayoutUtils::TransformAncestorPointToFrame: in the normal case it completely ignores the frame on aAncestor and passes nullptr to TransformGfxPointFromAncestor. This is usually okay because the main user of nsLayoutUtils::TransformAncestorPointToFrame is TransformRootPointToFrame which passes null for the ancestor. The only other users is an svg text frame case, so that probably doesn't come up in tests. I'll push a patch for that separately to see if it fixes or breaks anything.

(In reply to Timothy Nikkel (:tnikkel) from comment #68)

That might be harder than I thought: the resolution is on the root document which holds the test harness, the actual test is in an iframe, I don't think we support more than one zoomed document? So then we'd have to change the resolution for the test harness and therefore every single mochitest, which seems like it will tease out a bunch more problems.

We can spawn the test out into a new window, maybe? The same way the APZ test_group_* tests work.

Thanks!

My patch to fix test_bug977003.html also fixes test_event_target_radius.html if I include my test_event_target_radius.html patch. So we are down to one failure: dom/html/test/test_bug500885.html
I will probably start on looking into that today unless someone else beats me to it.

(In reply to Timothy Nikkel (:tnikkel) from comment #64)

I think the problem with test_event_target_radius.html might be that the coords that FindFrameTargetedByInputEvent deals with go from inside the resolution to outside of it when applying botond's patch series, but when we expand to the rect based on the prefs we factor in the resolution when converting from mm to app units.

Indeed, nice find!

The coordinates coming into FindFrameTargetedByInputEvent() can actually be in visual or layout coords, depending on the input frame. We'll probably need to propagate that information into the function, and factor in the resolution or not when converting from mm.

The test also includes the resolution, not sure if that is kosher or not.

The test including the resolution is appropriate, because it's converting from mm (which are physical units outside the resolution) to coordinates passed into synthesizeMouse() from EventUtils.js (which are CSS coords, inside the resolution).

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #70)

(In reply to Timothy Nikkel (:tnikkel) from comment #68)

That might be harder than I thought: the resolution is on the root document which holds the test harness, the actual test is in an iframe, I don't think we support more than one zoomed document? So then we'd have to change the resolution for the test harness and therefore every single mochitest, which seems like it will tease out a bunch more problems.

We can spawn the test out into a new window, maybe? The same way the APZ test_group_* tests work.

We may be able to do something simpler: SpecialPowers.getDOMWindowUtils(window.top).setResolutionAndScaleTo().

(In reply to Timothy Nikkel (:tnikkel) from comment #69)

The problem with test_bug977003.html happens here

https://searchfox.org/mozilla-central/rev/3f8c67d7fd836d559491e3fe497bc739f707c1a6/layout/base/PositionedEventTargeting.cpp#487

Note that nsLayoutUtils::TransformPoint takes raw frames and not RelativeTo (frame, viewport type) pairs. So the point gets transformed incorrectly.

Good catch on this one too. I wrote a patch to "upgrade" TransformPoint to take RelativeTo arguments to fix this.

With that + the AppUnitsToMM patch + the setResolutionAndScaleTo() thing, I have both layout tests passing locally.

At long last, I have a green Try push! \o/

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=e8af0ce892e43ffef5ba572be2d78273fe4601ed

Thank you Timothy for lending a helping hand with the last few failures!

Next steps here are:

  • clean up the patch series a bit
  • post it for review
Depends on: 1623476

I'm going to spin off part of the patch series (containing some light refactoring and test changes which can land independently of the functional changes) into bug 1623476.

Other than the 10 patches I've split off into bug 1623476, there are 26 other patches in my patch series, which need to land together. I've looked through them and made note of places that need commenting and other cleanup. The result is not going to be a work of art by any means, but I'll do my best to make them easily understandable and reviewable. I do plan to merge some of the patches, so the final number going up for review might be around 20.

Whiteboard: desktop-zoom-nightly
Blocks: 1619402

As this is a rather long patch series, I'm going to start posting patches for review in small batches, so that earlier patches can be reviewed in parallel with me cleaning up later patches.

Prior to this bug, it was necessary to handle non-e10s specially, because the
resolution was being unapplied at the process boundary, and in non-e10s there
was no process boundary.

The remaining patches in this bug move the resolution unapplication away from
the process boundary in all cases, making special handling for non-e10s
unnecessary.

This is to facilitate call sites that need to incorporate the transform into
a larger transform matrix rather than immediately applying the callback
transform to a point.

Depends on D68274

The implementation was already only using the scroll id, so there is no
functional change, but this change will make it easier to new call sites
to come up with the function's inputs.

Depends on D68276

Attachment #9133476 - Attachment is obsolete: true
Attachment #9133511 - Attachment is obsolete: true

This function (and helper functions that wrap it) will be used extensively
throughout layout code, so keeping it in APZCCallbackHelper seems awkward.

nsLayoutUtils would also be a reasonable place but has the downside that
adding a new function to it triggers recompiling the world.

Depends on D68277

We will be applying and unapplying this transform in many places, and
thinking about those operations as "applying the callback transform" and
"unapplying the callback transform" is not very intuitive, especially since
applying the callback transform involves unapplying the resolution.

Rather, going forward we will use the terminology "visual coordinates"
and "layout coordinates" and use this function to convert back and forth
between them.

ApplyCallbackTransform() is not renamed here because subsequent patches will
transition its callers to use GetVisualToLayoutTransform() and
ApplyCallbackTransform() will be removed.

Depends on D68296

Note that the propagation of the target guid to places where the transform
will be applied is best-effort at the moment. In particular, the
InputAPZContext will result in the correct guid being available in places
that are called synchronously from the Recv*() functions, but not places
called asynhcronously (e.g. via DelayedFireSingleTapEvent).

To mitigate this, places where the transform is applied fall back on the
RCD-RSF if a guid is not available via InputAPZContext (added in a
subsequent patch).

The cases that this gets wrong are fairly edge casey (it requires (a) an
asynchronous codepath, (b) an event targeting a subframe, and (c) that
subframe having a "could not accept the APZ scroll position" transform),
so we just punt on them for now. If it turns out to be important to handle,
then options for doing so include (1) propagating the guid through each of
the affected asynchronous codepaths, or (2) attaching the guid to the event
itself.

Depends on D68275

Attachment #9135815 - Attachment description: Bug 1556556 - Remove some cruft related to handling the resolution in non-e10s setups. r=tnikkel → Bug 1556556 - Remove some cruft related to handling the resolution in non-e10s setups. r?tnikkel
Attachment #9135816 - Attachment description: Bug 1556556 - Clarify the documentation of APZCCallbackHelper::ApplyCallbackTransform(). r=kats → Bug 1556556 - Clarify the documentation of APZCCallbackHelper::ApplyCallbackTransform(). r?kats
Attachment #9135817 - Attachment description: Bug 1556556 - Factor out an APZCCallbackHelper::GetCallbackTransform() helper. r=kats → Bug 1556556 - Factor out an APZCCallbackHelper::GetCallbackTransform() helper. r?kats
Attachment #9135818 - Attachment description: Bug 1556556 - Remove no-longer-used overloads of ApplyCallbackTransform(). r=kats → Bug 1556556 - Remove APZCCallbackHelper::ApplyCallbackTransform(). r?kats
Attachment #9135820 - Attachment description: Bug 1556556 - Have Get/ApplyCallbackTransform take just a scroll id rather than an entire guid. r=kats → Bug 1556556 - Have GetCallbackTransform take just a scroll id rather than an entire guid. r?kats
Attachment #9135860 - Attachment description: Bug 1556556 - Move GetCallbackTransform() into a new ViewportUtils class. r=kats → Bug 1556556 - Move GetCallbackTransform() into a new ViewportUtils class. r?kats
Attachment #9135862 - Attachment description: Bug 1556556 - Rename GetCallbackTransform() to GetVisualToLayoutTransform(). r=kats → Bug 1556556 - Rename GetCallbackTransform() to GetVisualToLayoutTransform(). r?kats
Duplicate of this bug: 1224748
Attachment #9136628 - Attachment description: Bug 1556556 - Add ViewportUtils::IsZoomedContentRoot(). r?kats,mattwoodrow → Bug 1556556 - Add ViewportUtils::IsZoomedContentRoot(). r?kats!,mattwoodrow!
Attachment #9136629 - Attachment description: Bug 1556556 - Apply the visual-to-layout transform during display list building and display list based hit testing. r?kats,mattwoodrow → Bug 1556556 - Apply the visual-to-layout transform during display list building and display list based hit testing. r?kats!,mattwoodrow!

Use it in document.elementFromPoint().

Depends on D68913

We were using "during event delivery" as a proxy for this, but it was an inaccurate proxy.

Depends on D68914

This is already the case for real input events since that's how they
arrive from APZ, and we are no longer changing the coordinates at
the process boundary.

For synthesized events, a future patch will add layout-to-visual
converions to code that sets mRefPoint as appropriate.

Depends on D68916

This "upgrades" various nsLayoutUtils functions which take as inputs
a set of coordinates and a frame that the coordinates are relative to,
to accept a RelativeTo object instead of a frame.

Most of the patch is just dumb propagation, but the few places where
we use an explicit ViewportType::Visual are important. There are
probably a few other places I've overlooked, but this seems to cover
the important ones that come up commonly.

There are undoubtedly other functions into which we can propagate
RelativeTo, in this patch I've propagated it as far as necessary
for my needs in this bug (mainly GetTransformToAncestor() and
GetEventCoordinatesRelativeTo()).

Depends on D68917

This is the "core" change of the patch series, which causes most
existing layout codepaths to correctly factor in the visual to
layout transform (or its inverse), as long as the callers correctly
propagate it in the correct ViewportType.

Depends on D68919

The idea here is:

  • The incoming point comes from WidgetEvent::mRefPoint which is in
    visual coordinates.

  • Depending on the value of the target RelativeTo parameter, we
    may need to convert this to layout coordinates.

  • In the fast-path, we do a direct check on the viewport type
    and apply the visual-to-layout transform if appropriate.

  • In the slow path, we rely on TransformRootPointToFrame() (which
    calls GetTransformToAnestor()) to include the visual-to-layout
    transform if appropriate, by correctly passing in
    ViewportType::Visual as the starting point.

    • To make sure we get into TransformRootPointToFrame(), we
      set transformFound if we'll be crossing a zoomed content root.

Depends on D68920

This is still not everything, but all the core changes are posted now. The rest is mostly odds and ends and test changes.

Blocks: 1627365

This test is failing due to the rounding error described in bug 1627365.
As this is a web platform test, it seems inappropriate to modify the
test itself upstream to avoid a Firefox-specific rounding error.

Depends on D69640

This is the anti-climactic end of the patch series.

I set out in this bug to get this test case to pass with apz.allow_zooming.
It took all these changes to do so without regressing other tests.

Depends on D69642

Ok, this is the full patch series now :) It ended up consisting of 26 patches.

Blocks: 1626080
Blocks: 1625948

Adding some more dependencies that might be fixed by this bug (or at least probably shouldn't be worked on until this one is done).

Blocks: 1244284, 1254030, 1622040, 1257579
Attachment #9136963 - Attachment description: Bug 1556556 - Include the layout-to-visual transform when crossing a zoomed content root in GetTransformToAncestor(). r=kats!,mattwoodrow! → Bug 1556556 - Include the layout-to-visual transform for a zoomed content root in GetTransformMatrix(). r=kats!,mattwoodrow!
Duplicate of this bug: 1631309
Blocks: 1631568
Whiteboard: desktop-zoom-nightly → apz-planning
Blocks: 1632789
Blocks: 1632268
No longer blocks: 1632268

All the patches have now passed code review -- thank you to my reviewers!

Here is a freshly rebased Try push of this + bug 1631568: https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=4772a11b669b2b1386f3c314b91223bee6e68c23

We have some crashiness in dom/events/test/test_coalesce_touchmove.html, but interestingly, only in opt builds. Seems to be related to the nsEventStatus_eSentinel patch. I didn't catch it previously because I was doing debug-only Try pushes.

This is looking better (with the nsEventStatus_eSentinel patch taken out for now): https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=ce47a2409b90fb305af797f1b0a6cf6ede468191

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9553e99137ea
Remove some cruft related to handling the resolution in non-e10s setups. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/f15334a3e803
Clarify the documentation of APZCCallbackHelper::ApplyCallbackTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/903c4de34e7a
Factor out an APZCCallbackHelper::GetCallbackTransform() helper. r=kats
https://hg.mozilla.org/integration/autoland/rev/d75a4ecb0d47
Change the default value of InputAPZContext::sApzResponse to nsEventStatus_eSentinel. r=kats
https://hg.mozilla.org/integration/autoland/rev/80fcb7e71188
Remove applications of the visual-to-layout transform at the process boundary (and equivalent places for non-e10s). r=kats
https://hg.mozilla.org/integration/autoland/rev/95c54c023779
Remove APZCCallbackHelper::ApplyCallbackTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/d11dbf6403a5
Have GetCallbackTransform take just a scroll id rather than an entire guid. r=kats
https://hg.mozilla.org/integration/autoland/rev/4312b2b5dda8
Move GetCallbackTransform() into a new ViewportUtils class. r=kats
https://hg.mozilla.org/integration/autoland/rev/629c58a9166b
Rename GetCallbackTransform() to GetVisualToLayoutTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/be8f5eb58460
Add some VisualToLayout() and LayoutToVisual() wrappers to ViewportUtils. r=kats
https://hg.mozilla.org/integration/autoland/rev/387320881876
Support both CSS and LayoutDevice units in GetVisualToLayoutTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/204881474cc1
Add ViewportUtils::IsZoomedContentRoot(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/24056e47183d
Apply the visual-to-layout transform during display list building and display list based hit testing. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fccd1d3c7189
Remove many uses of IgnoreRootScrollFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a7bd34d961bb
Add a "relative to layout viewport" option for display list building. r=mstange,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/93190ae4f5ff
Use the "relative to layout viewport" flag to determine when to divide the composition bounds clip by the resolution. r=mstange
https://hg.mozilla.org/integration/autoland/rev/700447945b4e
Introduce ViewportType and RelativeTo. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2c5d55a1f0b1
Document WidgetEvent::mRefPoint as being in visual coordinates. r=kats
https://hg.mozilla.org/integration/autoland/rev/62a223d057d2
Propagate RelativeTo far and wide. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2c62e61d9054
Use FrameForPointOption::IsRelativeToLayoutViewport when passing layout-relative coordinates to GetFrameForPoint(). r=kats
https://hg.mozilla.org/integration/autoland/rev/20d72a334530
Include the layout-to-visual transform for a zoomed content root in GetTransformMatrix(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/867946cf05bb
Handle visual/layout coordinate conversions correctly in GetEventCoordinatesRelativeTo(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/05a6a581e755
Perform app unit to milimetre conversion correctly in PositionedEventTargeting. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/98e26bc98b85
Miscellaneous fixes related to event coordinate transformations. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9c717eb067b8
Convert mRefPoint to visual coordinates for synthesized events. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/03c2c25d8023
Update test expectations for elementTiming.html. r=mstange
https://hg.mozilla.org/integration/autoland/rev/38ffc6215bbf
Update test expectations for tests which are now passing. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ed3c1e85d5e3
Run helper_drag_root_scrollbar.html with zooming enabled as well. r=kats

Backed out for multiple mochitest failures.

backout: https://hg.mozilla.org/integration/autoland/rev/95fac5915a31c26ced5c9634bc25617343d88d80

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=edd529f7a9c5093edf9cf178bac95b9bab19e683&searchStr=mochitest

failure logs:

Flags: needinfo?(botond)

Assertion failure: ViewportUtils::IsZoomedContentRoot(f), at /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:1861 -- https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299752669&repo=autoland&lineNumber=15767

(In reply to Natalia Csoregi [:nataliaCs] from comment #115)

These two are the nsEventStatus_eSentinel issue. I reordered my patch series locally to exclude that patch and resubmitted to Phabricator, but overlooked the fact that Phabricator doesn't actually pick up on the reorder and needs the parent/child links broken manually...

The other two will require investigation. It looks like they are somewhat low frequency intermittents.

Flags: needinfo?(botond)

(In reply to Botond Ballo [:botond] from comment #112)

We have some crashiness in dom/events/test/test_coalesce_touchmove.html, but interestingly, only in opt builds. Seems to be related to the nsEventStatus_eSentinel patch. I didn't catch it previously because I was doing debug-only Try pushes.

What's happening here is that JS code running in the parent process is synthesizing a touch event destined for a content process using nsIDOMWindowUtils.sendTouchEvent() (the non-native event sythesization mechanism).

Since the event is targeting a content process, the event dispatch goes through BrowserParent::SendRealTouchEvent(), which queries InputAPZContext::GetApzResponse() here, to tag the event with the APZ response. However, since the event was synthesized via the non-native mechanism, it did not go through APZ, so there is no InputAPZContext on the stack, and thus the default value of nsEventStatus_eSentinel is picked up.

Comment on attachment 9138939 [details]
Bug 1556556 - Change the default value of InputAPZContext::sApzResponse to nsEventStatus_eSentinel. r=kats

Revision D70084 was moved to bug 1634206. Setting attachment 9138939 [details] to obsolete.

Attachment #9138939 - Attachment is obsolete: true

(In reply to Botond Ballo [:botond] from comment #118)

What's happening here is that JS code running in the parent process is synthesizing a touch event destined for a content process using nsIDOMWindowUtils.sendTouchEvent() (the non-native event sythesization mechanism).

Since the event is targeting a content process, the event dispatch goes through BrowserParent::SendRealTouchEvent(), which queries InputAPZContext::GetApzResponse() here, to tag the event with the APZ response. However, since the event was synthesized via the non-native mechanism, it did not go through APZ, so there is no InputAPZContext on the stack, and thus the default value of nsEventStatus_eSentinel is picked up.

I have a fix for this, but I spun it off into bug 1634206.

Blocks: 1634490

(In reply to Botond Ballo [:botond] from comment #117)

The other two will require investigation. It looks like they are somewhat low frequency intermittents.

The remaining two failures were:

  • A Mac intermittent related to OOP iframes, fixed by changing an IsRootContentDocument() check in https://phabricator.services.mozilla.com/D68728 to IsRootContentDocumentCrossProcess().
  • An Android intermittent in dom/html/test/forms/test_input_color_picker_popup.html (and sometimes related tests), which looks to be more zoomToFocusedInput() fallout from previous tests, which I'm working around for now by disabling zoom-to-focused-input behaviour in dom/html/test/forms.
Blocks: 1625925
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15d1011493f4
Remove some cruft related to handling the resolution in non-e10s setups. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/740a8b776f5c
Clarify the documentation of APZCCallbackHelper::ApplyCallbackTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/22b125b810a7
Factor out an APZCCallbackHelper::GetCallbackTransform() helper. r=kats
https://hg.mozilla.org/integration/autoland/rev/a03147de488b
Remove APZCCallbackHelper::ApplyCallbackTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/8206cf323d5b
Have GetCallbackTransform take just a scroll id rather than an entire guid. r=kats
https://hg.mozilla.org/integration/autoland/rev/bda050135c64
Move GetCallbackTransform() into a new ViewportUtils class. r=kats
https://hg.mozilla.org/integration/autoland/rev/071e6527d743
Rename GetCallbackTransform() to GetVisualToLayoutTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/ff865d99f67e
Remove applications of the visual-to-layout transform at the process boundary (and equivalent places for non-e10s). r=kats
https://hg.mozilla.org/integration/autoland/rev/b76b8d0b94ca
Add some VisualToLayout() and LayoutToVisual() wrappers to ViewportUtils. r=kats
https://hg.mozilla.org/integration/autoland/rev/25d6c0b90ad2
Support both CSS and LayoutDevice units in GetVisualToLayoutTransform(). r=kats
https://hg.mozilla.org/integration/autoland/rev/7782bd4b305a
Add ViewportUtils::IsZoomedContentRoot(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3718d0d54e65
Apply the visual-to-layout transform during display list building and display list based hit testing. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/87f45543cf69
Remove many uses of IgnoreRootScrollFrame. r=mstange,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/62a47652d47a
Add a "relative to layout viewport" option for display list building. r=mstange,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1b0051225524
Use the "relative to layout viewport" flag to determine when to divide the composition bounds clip by the resolution. r=mstange
https://hg.mozilla.org/integration/autoland/rev/bc416b15fb11
Introduce ViewportType and RelativeTo. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c57fce62acf3
Document WidgetEvent::mRefPoint as being in visual coordinates. r=kats
https://hg.mozilla.org/integration/autoland/rev/72cf169efa43
Propagate RelativeTo far and wide. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7878684d1f8b
Use FrameForPointOption::IsRelativeToLayoutViewport when passing layout-relative coordinates to GetFrameForPoint(). r=kats
https://hg.mozilla.org/integration/autoland/rev/6aa68ae33358
Include the layout-to-visual transform for a zoomed content root in GetTransformMatrix(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c4dcb019b6ba
Handle visual/layout coordinate conversions correctly in GetEventCoordinatesRelativeTo(). r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/f40298497136
Perform app unit to milimetre conversion correctly in PositionedEventTargeting. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/185da341dbe6
Miscellaneous fixes related to event coordinate transformations. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2619b6a9e24a
Convert mRefPoint to visual coordinates for synthesized events. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/5e5a5fa146eb
Update test expectations for elementTiming.html. r=mstange
https://hg.mozilla.org/integration/autoland/rev/64ef8ec1688f
Update test expectations for tests which are now passing. r=mstange
https://hg.mozilla.org/integration/autoland/rev/15a926067e78
Run helper_drag_root_scrollbar.html with zooming enabled as well. r=kats
https://hg.mozilla.org/integration/autoland/rev/446eca28becc
Disable zoom-to-focused-input behaviour in dom/html/test/forms. r=kats

https://hg.mozilla.org/mozilla-central/rev/15d1011493f4
https://hg.mozilla.org/mozilla-central/rev/740a8b776f5c
https://hg.mozilla.org/mozilla-central/rev/22b125b810a7
https://hg.mozilla.org/mozilla-central/rev/a03147de488b
https://hg.mozilla.org/mozilla-central/rev/8206cf323d5b
https://hg.mozilla.org/mozilla-central/rev/bda050135c64
https://hg.mozilla.org/mozilla-central/rev/071e6527d743
https://hg.mozilla.org/mozilla-central/rev/ff865d99f67e
https://hg.mozilla.org/mozilla-central/rev/b76b8d0b94ca
https://hg.mozilla.org/mozilla-central/rev/25d6c0b90ad2
https://hg.mozilla.org/mozilla-central/rev/7782bd4b305a
https://hg.mozilla.org/mozilla-central/rev/3718d0d54e65
https://hg.mozilla.org/mozilla-central/rev/87f45543cf69
https://hg.mozilla.org/mozilla-central/rev/62a47652d47a
https://hg.mozilla.org/mozilla-central/rev/1b0051225524
https://hg.mozilla.org/mozilla-central/rev/bc416b15fb11
https://hg.mozilla.org/mozilla-central/rev/c57fce62acf3
https://hg.mozilla.org/mozilla-central/rev/72cf169efa43
https://hg.mozilla.org/mozilla-central/rev/7878684d1f8b
https://hg.mozilla.org/mozilla-central/rev/6aa68ae33358
https://hg.mozilla.org/mozilla-central/rev/c4dcb019b6ba
https://hg.mozilla.org/mozilla-central/rev/f40298497136
https://hg.mozilla.org/mozilla-central/rev/185da341dbe6
https://hg.mozilla.org/mozilla-central/rev/2619b6a9e24a
https://hg.mozilla.org/mozilla-central/rev/5e5a5fa146eb
https://hg.mozilla.org/mozilla-central/rev/64ef8ec1688f
https://hg.mozilla.org/mozilla-central/rev/15a926067e78
https://hg.mozilla.org/mozilla-central/rev/446eca28becc

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Awesome!

Regressions: 1635822
Duplicate of this bug: 1626080
Duplicate of this bug: 1625948
Duplicate of this bug: 1244284
Duplicate of this bug: 1257579
Duplicate of this bug: 1619402
Duplicate of this bug: 1627006
Regressions: 1637113
Regressions: 1638441
Regressions: 1638655
Regressions: 1638657
Regressions: 1638662
Regressions: 1643212
Regressions: 1638594
Regressions: 1661608
No longer regressions: 1661608
You need to log in before you can comment on or make changes to this bug.