Closed Bug 1459696 Opened 8 years ago Closed 8 years ago

Scrolling on fixed div does not perform as expected (due to incorrect APZ target?)

Categories

(Core :: Panning and Zooming, enhancement, P2)

Product:
Core
Component:
Panning and Zooming
Version:
59 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: joshuapatrickcarpenter, Assigned: botond)

References

(
URL
)

Details

(Whiteboard: [specification][type:bug][gfx-noted])

Attachments

(6 files, 6 obsolete files)

Reduced testcase
1.12 KB, text/html
Details
Display list dump
20.35 KB, text/plain
Details
Bug 1459696 - Add a mochitest.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1459696 - Remove an unused overload of SetTargetAPZC().
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1459696 - Improve const-correctness of nsIFrame methods and functions that accept an nsIFrame* argument.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1459696 - Respect nested inactive layer clips when combining a layer's event regions into its ancestors'.
59 bytes, text/x-review-board-request
botond
: review+
Details
What did you do? ================ 1. I created a web site view with a fixed layout 2. I created a fixed div with the attribute overflow: auto that is expected to scroll when the content length exceeds the height. What happened? ============== When the content exceeds the height of the div, a scrollbar appears, as it should. However, I cannot scroll using the mouse wheel, and I cannot scroll by dragging the scrollbar. I can scroll by using the scrollbar arrows. What should have happened? ========================== I should be able to scroll using all three methods Is there anything else we should know? ====================================== This has been tested in Chrome and Microsoft Edge and is working as expected in those two browsers. Also it may be worth noting that the height of this div is not being set with the height attribute, rather the top and bottom values are set and the height is determined from those.
What is your website's URL? If your website is not public, can you create the equivalent code in https://codepen.io/ or https://jsfiddle.net/?
Component: General → Untriaged
Flags: needinfo?(joshuapatrickcarpenter)
Product: developer.mozilla.org → Firefox
Here is an example The div I'm referring to is on the left. http://mathewsarchitecture.com.139.purplecat.net/what-we-design/multifamily/hudson-street-cottages/
Flags: needinfo?(joshuapatrickcarpenter)
I got a message "The requested content could not be found." the first time I visited. I was able to see it by navigating to "WHAT WE DESIGN" -> "RESIDENTIAL" -> "MULTI-FAMILY", then select "Hudson Street Cottages". I have a MacBook, so I use a touchpad. The left scrollbar works with a scroll gesture on: * Safari 11.1 * Chrome 66.0 It doesn't work with * Firefox 59 * Firefox 61.0b2 (dev edition)
URL: http://mathewsarchitecture.com.139.pu...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → Windows 10
Hardware: All → x86
Version: unspecified → 59 Branch
With Nightly on macOS 10.13, I get an insta-crash (of the whole browser, not just the tab) when I click the thumb of the scrollbar. Clicking below the thumb (or above it, once the pane has been scrolled) works as expected, but clicking the thumb (as if to start dragging it) crashes the chrome process. https://crash-stats.mozilla.com/report/index/c00e6818-2c26-4d78-aa7f-7d92f0180509 So in this case we're hitting the (Nightly-only) MOZ_RELEASE_ASSERT at https://hg.mozilla.org/mozilla-central/annotate/59005ba3cd3e7b3f9e8804bea881bf4c3a755d7c/gfx/layers/apz/src/InputBlockState.cpp#l72. Moving to Core:Panning&Zooming, as this seems to be an APZ-related issue.
Component: Untriaged → Panning and Zooming
OS: Windows 10 → All
Product: Firefox → Core
Hardware: x86 → All
Summary: Scrolling on fixed div does not perform as expected. → Scrolling on fixed div does not perform as expected (due to incorrect APZ target?)
Assignee: nobody → botond
Priority: -- → P2
Whiteboard: [specification][type:bug] → [specification][type:bug][gfx-noted]
Thank you for the testcase! The good news is that thanks to the work done in bug 1437694, in the Firefox 60 release this page behaves fine (albeit using a slightly slower codepath for starting the scrollbar drag). There is still an underlying issue that prevents us from using the fast path for the scrollbar drag, which is what the Nightly diagnostic assert is drawing attention to, and which I will investigate and fix in this bug.
Blocks: 1437694
(In reply to Botond Ballo [:botond] from comment #5) > The good news is that thanks to the work done in bug 1437694, in the Firefox > 60 release this page behaves fine (albeit using a slightly slower codepath > for starting the scrollbar drag). Slight correction: in Firefox 60, dragging the scrollbar on the page behaves fine. Scrolling with the mouse wheel still does not, since the mitigation implemented in bug 1437694 only applies to scrollbar dragging. (An interesting question is whether a similar mitigation can be implemented for mouse wheel scrolling. That's probably worth some investigation.)
Attached file Reduced testcase —
Here is a reduced testcase that demonstrates the issue.
Attached file Display list dump —
It looks like the overflowing content of the right pane, even though it's clipped away visually by the overflow:hidden, is eating the events that are intended to go to the left pane. This can be seen in the attached display list dump, where the layer representing the contents of the right pane (0x7f390d9f3000) has a hit region < (x=-856, y=0, w=2001, h=603); > which extends to cover the left pane as well, and this layer sits on top of the layers representing the contents of the left pane (such as 0x7f39091a9000).
(In reply to Botond Ballo [:botond] from comment #8) > This can be seen in the attached display list dump, where the layer > representing the contents of the right pane (0x7f390d9f3000) has a hit > region < (x=-856, y=0, w=2001, h=603); > which extends to cover the left > pane as well, and this layer sits on top of the layers representing the > contents of the left pane (such as 0x7f39091a9000). The excessively large hit region comes from the HitTestInfo display item 0x7f390f7142a0 in this dump. That's inside a Transform which is correctly clipped to the overflow:hidden bounds. Since transforms "capture" clips, it's expected that the clip is not propagated down to the descendant item. However, during flattening of the HitTestInfo display item into an EventRegion, the clip on the ancestor Transform item should be respected, and it looks like it's not for some reason.
(In reply to Botond Ballo [:botond] from comment #6) > (An interesting question is whether a similar mitigation can be implemented > for mouse wheel scrolling. That's probably worth some investigation.) Unfortunately, I think the answer is not really, or at least not easily. The recovery codepath where we replace the hit test target computed by APZ with the one computed by the main thread happens at the granularity of input blocks. Scrollbar drags have the property that the a drag gesture (from mouse-down to mouse-up) is one input block, and we often have to wait for feedback from the main thread to start the drag anyways. Wheel events, however, are typically processed by APZ right away, and multiple wheel events are only grouped into one input block if they are part of the same "wheel transaction". Any number of things can interrupt a wheel transaction, such as any mouse movement, enough time elapsing, and so on. (On this particular page, we don't create a transaction at all, because the (incorrect) APZ target is not even scrollable.) Since the mitigation would only apply to _subsequent_ wheel events within a transaction, many wheel events could still be processed with the wrong target. The only way I can see a mitigation working for wheel events is if, upon detecting a mismatch, we disable APZ on the page altogether. That's something we could entertain, but it's not as obvious of a win as bug 1437694 was for scrollbar dragging.
Joshua, if you're looking for a workaround for the mouse wheel issue that remains in Firefox 60, it looks like removing the following CSS rule from http://mathewsarchitecture.com.139.purplecat.net/css/slick.css does the trick: .slick-slider .slick-track, .slick-slider .slick-list { -webkit-transform: translate3d(0, 0, 0); -moz-transform: translate3d(0, 0, 0); -ms-transform: translate3d(0, 0, 0); -o-transform: translate3d(0, 0, 0); transform: translate3d(0, 0, 0); }
Hey everybody, thanks for weighing in on this. Botond Ballo, looks like you had the right assessment earlier, the Slick carousel I was using was creating overflow content that was covering the scroll track. I was confused because when I used the Firefox developer tools to see what I was clicking on (Inspect Element), it did not indicate that. And I suppose that might be where the real bug lives. Anyway, problem is solved with a z-index change. I appreciate all of your help!
(In reply to joshuapatrickcarpenter from comment #12) > the Slick carousel I was using was > creating overflow content that was covering the scroll track. I was confused > because when I used the Firefox developer tools to see what I was clicking > on (Inspect Element), it did not indicate that. And I suppose that might be > where the real bug lives. I think the carousel code is correct, as there is an overflow:hidden that's supposed to clip the overflowing content away. The developer tools are also correct to respect the overflow:hiddden. It's the browser's hit testing code that buggy and doesn't seem to respect the overflow:hidden.
(In reply to Botond Ballo [:botond] from comment #9) > The excessively large hit region comes from the HitTestInfo display item > 0x7f390f7142a0 in this dump. That's inside a Transform which is correctly > clipped to the overflow:hidden bounds. Since transforms "capture" clips, > it's expected that the clip is not propagated down to the descendant item. > However, during flattening of the HitTestInfo display item into an > EventRegion, the clip on the ancestor Transform item should be respected, > and it looks like it's not for some reason. The issue here is very similar to bug 1357903. The code in FrameLayerBuilder::FinishPaintedLayerData() that intersects the event regions of the child PLD into the parent PLD is supposed to apply the clip. Prior to bug 1357903, we didn't do that at all. In bug 1357903, we started doing it, but the place where we get the clip from is the display item that created the inactive layer tree. In this case, there are two nested Transform items, both inactive. The outer one creates an inactive layer tree, but the inner one doesn't since we're already inside an inactive layer tree. As a result, the inner one's clip is not respected (and that happens to be the important one in this case).
See Also: → 1357903
The posted patches fix the problem for me locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e39313541f1dee6fef6cd994f40130bd822826 I also plan to turn the reduced testcase into a mochitest, but I thought I'd post the fix for review in the meantime.
Attachment #8981283 - Attachment is obsolete: true
Attachment #8981283 - Flags: review?(mstange)
Attachment #8981284 - Attachment is obsolete: true
Attachment #8981284 - Flags: review?(mstange)
(There was a silly lifetime error in the original patches which was caught by Asan. Fixed in the updated patches.) https://treeherder.mozilla.org/#/jobs?repo=try&revision=c19f36be18b716a6ab7eaaed2265f0baf3624843
(One more update to fix an assertion caused by not handling an empty clip.)
Comment on attachment 8981292 [details] Bug 1459696 - Improve const-correctness of nsIFrame methods and functions that accept an nsIFrame* argument. https://reviewboard.mozilla.org/r/247396/#review256922 nice
Attachment #8981292 - Flags: review?(mstange) → review+
Comment on attachment 8981293 [details] Bug 1459696 - Respect nested inactive layer clips when combining a layer's event regions into its ancestors'. https://reviewboard.mozilla.org/r/247398/#review256924 ::: layout/painting/FrameLayerBuilder.h:706 (Diff revision 2) > const DisplayItemClip* GetInactiveLayerClip() const > { > return mInactiveLayerClip; > } > > + void SetInactiveLayerClip(const DisplayItemClip* aClip) Please add a comment about what coordinate space the clip coordinates are in.
Attachment #8981293 - Flags: review?(mstange) → review+
Attachment #8981292 - Attachment is obsolete: true
Attachment #8981293 - Attachment is obsolete: true
Here is the promised mochitest. The last patch is a cleanup to remove a function that I noticed was unused while originally debugging this. It's not really related to the fix but I thought I'd throw it in.
Comment on attachment 8986570 [details] Bug 1459696 - Remove an unused overload of SetTargetAPZC(). https://reviewboard.mozilla.org/r/251870/#review258342
Attachment #8986570 - Flags: review?(bugmail) → review+
Oops, I messed up and reverted the changes mentioned in comment 20 and comment 22. Also MozReview dropped Markus' r+ flags.
Attachment #8986567 - Attachment is obsolete: true
Attachment #8986567 - Flags: review?(mstange)
Attachment #8986568 - Attachment is obsolete: true
Attachment #8986568 - Flags: review?(mstange)
Comment on attachment 8986627 [details] Bug 1459696 - Improve const-correctness of nsIFrame methods and functions that accept an nsIFrame* argument. Carrying r+ from Markus.
Attachment #8986627 - Flags: review?(mstange) → review+
Comment on attachment 8986628 [details] Bug 1459696 - Respect nested inactive layer clips when combining a layer's event regions into its ancestors'. Carrying r+ from Markus.
Attachment #8986628 - Flags: review?(mstange) → review+
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/035c6c987073 Improve const-correctness of nsIFrame methods and functions that accept an nsIFrame* argument. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/c4312c1591dd Respect nested inactive layer clips when combining a layer's event regions into its ancestors'. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/cd61f8689a52 Add a mochitest. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a9cee249aa Remove an unused overload of SetTargetAPZC(). r=kats
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: