mozilla :: #apz

6 Oct 2017
16:58botondtnikkel: ping?
18:52kats-afkbotond: ping
18:52botondkats: pong
18:52katsbotond: hey, question about your nsDisplayMask changes
18:53katsdo those have any impact on the clip chains of items inside the mask?
18:53kats(i haven't really read the patches)
18:57botondkats: i think so
18:59botondkats: we clip descendant items here: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2582
18:59botondkats: and my patch modifies that clip (potentially causing it to go from Nothing to Some): https://hg.mozilla.org/try/rev/078fca6a9b81#l1.186
19:00botondkats: i would think that impacts the clip chains of the descendants, but mstange can answer that more definitively
19:00katsbotond: ok, thanks
19:23botondkats: ping
19:24katsbotond: pong
19:25botondkats: hey, i have a question for you about ScrollOffsetUpdate stuff
19:25katssure
19:25botondkats: so we have a bunch of scroll offset updates types here: http://searchfox.org/mozilla-central/source/gfx/layers/FrameMetrics.h#55
19:25botondkats: and one of them is ePending
19:26botondkats: and that seems to be used during paint skipping: http://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2902
19:27botondkats: and having any scroll update type besides eNone causes FrameMetrics::GetScrollOffsetUpdated() to return true: http://searchfox.org/mozilla-central/source/gfx/layers/FrameMetrics.h#394
19:28botondkats: and if that's the case, the variable "scrollOffsetUpdated" in NotifyLayersUpdated can become true: http://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#3768
19:28botondkats: and one of the things we do if that variable is true is call CancelAnimation(): http://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#3888
19:28botondkats: so... paint skipping causes animations to be cancelled? is that intentional?
19:29katsbotond: presumably if you turn off paint-skipping, you get a full repaint transaction instead of an ePending scroll update, and that would also cancel the animations
19:29katsbotond: so it seems reasonable to me that the animation-cancelling is consistent with and without paint skipping
19:30botondkats: what scroll offset update type would you expect to receive if we turn off paint-skipping?
19:31katsbotond: eMainThread
19:34botondkats: what if the ScrollToImpl() call that would do paint skipping is called with aOrigin=nsGkAtoms::apz?
19:35botondkats: oh, never mind, then we don't get into the branch that sets ePending
19:36botondkats: perhaps my problem is orthogonal to paint-skipping, then
19:36* botond tries to repro without paint skipping
19:40tnikkelbotond: pong
19:45botondtnikkel: hey, thanks for your feedback in https://bugzilla.mozilla.org/show_bug.cgi?id=1402995#c11
19:45firebotBug 1402995 NEW, botond@mozilla.com Scroll thumb not synchronized with mouse
19:46botondtnikkel: you're right, i overlooked the fact that CurPosAttributeChanged does scrolling
19:47botondtnikkel: i agree that we should avoid calling that in the partial reflow case to make sure we don't regress bug 1092626
19:47firebothttps://bugzil.la/1092626 FIXED, roc@ocallahan.org [non-e10s] twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image
19:50botondtnikkel: regarding the FrameNeedsReflow() calls, those are being called for position:fixed children
19:50botondtnikkel: i think we could probably go either way on whether we reflow position:fixed children?
19:50botondtnikkel: i mean, i don't believe doing so is necessary for fixing my bug, but i also don't think avoiding it was the point of the change in bug 1092626
19:51tnikkelbotond: that part doesn't seem too important, yes
19:51botondtnikkel: one other thing that i didn't think about previously, was the return value of ReflowFinished()
19:52botondtnikkel: the documentation in nsIReflowCallback.h says "return true if you need a FlushType::Layout to happen after this"
19:52botondtnikkel: with the change in bug 1092626, we would return false in the partial reflow case
19:53botondtnikkel: whereas with my change, we return true (unless we taken another one of the early-exits, like mUpdateScrollbarAttributes=false or (!mHScrollbarBox && !mVScrollbarBox))
19:53botondtnikkel: i was just wondering whether that's appropriate
19:54tnikkelbotond: i think that'll make us reflow right away, thus breaking the interuptable part of interuptable reflow
19:54botondtnikkel: ok - so we want to keep returning false?
19:57botondi guess i should check that that doesn't break my fix
20:02botondtnikkel: so, unfortunately, returning false breaks my fix
20:05tnikkelbotond: you skipped calling CurPosChanged in your fix too?
20:07botondtnikkel: actually!
20:07botondtnikkel: it's not returning false that breaks my fix, it's not call CurPosChanged!
20:07botondnot calling*
20:07tnikkelyeah, that makes sense
20:07tnikkelcurposchanged seems important
20:08botondtnikkel: hmm, so... should we only skip the ScrollToWithOrigin call inside CurPosAttributeChanged?
20:10tnikkelyeah
20:10tnikkelthe other stuff is setting props on scrollbars, ie the part you want
20:27botondkats: huh... i'm getting ePending updates even with paint-skipping disabled...
20:28botondkats: nevermind. i just don't know how to change prefs...
20:28katslol, phew
20:28* botond only changed it in gfxPrefs.h
20:28katsit's a live pref, you can change it in about:config
20:29botondkats: my bug only repros on Try
20:29botondkats: (sometimes after 100+ runs...)
20:29katsoh
20:31botondtnikkel: ok, only skipping the ScrollToWithOrigin call inside CurPosAttributeChanged, and returning false form ReflowFinished, seems to work
20:31botondtnikkel: that still leaves the question i asked in https://bugzilla.mozilla.org/show_bug.cgi?id=1402995#c10
20:31firebotBug 1402995 NEW, botond@mozilla.com Scroll thumb not synchronized with mouse
20:32botondtnikkel: "...as a result of this change, the scrollbar briefly changes to be long before becoming short again. Do you think that's an acceptable side effect?"
20:37tnikkelbotond: yeah, thats pretty much a required side effect. unless you somehow fake the values we set on the scrollbar attributes
20:41botondtnikkel: ok - in that case, i think we have a fix!
7 Oct 2017
No messages
   
Last message: 11 days and 2 hours ago