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)
Tracking
()
People
(Reporter: botond, Assigned: botond)
References
(Blocks 3 open bugs)
Details
(Whiteboard: apz-planning)
Attachments
(28 files, 7 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
STR
- Run Nightly with
apz.allow_zooming=true
. - Load planet.mozilla.org.
- 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.
Assignee | ||
Comment 1•2 years ago
|
||
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.
Assignee | ||
Comment 2•2 years ago
|
||
(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).
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
Assignee | ||
Comment 4•2 years ago
|
||
Depends on D34256
Assignee | ||
Comment 5•2 years ago
|
||
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
Assignee | ||
Comment 6•2 years ago
|
||
Depends on D34258
Assignee | ||
Comment 7•2 years ago
|
||
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.
Assignee | ||
Comment 9•2 years ago
|
||
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.
Assignee | ||
Comment 10•2 years ago
|
||
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.
Comment 11•2 years ago
|
||
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.
Comment 12•2 years ago
|
||
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.
Comment 13•2 years ago
|
||
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.
Comment 14•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 15•2 years ago
•
|
||
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:
As you can see there are still a lot of failures, especially on Android, but it's greener than previous iterations (like this one).
Assignee | ||
Comment 16•2 years ago
|
||
Assignee | ||
Comment 17•1 year ago
|
||
Assignee | ||
Comment 18•1 year ago
|
||
Assignee | ||
Comment 19•1 year ago
•
|
||
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.)
- 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,
- 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:
-
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. -
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.
-
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!
Comment 20•1 year ago
|
||
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).
Comment 21•1 year ago
|
||
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.
Assignee | ||
Comment 22•1 year ago
|
||
(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.
Comment 23•1 year ago
|
||
(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.
Assignee | ||
Comment 24•1 year ago
|
||
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
Assignee | ||
Comment 25•1 year ago
|
||
Assignee | ||
Comment 26•1 year ago
|
||
^ 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.
Assignee | ||
Comment 27•1 year ago
|
||
^ Even fewer wpt failures. I also have a fix for wpt8 locally which is not reflected in this push.
Assignee | ||
Comment 28•1 year ago
|
||
^ All wpt failures fixed now except for a debug-only assertion wpt8.
Assignee | ||
Comment 29•1 year ago
|
||
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
Assignee | ||
Comment 31•1 year ago
|
||
(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
Assignee | ||
Comment 34•1 year ago
|
||
(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.
Assignee | ||
Comment 35•1 year ago
|
||
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
ornsIDOMWindowUtils.setResolutionAndScaleTo()
to trigger zooming rather thaninitial-scale
in a meta-viewport tag.) - For the remaining test cases, split them out into a new test group which only runs on mobile.
Assignee | ||
Comment 36•1 year ago
|
||
(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.)
Assignee | ||
Comment 37•1 year ago
|
||
(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.
Assignee | ||
Comment 38•1 year ago
|
||
^ 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.
Comment 39•1 year ago
|
||
Just FYI: you can workaround bug 1608932 by running the tests with --disable-e10s.
Assignee | ||
Comment 40•1 year ago
|
||
(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.
Assignee | ||
Comment 41•1 year ago
•
|
||
(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?):
Assignee | ||
Comment 42•1 year ago
|
||
(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?):
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:
Assignee | ||
Comment 43•1 year ago
|
||
I have a fix for the failure I could repro locally. Now down to 5 failures:
Moreover, I found a way to repro one of these locally, by running the entire directory rather than just the individual test.
Assignee | ||
Comment 44•1 year ago
|
||
Down to 4 failures:
Unfortunately, I can't repro any of these locally, so I'll have to debug them over Try.
Assignee | ||
Comment 45•1 year ago
|
||
After a rebase, we seem to be down to 2 failures:
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 46•1 year ago
|
||
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:
Comment 47•1 year ago
|
||
(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.
Assignee | ||
Comment 48•1 year ago
|
||
(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:
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.
Comment 49•1 year ago
|
||
(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:
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).
Comment 50•1 year ago
|
||
(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.
Assignee | ||
Comment 51•1 year ago
|
||
(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?
Comment 52•1 year ago
|
||
(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.
Comment 54•1 year ago
|
||
(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.
Comment 55•1 year ago
|
||
(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.
Assignee | ||
Comment 56•1 year ago
|
||
(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).
Comment 57•1 year ago
|
||
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
Assignee | ||
Comment 58•1 year ago
|
||
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).
Assignee | ||
Comment 59•1 year ago
|
||
I can repro all of the above locally except for dom/html/test/test_bug500885.html.
Comment 60•1 year ago
|
||
(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?
Assignee | ||
Comment 61•1 year ago
|
||
(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).
Comment 62•1 year ago
|
||
(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.
Assignee | ||
Comment 63•1 year ago
|
||
I have a fix for layout/style/test/test_pseudoelement_state.html:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3c6c252c1505dd0ff8e5dab9197a9c7cd186ef4
Comment 64•1 year ago
|
||
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.
Comment 65•1 year ago
|
||
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
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?
Comment 66•1 year ago
|
||
(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.
Comment 68•1 year ago
|
||
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.
Comment 69•1 year ago
|
||
The problem with test_bug977003.html happens here
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.
Comment 71•1 year ago
|
||
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.
Assignee | ||
Comment 72•1 year ago
|
||
(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).
Assignee | ||
Comment 73•1 year ago
|
||
(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()
.
Assignee | ||
Comment 74•1 year ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #69)
The problem with test_bug977003.html happens here
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.
Assignee | ||
Comment 75•1 year ago
|
||
At long last, I have a green Try push! \o/
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
Assignee | ||
Comment 76•1 year ago
|
||
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.
Assignee | ||
Comment 77•1 year ago
|
||
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.
Updated•1 year ago
|
Assignee | ||
Comment 78•1 year ago
|
||
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.
Assignee | ||
Comment 79•1 year ago
|
||
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.
Assignee | ||
Comment 80•1 year ago
|
||
Depends on D68273
Assignee | ||
Comment 81•1 year ago
|
||
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
Assignee | ||
Comment 82•1 year ago
|
||
Depends on D68275
Assignee | ||
Comment 83•1 year ago
|
||
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
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 84•1 year ago
|
||
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
Assignee | ||
Comment 85•1 year ago
|
||
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
Assignee | ||
Comment 86•11 months ago
|
||
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
Assignee | ||
Comment 87•11 months ago
|
||
Depends on D68297
Assignee | ||
Comment 88•11 months ago
|
||
Depends on D68724
Assignee | ||
Comment 89•11 months ago
|
||
Depends on D68725
Assignee | ||
Comment 90•11 months ago
|
||
Depends on D68727
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Assignee | ||
Comment 92•11 months ago
|
||
Depends on D68728
Assignee | ||
Comment 93•11 months ago
|
||
Use it in document.elementFromPoint().
Depends on D68913
Assignee | ||
Comment 94•11 months ago
|
||
We were using "during event delivery" as a proxy for this, but it was an inaccurate proxy.
Depends on D68914
Assignee | ||
Comment 95•11 months ago
|
||
Depends on D68915
Assignee | ||
Comment 96•11 months ago
|
||
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
Assignee | ||
Comment 97•11 months ago
|
||
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
Assignee | ||
Comment 98•11 months ago
|
||
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
Assignee | ||
Comment 99•11 months ago
|
||
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.
- To make sure we get into TransformRootPointToFrame(), we
Depends on D68920
Assignee | ||
Comment 100•11 months ago
|
||
Depends on D68921
Assignee | ||
Comment 101•11 months ago
|
||
This is still not everything, but all the core changes are posted now. The rest is mostly odds and ends and test changes.
Assignee | ||
Comment 102•11 months ago
|
||
Assignee | ||
Comment 103•11 months ago
|
||
Depends on D69639
Assignee | ||
Comment 104•11 months ago
|
||
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
Assignee | ||
Comment 105•11 months ago
|
||
Depends on D69641
Assignee | ||
Comment 106•11 months ago
|
||
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
Assignee | ||
Comment 107•11 months ago
|
||
Ok, this is the full patch series now :) It ended up consisting of 26 patches.
Assignee | ||
Comment 108•11 months ago
|
||
Depends on D68275
Comment 109•11 months ago
|
||
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).
Assignee | ||
Comment 110•11 months ago
|
||
Depends on D68919
Updated•11 months ago
|
Updated•11 months ago
|
Assignee | ||
Comment 112•10 months ago
•
|
||
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.
Assignee | ||
Comment 113•10 months ago
•
|
||
This is looking better (with the nsEventStatus_eSentinel
patch taken out for now): https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=ce47a2409b90fb305af797f1b0a6cf6ede468191
Comment 114•10 months ago
|
||
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
Comment 115•10 months ago
|
||
Backed out for multiple mochitest failures.
backout: https://hg.mozilla.org/integration/autoland/rev/95fac5915a31c26ced5c9634bc25617343d88d80
failure logs:
- ThreadSanitizer: SEGV /builds/worker/workspace/obj-build/dist/include/ipc/IPCMessageUtils.h:123:5 in Write https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299749958&repo=autoland&lineNumber=2017
- Assertion failure: EnumValidator::IsLegalValue(aValue), at /builds/worker/workspace/obj-build/dist/include/ipc/IPCMessageUtils.h https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299749926&repo=autoland&lineNumber=2434
- dom/html/test/forms/test_input_file_picker.html | Test timed out. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299742457&repo=autoland&lineNumber=2293
- and more
Comment 116•10 months ago
|
||
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
Assignee | ||
Comment 117•10 months ago
|
||
(In reply to Natalia Csoregi [:nataliaCs] from comment #115)
- ThreadSanitizer: SEGV /builds/worker/workspace/obj-build/dist/include/ipc/IPCMessageUtils.h:123:5 in Write https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299749958&repo=autoland&lineNumber=2017
- Assertion failure: EnumValidator::IsLegalValue(aValue), at /builds/worker/workspace/obj-build/dist/include/ipc/IPCMessageUtils.h https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299749926&repo=autoland&lineNumber=2434
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.
Assignee | ||
Comment 118•10 months ago
|
||
(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 119•10 months ago
|
||
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.
Assignee | ||
Comment 120•10 months ago
|
||
(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 queriesInputAPZContext::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 noInputAPZContext
on the stack, and thus the default value ofnsEventStatus_eSentinel
is picked up.
I have a fix for this, but I spun it off into bug 1634206.
Assignee | ||
Comment 121•10 months ago
|
||
Depends on D69644
Assignee | ||
Comment 122•10 months ago
|
||
(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 toIsRootContentDocumentCrossProcess()
. - 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.
Comment 123•10 months ago
|
||
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
Comment 124•10 months ago
|
||
bugherder |
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
Comment 125•10 months ago
|
||
\o/
Comment 126•10 months ago
|
||
👏
Comment 127•10 months ago
|
||
Awesome!
Updated•10 months ago
|
Description
•