Closed Bug 1465616 Opened 7 years ago Closed 6 years ago

Have Layout attach position:fixed elements to the layout ("fixed") viewport, and have APZ keep them attached

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
64 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox62 --- unaffected
firefox63 + verified
firefox64 --- affected

People

(Reporter: botond, Assigned: u608768)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(8 files, 7 obsolete files)

Bug 1465616 - Add a mochitest for fixed position hit-testing.
(deleted), text/x-review-board-request
botond
: review+
Details
Bug 1465616 - Use the larger viewport to layout and size fixed position elements.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1465616 - Add reftests for main-thread scroll behaviour.
(deleted), text/x-review-board-request
botond
: review+
Details
Bug 1465616 - Temporarily apply async test attributes when compositing.
(deleted), text/x-review-board-request
botond
: review+
Details
Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements.
(deleted), text/x-review-board-request
botond
: review+
Details
Bug 1465616 - Add async-scroll reftests.
(deleted), text/x-review-board-request
botond
: review+
Details
Bug 1465616 - Add failing fixed-position async-zoom reftests.
(deleted), text/x-review-board-request
botond
: review+
Details
video.mp4
(deleted), video/mp4
Details
This is the second piece of the work for bug 656036. It involves modifying Layout code to attach position:fixed elements to the layout ("fixed") viewport, rather than the visual vieport.
Blocks: 1465618
Priority: -- → P3
Comment on attachment 8989863 [details] Bug 1465616 - Don't use SPC-SPS to layout fixed position elements. https://reviewboard.mozilla.org/r/254844/#review261710 Looks good to me, but Markus is the appropriate reviewer for this code so I'll let him give the r+.
Attachment #8989863 - Flags: review?(botond)
Comment on attachment 8989865 [details] Bug 1465616 - Use layout viewport size to compute visible rect for fixed position elements. https://reviewboard.mozilla.org/r/254848/#review261712 Likewise. ::: commit-message-210d1:1 (Diff revision 1) > +Bug 1465616 - Use viewport size to compute visible rect for fixed position elements. r?botond,r?mstange viewport -> layout viewport Since we have two kinds of viewport now, might as well be specific.
Attachment #8989865 - Flags: review?(botond)
A lot of the reftest failures look like we're clipping away content that should be painted, which suggests an issue with the change to ComputeVisibleRectForFrame(). Perhaps the fallback value for the visible rect on Android should be soemthing different in the case where the SPC-SPS is set (even if it's not the SPC-SPS itself)?
Comment on attachment 8989866 [details] Bug 1465616 - Add reftests for fixed and sticky elements. https://reviewboard.mozilla.org/r/254850/#review261732 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-fixed-ref.html:13 (Diff revision 1) > + height: 2000px; > + overflow: hidden; > + } > + div { > + position: fixed; > + bottom: -300px; Can you describe the calculation that leads to this value? In particular, are you assuming that the layout viewport is 1000 CSS pixels in height? If so, I'd prefer to avoid making that assumption. ::: gfx/layers/apz/test/reftest/pinch-zoom-position-fixed.html:2 (Diff revision 1) > +<!DOCTYPE html> > +<html> Shouldn't this have class="reftest-wait"? ::: gfx/layers/apz/test/reftest/reftest.list:27 (Diff revision 1) > skip-if(!Android) pref(apz.allow_zooming,true) == initial-scale-1.html initial-scale-1-ref.html > > skip-if(!asyncPan) == frame-reconstruction-scroll-clamping.html frame-reconstruction-scroll-clamping-ref.html > + > +# Test that position:fixed and position:sticky elements are positioned according to the layout viewport. > +skip-if(!Android) pref(apz.allow_zooming,true) pref(dom.meta-viewport.enabled,true) == pinch-zoom-position-fixed.html pinch-zoom-position-fixed-ref.html I don't think setting dom.meta-viewport.enabled is necessary for an Android-only test, since the preference is already enabled on Android. (The only reason setting apz.allow_zooming is necessary is that we disable it by default [1] when running Android tests.) [1] https://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/tools/reftest/remotereftest.py#238
Attachment #8989866 - Flags: review?(botond)
Comment on attachment 8989867 [details] Bug 1465616 - Add a mochitest for fixed position hit-testing. https://reviewboard.mozilla.org/r/254852/#review261734 ::: gfx/layers/apz/test/mochitest/helper_position_fixed_scroll_and_click.html:48 (Diff revision 1) > + function* test(testDriver) { > + yield synthesizeNativeTouchDrag(document.querySelector("body"), 10, 10, -1000, 0, testDriver); > + yield waitForApzFlushedRepaints(testDriver); > + > + const point = centerOf(document.querySelector("input")); > + SpecialPowers.getDOMWindowUtils(window).sendMouseEvent("MozMouseHittest", point.x, point.y, 0, 0, 0); Does a MozMouseHittest actually trigger a click? The way we usually use MozMouseHittest in APZ tests [1] is to send it, and then query what the hit-test result of the last hit test event was (which APZ exposes via a test-only API). The hit-test result contains the scroll-id of the nearest enclosing scrollframe, so to check cases like this, we'd put a scrollable element in the place of the input (see existing tests in test_group_hittest as an example). Alternatively, you can use onclick as you're doing now, but then (I believe) you need to send actual mouse-down and mouse-up events. [1] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/layers/apz/test/mochitest/apz_test_utils.js#559 ::: gfx/layers/apz/test/mochitest/mochitest.ini:33 (Diff revision 1) > skip-if = (verify && debug && (os == 'win')) > [test_group_wheelevents.html] > skip-if = (toolkit == 'android') # wheel events not supported on mobile > [test_group_zoom.html] > skip-if = (toolkit != 'android') # only android supports zoom > +[test_position_fixed_scroll_and_click.html] Any reason not to use test_group_zoom?
Attachment #8989867 - Flags: review?(botond)
Comment on attachment 8989864 [details] Bug 1465616 - Remove call to AlignFixedAndStickyLayers for RCD-RSF. https://reviewboard.mozilla.org/r/254846/#review261736 ::: gfx/layers/composite/AsyncCompositionManager.cpp:1071 (Diff revision 1) > LayerToParentLayerMatrix4x4 transformWithoutOverscrollOrOmta = > layer->GetTransformTyped() > * CompleteAsyncTransform( > AdjustForClip(asyncTransformWithoutOverscroll, layer)); > > + if (!metrics.IsRootContent() || !gfxPrefs::APZAllowZooming()) { I would move the explanation from the commit message to a comment here.
Attachment #8989864 - Flags: review?(botond)
Attachment #8989863 - Attachment is obsolete: true
Attachment #8989863 - Flags: review?(mstange)
Attachment #8989864 - Attachment is obsolete: true
Attachment #8989864 - Flags: review?(botond)
Attachment #8989865 - Attachment is obsolete: true
Attachment #8989865 - Flags: review?(mstange)
Attachment #8989866 - Attachment is obsolete: true
Attachment #8989866 - Flags: review?(botond)
Comment on attachment 8989867 [details] Bug 1465616 - Add a mochitest for fixed position hit-testing. https://reviewboard.mozilla.org/r/254852/#review263252 r+ with comment addressed. (That means "please address the comment, but I don't need to look at the updated patch again". MozReview will carry forward an r+ automatically to a newer version of a patch.) ::: gfx/layers/apz/test/mochitest/helper_fixed_position_scroll_hittest.html:36 (Diff revision 3) > + function* test(testDriver) { > + document.addEventListener("click", (e) => { > + is(e.target, document.querySelector("input"), "got click"); > + subtestDone(); > + }); > + yield synthesizeNativeTouchDrag(document.querySelector("body"), 10, 10, -2000, 0, testDriver); // Scroll all the way to the right to bring the button into view
Attachment #8989867 - Flags: review?(botond) → review+
Comment on attachment 8991460 [details] Bug 1465616 - Remove call to AlignFixedAndStickyLayers for RCD-RSF. https://reviewboard.mozilla.org/r/256350/#review263254
Attachment #8991460 - Flags: review?(botond) → review+
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263450 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky.html:20 (Diff revision 1) > + height: 500px; > + background: linear-gradient(135deg, white, black); > + } > + </style> > +</head> > +<body onload="scrollTo(0, 500); document.documentElement.classList.remove('reftest-wait');"> Is this scrollTo() and reftest-wait actually necessary here? Since scrollTo() will scroll both the layout viewport and the visual viewport, I expect the rendering to be identical before and after the scrollTo().
(In reply to Botond Ballo [:botond] from comment #24) > Comment on attachment 8991462 [details] > Bug 1465616 - Add reftests for fixed and sticky elements. > > https://reviewboard.mozilla.org/r/256354/#review263450 > > ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky.html:20 > (Diff revision 1) > > + height: 500px; > > + background: linear-gradient(135deg, white, black); > > + } > > + </style> > > +</head> > > +<body onload="scrollTo(0, 500); document.documentElement.classList.remove('reftest-wait');"> > > Is this scrollTo() and reftest-wait actually necessary here? > > Since scrollTo() will scroll both the layout viewport and the visual > viewport, I expect the rendering to be identical before and after the > scrollTo(). Sorry, I meant this comment to apply to the position:fixed testcase, not the position:sticky testcase.
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263764 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-fixed.html:12 (Diff revision 1) > + margin: 0; > + height: 2000px; > + overflow: hidden; > + } > + div { > + position: fixed; It took me a while to understand why this testcase works, so I think some explanation (in a comment) would be helpful. Here's how I understand it: * The reference page has a position:absolute element in place a position:fixed element, both position using "bottom" * position:absolute elements are attached to the initial containing block, which coincides with the layout viewport on page load * After zooming in, the top edge of the visual viewport will continue to coincide with the top edge of the layout viewport, but the bottom edges of the two will diverge * Therefore, if we are attaching fixed elements to the visual viewport, the rendering will be different than for the absolute element, but if we're attaching fixed element to the layout viewport, the rendering will be the same
Attachment #8991462 - Flags: review?(botond)
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263772 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky-ref.html:13 (Diff revision 1) > + height: 2000px; > + overflow: hidden; > + } > + div { > + position: absolute; > + top: calc(100vh - 500px); As discussed in person, this doesn't actually test what we want it to, because the top edges of the visual and layout viewports coincide after zooming in, so attaching something to the top doesn't discriminate between attachment to the visual viewport vs. the layout viewport. ::: gfx/layers/apz/test/reftest/reftest.list:27 (Diff revision 1) > skip-if(!Android) pref(apz.allow_zooming,true) == initial-scale-1.html initial-scale-1-ref.html > > skip-if(!asyncPan) == frame-reconstruction-scroll-clamping.html frame-reconstruction-scroll-clamping-ref.html > + > +# Test that position:fixed and position:sticky elements are positioned according to the layout viewport. > +skip-if(!Android) pref(apz.allow_zooming,true) == pinch-zoom-position-fixed.html pinch-zoom-position-fixed-ref.html It's useful to document why we are skipping a test on a particular platform, to make it easier to enable it on other platforms in the future. In this case, the reason is that the relevant functionality is only implemented for container scrolling, which is only enabled by default on Android, and it's a "Once" pref, and we can't enable "Once" prefs for a single reftest (since, I believe, the harness runs the entire directory (or possibly the entire chunk, I'm not sure) in a single browser invocation).
Attachment #8991462 - Flags: review?(botond)
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263778 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky-ref.html:11 (Diff revisions 1 - 2) > body { > margin: 0; > height: 2000px; > overflow: hidden; > } > - div { > + div:nth-child(1) { I find ids (or classes) with descriptive names to be more readable (and maintainable in the presence of changes to the page structure). ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky.html:23 (Diff revisions 1 - 2) > height: 500px; > - background: linear-gradient(135deg, white, black); > + background: repeating-linear-gradient(90deg, transparent, transparent 20px, black 20px, black 40px); > } > </style> > </head> > <body onload="scrollTo(0, 500); document.documentElement.classList.remove('reftest-wait');"> A comment would be nice here, too: "This is similar to the fixed testcase, but we add a tall element before the sticky element to ensure that the sticky element is in it's 'fixed' configuration on page load."
(In reply to Botond Ballo [:botond] from comment #25) > > Is this scrollTo() and reftest-wait actually necessary here? > > > > Since scrollTo() will scroll both the layout viewport and the visual > > viewport, I expect the rendering to be identical before and after the > > scrollTo(). > > Sorry, I meant this comment to apply to the position:fixed testcase, not the > position:sticky testcase. After thinking it over a bit more, I think the scrollTo() is unnecessary in both testcases. Let me know if you agree.
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263782 ::: gfx/layers/apz/test/reftest/pinch-zoom-position-sticky.html:26 (Diff revisions 2 - 3) > </style> > </head> > <body onload="scrollTo(0, 500); document.documentElement.classList.remove('reftest-wait');"> > - <div></div> > - <div></div> > + <!-- This is similar to the position-fixed testcase, but we add a tall > + element before the sticky element to ensure that the sticky element is > + in it's "fixed" configuration on page load. --> it's -> its (I know, this was my typo. I just noticed it.)
Attachment #8991462 - Flags: review?(botond)
Comment on attachment 8991462 [details] Bug 1465616 - Add reftests for main-thread scroll behaviour. https://reviewboard.mozilla.org/r/256354/#review263784 Thanks!
Attachment #8991462 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #31) > After thinking it over a bit more, I think the scrollTo() is unnecessary in > both testcases. (We chatted about this and agreed that while the scrollTo() is not necessary to test that the fixed/sticky element is positioned relative to the layout viewport (and not the visual viewport) on page load, it does test something additional that's useful: that the element remains attached to the layout viewport as you scroll.)
Comment on attachment 8991459 [details] Bug 1465616 - Use the larger viewport to layout and size fixed position elements. https://reviewboard.mozilla.org/r/256348/#review264494
Attachment #8991459 - Flags: review?(mstange) → review+
Comment on attachment 8991461 [details] Bug 1465616 - Use layout viewport size to compute visible rect for fixed position elements. https://reviewboard.mozilla.org/r/256352/#review264496
Attachment #8991461 - Flags: review?(mstange) → review+
Assignee: nobody → kmadan
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e48a27950be Don't use SPC-SPS to layout fixed position elements. r=mstange https://hg.mozilla.org/integration/autoland/rev/3f5ca59998bf Remove call to AlignFixedAndStickyLayers for RCD-RSF. r=botond https://hg.mozilla.org/integration/autoland/rev/559c8997eb53 Use layout viewport size to compute visible rect for fixed position elements. r=mstange https://hg.mozilla.org/integration/autoland/rev/ea79ddd406f4 Add reftests for fixed and sticky elements. r=botond https://hg.mozilla.org/integration/autoland/rev/403ba3d9539a Add a mochitest for fixed position hit-testing. r=botond
This patch is known to regress the behaviour of fixed elements during async scrolling (after a repaint things should be fine). The regression should be short-lived and will be fixed by bug 1465618 soon.
Depends on: 1477093
Depends on: 1476942
This has caused significant regressions on Fennec Nightly with scrolling. While it sounds like those issues were anticipated, this shouldn't have landed without bug 1465618 more ready to land. Leaving our users with buggy scrolling for many days on end is not acceptable. Backed out and Fennec nightlies respun. https://hg.mozilla.org/mozilla-central/rev/cd8671232fd851520c6e7a5619b65b420984cfa7
Status: RESOLVED → REOPENED
status-firefox63: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
Attachment #8991461 - Flags: review+ → review?(mstange)
Depends on: 1473699
Blocks: 1477403
Attachment #8991461 - Flags: review?(mstange)
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review265546 Thanks! I like the simplification that this approach brings. ::: gfx/layers/apz/src/AsyncPanZoomController.h:202 (Diff revision 1) > + * An RAII class to apply mTestAsyncScrollOffset and mTestAsyncZoom to the > + * FrameMetrics of the given AsyncPanZoomController. > + * > + * This class does nothing when neither value is set. > + */ > + class AutoAsyncPanZoomController { * We can give this class a more specific name, like AutoApplyTestScrollOffset. * The class can be just forward-declared in the .h file, and defined in the .cpp file (compare e.g. DisplayListClipState::AutoSaveRestore). ::: gfx/layers/apz/src/AsyncPanZoomController.h:204 (Diff revision 1) > + * > + * This class does nothing when neither value is set. > + */ > + class AutoAsyncPanZoomController { > + public: > + explicit AutoAsyncPanZoomController(AsyncPanZoomController *aApzc) nit: * goes with the type ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3815 (Diff revision 1) > > if (aMode == eForCompositing && mScrollMetadata.IsApzForceDisabled()) { > return AsyncTransform(); > } > > + AsyncPanZoomController::AutoAsyncPanZoomController autoApzc((AsyncPanZoomController *)this); Please move the cast to the constructor implementation, and use const_cast.
Attachment #8993805 - Flags: review?(botond)
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review265548 ::: gfx/layers/apz/src/AsyncPanZoomController.h:1087 (Diff revision 1) > * overscrolled state, if any. > */ > AsyncTransformComponentMatrix GetOverscrollTransform(AsyncTransformConsumer aMode) const; > > /** > * Returns the incremental transformation corresponding to the async pan/zoom I would say " ... coresponding to async panning / zooming of the layout viewport (unlike GetCurrentAsyncTransform which deals with async movement of the visual viewport)." ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3827 (Diff revision 1) > + CSSRect currentViewport = GetEffectiveLayoutViewport(aMode); > + CSSPoint currentViewportOffset = currentViewport.TopLeft(); > + > + // If checkerboarding has been disallowed, clamp the scroll position to stay > + // within rendered content. > + if (!gfxPrefs::APZAllowCheckerboarding() && This calculation seems to be making sure that we don't scroll checkerboarded content into the layout viewport. That's a different restriction on the amount of the async scroll than the one in GetCurrentAsyncTransform(), which ensures we don't scroll checkerboarded content into the visual viewport. As a result, if apz.allow_checkerboarding is enabled, this function and GetCurrentAsyncTransform() will report inconsistent values, and fixed layers will not be positioned correctly. A correct calculation would involve something like: * Calculating a clamped visual scroll offset, like in GetCurrentAsyncTransform(). * Running RecalculateViewportOffset() to compute the layout viewport offset corresponding to that visual offset. * Using that as the clamped layout viewport offset. However, since apz.allow_checkerboarding is something we haven't used in a very long time, I wouldn't bother doing anything at all here (except maybe adding a TODO comment). ::: gfx/layers/composite/AsyncCompositionManager.cpp:1066 (Diff revision 1) > // 'aCurrentTransformForRoot' parameter. This ensures that the overscroll > // and OMTA transforms are not unapplied, and therefore that the visual > // effects apply to fixed and sticky layers. We do this by using > // GetTransform() as the base transform rather than GetLocalTransform(), > // which would include those factors. > - LayerToParentLayerMatrix4x4 transformWithoutOverscrollOrOmta = > + LayerToParentLayerMatrix4x4 transformWithoutOverscrollOrOmta; Suggestion for reducing duplication a bit: AsyncTransform asyncTransformForFixedAdjustment = (metrics.isRootContent() && gfxPrefs::APZAllowZooming()) ? sampler->GetCurrentAsyncViewportTransform(wrapper) : asyncTransformWithoutOverscroll; LayerToParentLayerMatrix4x4 transformWithoutOverscrollOrOmta = layer->GetTransformTyped() \* CompleteAsyncTransform(AdjustForClip(asyncTransformForFixedAdjustment, layer));
Attachment #8993806 - Flags: review?(botond)
Comment on attachment 8993807 [details] Bug 1465616 - Add async-scroll reftests. https://reviewboard.mozilla.org/r/258492/#review265550 ::: layout/reftests/async-scrolling/position-fixed-async-zoom-1.html:3 (Diff revision 1) > +<!DOCTYPE html> > +<html reftest-async-scroll > + reftest-async-scroll-x="0" reftest-async-scroll-y="50" We usually set reftest-displayport-* attributes on elements that we async scroll. I guess in this case we get away without it because the RCD-RSF is layerized even without a displayport being explicitly set, and any checkerboarding we may see is not relevant to the outcome? It might still be a good idea to specify those attributes to be on the safe side. ::: layout/reftests/async-scrolling/position-fixed-async-zoom-1.html:4 (Diff revision 1) > +<!DOCTYPE html> > +<html reftest-async-scroll > + reftest-async-scroll-x="0" reftest-async-scroll-y="50" > + reftest-async-zoom="2.0"> I'm surprised that the tests work with async zooming (I was imagining that the test pages in this bug would use initial-scale to zoom, and only the scroll would be async). How do we avoid the positioning issues with async zooming that we said we would defer to a follow-up bug? ::: layout/reftests/async-scrolling/position-fixed-async-zoom-2.html:25 (Diff revision 1) > + </style> > +</head> > +<body onload="scrollTo(0, 1000); document.documentElement.classList.remove('reftest-wait');"> > + <!-- Test that position:fixed elements scroll with the layout viewport. > + > + Scroll the window (i.e., the layout viewport) to (0, 500). An async I assume you mean "to (0, 1000)".
Attachment #8993807 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #62) > As a result, if apz.allow_checkerboarding is enabled, I meant, of course, "if apz.allow_checkerboarding is disabled" ... > However, since apz.allow_checkerboarding is something we haven't used in a > very long time .. and here, "apz.allow_checkerboarding is something we haven't set to false in a very long time".
Comment on attachment 8993807 [details] Bug 1465616 - Add async-scroll reftests. https://reviewboard.mozilla.org/r/258492/#review265550 > I'm surprised that the tests work with async zooming (I was imagining that the test pages in this bug would use initial-scale to zoom, and only the scroll would be async). > > How do we avoid the positioning issues with async zooming that we said we would defer to a follow-up bug? I think I'd asked you about this last week - the follow-up bug is related to the position of the anchor point (i.e., the point at which we want to center the screen after (and during?) zooming). I'm not sure what anchor point reftest-async-zoom uses - I can update these tests to use initial-scale instead, but the results appear to be the same.
Summary: Have Layout attach position:fixed elements to the layout ("fixed") viewport → Have Layout attach position:fixed elements to the layout ("fixed") viewport, and have APZ keep them attached
(Updated bug description to reflect the fact that bug 1465618 has been folded into this one.)
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review265764 ::: gfx/layers/apz/src/AsyncPanZoomController.h:202 (Diff revisions 1 - 2) > * An RAII class to apply mTestAsyncScrollOffset and mTestAsyncZoom to the > * FrameMetrics of the given AsyncPanZoomController. > * > * This class does nothing when neither value is set. > */ > - class AutoAsyncPanZoomController { > + class AutoApplyTestAttributes { I actually meant forward-declaring the class itself, not just its methods: // AsyncPanZoomController.h class AsyncPanZoomController { ... class AutoApplyTestAttributes; ... }; // AsyncPanZoomController.cpp class AsyncPanZoomController::AutoApplyTestAttributes { ... };
Attachment #8993805 - Flags: review?(botond)
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review265766 ::: gfx/layers/composite/AsyncCompositionManager.cpp:1068 (Diff revisions 1 - 2) > // effects apply to fixed and sticky layers. We do this by using > // GetTransform() as the base transform rather than GetLocalTransform(), > // which would include those factors. > - LayerToParentLayerMatrix4x4 transformWithoutOverscrollOrOmta; > - if (metrics.IsRootContent() && gfxPrefs::APZAllowZooming()) { > - // In order to attach root-level fixed/sticky layers to the > + AsyncTransform asyncTransformForFixedAdjustment > + = metrics.IsRootContent() && gfxPrefs::APZAllowZooming() > + ? sampler->GetCurrentAsyncViewportTransform(wrapper) The comment "In order to attach root-level fixed/sticky layers to the layout viewport, ..." was helpful, please keep it.
Attachment #8993806 - Flags: review?(botond)
Attachment #8991461 - Flags: review?(mstange)
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review265802
Attachment #8993806 - Flags: review?(botond) → review+
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review265804
Attachment #8993805 - Flags: review?(botond) → review+
Comment on attachment 8991461 [details] Bug 1465616 - Use layout viewport size to compute visible rect for fixed position elements. https://reviewboard.mozilla.org/r/256352/#review265846 ::: layout/painting/nsDisplayList.h:1469 (Diff revision 4) > - // viewport, or the scroll position clamping scrollport size, if one is > - // set. > + // scrolled, paint them into the frame's parent (i.e., the layout > + // viewport). I'd say "paint them at their parent frame's size, i.e. the layout viewport size."
Attachment #8991461 - Flags: review?(mstange) → review+
Attachment #8991460 - Attachment is obsolete: true
Attachment #8991461 - Attachment is obsolete: true
Attachment #8991459 - Flags: review+ → review?(mstange)
Attachment #8989867 - Flags: review+ → review?(botond)
Attachment #8993806 - Flags: review+ → review?(botond)
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review267064
Attachment #8993806 - Flags: review?(botond) → review+
Comment on attachment 8989867 [details] Bug 1465616 - Add a mochitest for fixed position hit-testing. https://reviewboard.mozilla.org/r/254852/#review267070 ::: gfx/layers/apz/test/mochitest/helper_fixed_position_scroll_hittest.html:31 (Diff revisions 3 - 13) > </style> > </head> > <body> > <div id="fixed"><input type="button" value="Button" /></div> > <script> > + const INPUT = document.querySelector("input"); Please use lowercase (all-caps is usually for "compile-time" constants) ::: gfx/layers/apz/test/mochitest/helper_fixed_position_scroll_hittest.html:34 (Diff revisions 3 - 13) > <div id="fixed"><input type="button" value="Button" /></div> > <script> > + const INPUT = document.querySelector("input"); > function* test(testDriver) { > - document.addEventListener("click", (e) => { > - is(e.target, document.querySelector("input"), "got click"); > + SpecialPowers.Services.obs.addObserver(testDriver, "APZ:TransformEnd"); > + yield synthesizeNativeTouchDrag(document.body, 10, 10, -2000, 0, testDriver); If the intention is to wait for the APZ:TransformEnd message to arrive before proceeding, we don't want to pass |testDriver| in as a callback here, since that could get called sooner. ::: gfx/layers/apz/test/mochitest/helper_fixed_position_scroll_hittest.html:39 (Diff revisions 3 - 13) > - is(e.target, document.querySelector("input"), "got click"); > + yield synthesizeNativeTouchDrag(document.body, 10, 10, -2000, 0, testDriver); > + SpecialPowers.Services.obs.removeObserver(testDriver, "APZ:TransformEnd", false); > + > + yield flushApzRepaints(testDriver); > + > + SpecialPowers.Services.obs.addObserver(testDriver, "mouseevent"); I believe adding this observer is superfluous, since synthesizeNativeClick will call the observer passed in as argument directly, rather than broadcasting the "mouseevent" topic to all registered observers.
Attachment #8989867 - Flags: review?(botond)
Comment on attachment 8991459 [details] Bug 1465616 - Use the larger viewport to layout and size fixed position elements. https://reviewboard.mozilla.org/r/256348/#review267074
Attachment #8991459 - Flags: review?(mstange) → review+
Comment on attachment 8989867 [details] Bug 1465616 - Add a mochitest for fixed position hit-testing. https://reviewboard.mozilla.org/r/254852/#review267616
Attachment #8989867 - Flags: review?(botond) → review+
Attachment #8993806 - Flags: review+ → review?(botond)
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review268312 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3982 (Diff revision 8) > SampleCompositedAsyncTransform(); > return true; > } > > bool > AsyncPanZoomController::UnapplyAsyncTestAttributes(FrameMetrics aPrevFrameMetrics) { Warning: The parameter 'aPrevFrameMetrics' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param] AsyncPanZoomController::UnapplyAsyncTestAttributes(FrameMetrics aPrevFrameMetrics) { ~~~~~~~~~~~~ ^ const &
Attachment #8993806 - Flags: review?(botond)
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review268314 ::: gfx/layers/apz/src/APZUtils.h:109 (Diff revision 7) > + * An RAII class to temporarily apply async test attributes to the provided > + * AsyncPanZoomController. > + */ > +class AutoApplyAsyncTestAttributes { > +public: > + explicit AutoApplyAsyncTestAttributes(const AsyncPanZoomController*); Now that we are using this in AsyncCompositionManager rather than inside GetCurrentAsyncTransform(), this parameter doesn't need to be const (and we can get rid of the const_cast hack in the constructor implementation). ::: gfx/layers/composite/AsyncCompositionManager.cpp:981 (Diff revision 7) > LayerMetricsWrapper wrapper(layer, i); > if (!wrapper.GetApzc()) { > continue; > } > + > + auto testAttrs = sampler->ApplyAsyncTestAttributes(wrapper); // Apply any additional async scrolling for testing purposes // (used for reftest-async-scroll and reftest-async-zoom).
Comment on attachment 8993806 [details] Bug 1465616 - Use layout viewport transformations to async-adjust fixed position elements. https://reviewboard.mozilla.org/r/258490/#review268316 ::: gfx/layers/apz/src/AsyncPanZoomController.h:1069 (Diff revision 8) > */ > AsyncTransform GetCurrentAsyncTransform(AsyncTransformConsumer aMode) const; > > /** > + * Returns the incremental transformation corresponding to the async > + * panning/zooming of the larger viewport. the larger viewport -> the larger of the visual or layout viewport
Attachment #8993806 - Flags: review?(botond) → review+
Comment on attachment 8997527 [details] Bug 1465616 - Add failing fixed-position async-zoom reftests. https://reviewboard.mozilla.org/r/261252/#review268318 Nice!
Attachment #8997527 - Flags: review?(botond) → review+
Comment on attachment 8993807 [details] Bug 1465616 - Add async-scroll reftests. https://reviewboard.mozilla.org/r/258492/#review268320
Attachment #8993807 - Flags: review?(botond) → review+
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review268324 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3909 (Diff revision 7) > + SampleCompositedAsyncTransform(); > + return true; > +} > + > +bool > +AsyncPanZoomController::UnapplyAsyncTestAttributes(FrameMetrics aPrevFrameMetrics) { Warning: The parameter 'aPrevFrameMetrics' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param] AsyncPanZoomController::UnapplyAsyncTestAttributes(FrameMetrics aPrevFrameMetrics) { ~~~~~~~~~~~~ ^ const &
Given how much trouble we're having with getting the "Temporarily apply async test attributes when compositing" patch right, I'd like Kats to review it as well to have a second pair of eyes on it (once it's passing tests).
The following are a few questions I had regarding attachment 8997527 [details]: Add failing fixed-position async-zoom reftests which are mentioned in this comment since I am not authorized to review patches with my current Bugzilla privileges: Are we planning to land this patch with the behaviour that the compositor positions the position:fixed elements incorrectly and the main thread corrects the positioning? If so, is there a bug to correct the compositor side behaviour? Also, does an "invalid" offset transform mean that the compositor applies the wrong transformation to the offset or is the transformation applied undefined? Is this related to the incorrect positioning of position:fixed elements when visual viewport size > layout viewport size? Otherwise, everything looks good. Great work! review+
Thanks for the review Tanushree! (In reply to Tanushree Podder [:tanushree] from comment #135) > Are we planning to land this patch with the behaviour that the compositor > positions the position:fixed elements incorrectly and the main thread > corrects the positioning? Yes, but note that the incorrect positioning on the compositor side now only happens during async zooming. When this bug first landed (comment 45), the compositor positioning was incorrect during async scrolling too, and we were going to fix that separately in bug 1465618. However, the changes got backed out (comment 46), in part because the async scrolling behaviour was too severe of a regression. We then decided to fold bug 1465618 into this one, and the updated patch series here includes the fix for compositor-side positioning during async scrolling (in the patch "Use layout viewport transformations to async-adjust fixed position elements"). However, compositor positioning during async zooming still needs to be fixed and we are planning to do that in a separate bug. > If so, is there a bug to correct the compositor side behaviour? I don't think so. Kashav, please go ahead and file one. > Also, does an "invalid" offset transform mean that the compositor applies > the wrong transformation to the offset or is the transformation applied > undefined? A transformation is still applied, but its value is incorrect. I don't think we've established what exactly goes wrong (although Markus was telling me on Friday that he believes it's related to the anchor point for fixed-position elements). > Is this related to the incorrect positioning of position:fixed elements when > visual viewport size > layout viewport size? I don't believe so; I believe the incorrect positioning happens even when async-zooming between two zoom levels such that the layout viewport is larger than the visual viewport at both levels.
(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #136) Thanks for the clarifications, Botond! > > Is this related to the incorrect positioning of position:fixed elements when > > visual viewport size > layout viewport size? > > I don't believe so; I believe the incorrect positioning happens even when > async-zooming between two zoom levels such that the layout viewport is > larger than the visual viewport at both levels. I see. So, the positioning issue occurs in all situations when asyn-zooming and not just situations where visual viewport > layout viewport. That's why we are only addressing the sizing issue of position:fixed elements when visual viewport > layout layout in this bug and not the positioning issue.
Attachment #8993805 - Flags: review+ → review?(botond)
Attachment #8989867 - Flags: review+ → review?(botond)
Comment on attachment 8989867 [details] Bug 1465616 - Add a mochitest for fixed position hit-testing. https://reviewboard.mozilla.org/r/254852/#review269284
Attachment #8989867 - Flags: review?(botond) → review+
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review269288 Thanks! As mentioned in comment 134, I would like Kats to review this patch as well. However, if he is on parental leave, we can go ahead and land, and needinfo him for a post-landing review after he comes back. If you have a green Try push, please show it to Markus and ask him to autoland the patch series. ::: gfx/layers/apz/src/APZSampler.cpp:147 (Diff revisions 8 - 10) > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > AssertOnSamplerThread(); > > + // Apply any additional async scrolling for testing purposes (used for > + // reftest-async-scroll and reftest-async-zoom). > + auto _ = ApplyAsyncTestAttributes(aContent); If we move this inside AsyncPanZoomController::ComputeTransformForScrollThumb(), it would handle the other two call sites of that function (in SampleForWebRender() and ComputeTransformForNode()) as well, and we wouldn't need separate handling for them
Attachment #8993805 - Flags: review?(botond) → review+
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review269300 LGTM but it would be good to add some comments on the function in APZSampler.h that describe which types of call sites should use the function. ::: gfx/layers/apz/src/APZUtils.h:107 (Diff revision 10) > > +/** > + * An RAII class to temporarily apply async test attributes to the provided > + * AsyncPanZoomController. > + */ > +class AutoApplyAsyncTestAttributes { nit: add a MOZ_RAII decoration here
Attachment #8993805 - Flags: review+
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review269288 > If we move this inside AsyncPanZoomController::ComputeTransformForScrollThumb(), it would handle the other two call sites of that function (in SampleForWebRender() and ComputeTransformForNode()) as well, and we wouldn't need separate handling for them I'm assuming you meant APZCTreeManager::ComputeTransformForScrollThumb here (instead of AsyncPanZoomController::ComputeTransformForScrollThumb). I tried that originally, but it was causing WR reftest failures. The reason is that there's a call to AsyncPanZoomController::GetCurrentAsyncTransform in APZCTreeManager::SampleForWebRender **before** the call to APZCTreeManager::ComputeTransformForScrollThumb, so AsyncPanZoomController::GetCurrentAsyncTransform was returning an _incorrect_ value (since reftest async attributes weren't being applied until the call to APZCTreeManager::ComputeTransformForScrollThumb).
(In reply to Kashav Madan [:kashav] from comment #148) > > If we move this inside AsyncPanZoomController::ComputeTransformForScrollThumb(), it would handle the other two call sites of that function (in SampleForWebRender() and ComputeTransformForNode()) as well, and we wouldn't need separate handling for them > > I'm assuming you meant APZCTreeManager::ComputeTransformForScrollThumb here > (instead of AsyncPanZoomController::ComputeTransformForScrollThumb). Yep, sorry. > I tried that originally, but it was causing WR reftest failures. The reason > is that there's a call to AsyncPanZoomController::GetCurrentAsyncTransform > in APZCTreeManager::SampleForWebRender **before** the call to > APZCTreeManager::ComputeTransformForScrollThumb, so > AsyncPanZoomController::GetCurrentAsyncTransform was returning an > _incorrect_ value (since reftest async attributes weren't being applied > until the call to APZCTreeManager::ComputeTransformForScrollThumb). There are two uses of AutoApplyAsyncTestAttributes in SampleForWebRender(). The refactoring I had in mind, would only have eliminated the second one. Anyways, the way it is now is fine as well. Thanks for continuing to look at this!
Comment on attachment 8993805 [details] Bug 1465616 - Temporarily apply async test attributes when compositing. https://reviewboard.mozilla.org/r/258488/#review269526 Code analysis found 2 defects in this patch: - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/layers/apz/src/APZCTreeManager.cpp:3057 (Diff revision 11) > { > mTreeLock.AssertCurrentThreadIn(); > if (AsyncPanZoomController* apzc = aNode->GetApzc()) { > + // Apply any additional async scrolling for testing purposes (used for > + // reftest-async-scroll and reftest-async-zoom). We repeat this below. > + auto _ = AutoApplyAsyncTestAttributes(apzc); Error: Variable of type 'mozilla::layers::AutoApplyAsyncTestAttributes' is not valid in a temporary [clang-tidy: mozilla-scope] auto _ = AutoApplyAsyncTestAttributes(apzc); ^ ::: gfx/layers/apz/src/APZCTreeManager.cpp:3071 (Diff revision 11) > // AsyncCompositionManager. > ScrollableLayerGuid guid{aNode->GetLayersId(), 0, aNode->GetScrollTargetId()}; > if (RefPtr<HitTestingTreeNode> scrollTargetNode = GetTargetNode(guid, &GuidComparatorIgnoringPresShell)) { > AsyncPanZoomController* scrollTargetApzc = scrollTargetNode->GetApzc(); > MOZ_ASSERT(scrollTargetApzc); > + auto _ = AutoApplyAsyncTestAttributes(scrollTargetApzc); Error: Variable of type 'mozilla::layers::AutoApplyAsyncTestAttributes' is not valid in a temporary [clang-tidy: mozilla-scope] auto _ = AutoApplyAsyncTestAttributes(scrollTargetApzc); ^
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b14350eb2d92 Use the larger viewport to layout and size fixed position elements. r=mstange https://hg.mozilla.org/integration/autoland/rev/fbb567678a4d Temporarily apply async test attributes when compositing. r=botond,kats https://hg.mozilla.org/integration/autoland/rev/430f2e4f9d3b Use layout viewport transformations to async-adjust fixed position elements. r=botond https://hg.mozilla.org/integration/autoland/rev/232b161a64f9 Add a mochitest for fixed position hit-testing. r=botond https://hg.mozilla.org/integration/autoland/rev/ac55e50f906c Add reftests for main-thread scroll behaviour. r=botond https://hg.mozilla.org/integration/autoland/rev/ca7f6311613c Add async-scroll reftests. r=botond https://hg.mozilla.org/integration/autoland/rev/bac4139e4ff9 Add failing fixed-position async-zoom reftests. r=botond
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (parental leave) from comment #147) > nit: add a MOZ_RAII decoration here Tried adding this, but ran into OS X build failures on Try [1]. From my understanding, this is because we're returning a dynamically-allocated instance of the RAII class in APZSampler::ApplyAsyncTestAttributes, which MOZ_STACK_CLASS [2] doesn't seem to like (correct me if I'm mistaken). [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=382c69a0cbc49ef9207186dfca9c72554a2e76cc&selectedJob=194110261 [2] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/mfbt/Attributes.h#558-564
Ah, ok. That's fine, although given that there's only one call site to the APZSampler function we could have inlined that into the call site, and created all of the instances on the stack instead of via MakeUnique. But it's not a big deal.
Blocks: 1484884
Blocks: 1489630
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (parental leave) from comment #173) > Ah, ok. That's fine, although given that there's only one call site to the > APZSampler function we could have inlined that into the call site, and > created all of the instances on the stack instead of via MakeUnique. Good point. The UniquePtr was necessary in an earlier iteration, but it no longer is now. I'll implement your suggestion in bug 1489630.
Attachment #9007340 - Attachment is obsolete: true
Devices: - Samsung Galaxy Note 8 (Android 8); - OnePlus Two (Android 6.0.1). While testing I have noticed this issue reproducing on Nightly builds, verified on Beta and it's no longer reproducible, the same goes for Release. Seeing as bug 1476942 & bug 1465618 have been closed as duplicates and the tracking for this is done here, I am reopening the issue due to the fact that Nightly is still affected by this.
Status: RESOLVED → REOPENED
status-firefox62: --- → unaffected
status-firefox63: fixed → verified
status-firefox64: --- → affected
Resolution: FIXED → ---
Version: unspecified → 64 Branch
(In reply to Bogdan Surd, QA [:BogdanS, NI] from comment #178) > Devices: > - Samsung Galaxy Note 8 (Android 8); > - OnePlus Two (Android 6.0.1). > > While testing I have noticed this issue reproducing on Nightly builds, > verified on Beta and it's no longer reproducible, the same goes for Release. > > Seeing as bug 1476942 & bug 1465618 have been closed as duplicates and the > tracking for this is done here, I am reopening the issue due to the fact > that Nightly is still affected by this. Bogdan, could you provide some details on what "this issue" is (i.e. some STR)? This is an architectural change that can have a variety of potential user-visible effects.
Flags: needinfo?(bogdan.surd)
Attached video video.mp4 (deleted) —
STR: 1. Add a page to the reading list; 2. Scroll up and down the content of the page; 3. Pay attention to 'Aa' option. Expected result: Scroll down - "Aa" option not displayed. Scroll up - "Aa" option displayed and stays in a fixed position Actual result: When the "Aa" option is displayed it scrolls up with the page, sometimes jumps up and down before remaining in a fixed position.
Flags: needinfo?(bogdan.surd)
This looks like a dynamic toolbar issue. I can reproduce the problem with "full screen browsing" enabled but can't reproduce if I disable that setting.
Indeed if toggling the "full screen browsing" option the issue is no longer reproducible. However, the changes made here seem to be at fault. Thanks @Kartikaya for pointing that out. Here is a regression-window: Last good revision: c6e7b65bf8b02a32a6c1d583eb1d79e3116d692d First bad revision: bac4139e4ff9b3071e1ce17113ac65ed1d8e8598 Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c6e7b65bf8b02a32a6c1d583eb1d79e3116d692d&tochange=bac4139e4ff9b3071e1ce17113ac65ed1d8e8598
Depends on: 1493742
Depends on: 1495055
Thanks, Bogdan. I filed bug 1495055 to track the described issue with Reader Mode, and will re-close this bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Depends on: 1497762
Depends on: 1497884
No longer depends on: 1497884
Depends on: 1493976
Regressions: 1532386
Regressions: 1542832
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: