mozilla :: #apz

15 Mar 2017
16:26botondtnikkel: apologies for the confusion in bug 1346109. i saw your "this approach won't work" comment and responded to it, before seeing the "this approach will work after all" comment :)
16:26firebothttps://bugzil.la/1346109 NEW, tnikkel@gmail.com don't hit the "no displayport base rect set" case in practice
18:36rbarkerkats: ping?
18:37katsrbarker: pong
18:38rbarkerkats: So I've hit a small issue with the new dynamic toolbar and was wondering if you had to deal with this.
18:38* kats is filled with a sense of foreboding
18:39rbarkerkats: When I'm slowly dragging the toolbar up or down, the events don't look like they are changing so it is interpreted as a long press.
18:39rbarkerkats: did you have to deal with that? I'm not sure how to prevent it being interpreted as a long press.
18:40katsrbarker: i think in the current model you have to scroll a little before the toolbar starts stealing events
18:40katsrbarker: and you can't go into long-press detection after scrolling
18:40rbarkerkats: Ah, okay, thanks, I haven't implemented the delay yet.
18:41rbarkerkats: I'll do that now.
18:46rbarkerkats: things are actually going pretty well. Fixed footer elements no longer bounce. I have a few more things left to implement but I think the hard part is done.
18:47katsrbarker: great to hear!
19:19rbarkerkats: The delay fixed it thanks :)
19:19katscool
19:44botondmstange: ping?
19:44mstangebotond: pong
19:44botondmstange: do you know where display-list building for fixed-position items happens?
19:45mstangebotond: when you encounter the placeholder frame for that fixed position frame during regular display list building
19:45mstangebotond: one sec, let me look up the code
19:45botondmstange: i would expect it to be in nsPlaceholderFrame::BuildDisplayList, but that's a debug-only function...
19:46mstangebotond: http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2796
19:46mstange" child = placeholder->GetOutOfFlowFrame();"
19:47mstangethe "child" variable is mutated to point to the out-of-flow frame
19:52botondmstange: thanks. another question: would you expect the clip chain of the resulting FixedPosition display item to contain a content clip that clips to the viewport (or something smaller)?
19:53mstangebotond: yes, I would
19:54mstangebotond: http://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3447
19:54mstangebotond: the root scroll frame has mClipAllDescendants == true, and it's an ancestor of the placeholder frame
19:58botondmstange: and would you expect that clip to show up as an entry in that FixedPosition display item's mClipChain?
19:58mstangebotond: possibly merged with a containing block clip, but yes
19:59botondmstange: so, if the FixedPosition display item has an empty clip chain, something went wrong?
19:59mstangebotond: when the FixedPosition display item is created, DisplayListClipState::GetCurrentCombinedClipChain should find that DisplayItemClipChain in its mClipChainContentDescendants
19:59mstangebotond: yes, I'd say so
20:05botondmstange: in that function, both mClipChainContentDescendants and mClipChainContainingBlockDescendants are nullptr
20:05mstangebotond: okay, mClipChainContentDescendants should definitely not be null
20:06mstangebotond: is it null after the line http://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3447 ?
20:06mstangeI'd expect not
20:06mstange(but I think it'll also be a different ClipState object, not sure)
20:07botondmstange: are you asking about display list building for the placeholder frame's enclosing scroll frame, up-stack?
20:09mstangebotond: correct
20:10mstangethat scroll frame should be setting mClipChainContentDescendants to something non-null, but by the time we get to the fixed frame, it's null again, so something must have nulled it out in between.
20:15botondmstange: the BuildDisplayList call for the enclosing scroll frame early-exits here: http://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3341
20:16mstangebotond: aha!
20:16mstangeignoringThisScrollFrame strikes again
20:17mstangewell, it works as advertised, really - we ignore that scroll frame's clip...
20:18mstangebotond: I don't know why we have an IgnoreScrollFrame in that case, but maybe tnikkel knows
20:18tnikkelwhich case?
20:19mstangetnikkel: hit testing
20:20mstangeon a certain testcase with a fixed position element
20:20mstangebotond: what's the bug #?
20:20botondmstange: we are asking it to: http://searchfox.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp#613
20:20mstangeoh haha
20:21botond(the bug is bug 1346632)
20:21firebothttps://bugzil.la/1346632 NEW, botond@mozilla.com Cannot scroll by mouse-dragging the scrollbar thumb (reduced test case included)
20:23mstangebotond: my guess is that removing that argument will fix your bug, but it might cause regressions on Android
20:23mstangetnikkel: do you know why we pass nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME in that call?
20:23tnikkelwe might need that argument for when the page has async scroll that hasn't been propagated to layout yet? ie the point could be clipped out by the scroll frame after we adjust for async scroll
20:24mstangeah
20:24mstangeI hope we've fixed that case by flushing the current scroll position to layout before we hit test
20:24mstangebecause ignoring clips is really fragile and prone to bugs like this one
20:25botondkats: ping?
20:26katsbotond: in a meeting, but pong
20:27botondkats: when you wrote the initial version of the code that's now in PrepareForSetTargetAPZCNotification (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=918288&attachment=8526205), do you recall why you passed IGNORE_ROOT_SCROLL_FRAME as an argument to nsLayoutUtils::GetFrameForPoint()?
20:27botondkats: was it cargo-culted from somewhere, or did we actually need it for some reason?
21:09katsbotond: i don't remember precisely, but in general the IGNORE_ROOT_SCROLL_FRAME was needed in some places for hit-testing properly on mobile. because when you zoom out, the root scroll frame shrinks so as to not include all the things visible to the user. and then if we don't have the IGNORE_ROOT_SCROLL_FRAME, it clips the hit-test
21:10katsbotond: it looks like in this case it would have been needed on FxOS for touch hit-testing
21:16botondkats: do you think it should be safe to remove now
21:16botond?
21:17katsbotond: not if fennec gets e10s
21:17kats(which is planned)
21:18botondkats: why is this specific to e10s? PrepareForSetTargetAPZCNotification is called by nsBaseWidget, too
21:21botondkats: oh, but i guess is a non-e10s scenario, the widget's root document isn't the thing that's zoomed
21:22botondkats: would you consider is reasonable to pass IGNORE_ROOT_SCROLL_FRAME on fennec only?
21:22botondit*
21:23katsbotond: yeah that should be ok
21:24katsi don't think we need it on desktop
16 Mar 2017
No messages
   
Last message: 45 days and 2 hours ago