Closed Bug 1357903 Opened 5 years ago Closed 5 years ago

[e10s] scrolling with mouse wheel doesn't work in some cases

Categories

(Core :: Layout, defect, P2)

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: 684sigma, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

I have a problem with Firefox Beta 53. It also happens in Nightly 55, Beta 52, doesn't happen in ESR 45.
Sometimes scrolling with mouse wheel doesn't work. Here's how to reproduce the bug:

1. Open attached page
2. Try to scroll the <div> element on the page with mouse wheel

Result: Mouse wheel scrolls the page down
Expected: Mouse wheel scrolls the <div> element
See Also: → 1357904
Do you think this bug also a fix-optional?
Flags: needinfo?(miket)
fix-optional is a signal that a regression shouldn't block a release its just a regular bug. Since we apparently shipped this in 52, I guess that ship has sailed and it should be prioritized with the other known bugs, IMO.
Flags: needinfo?(miket)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [gfx-noted]
I can repro on OS X, seems to only happen with APZ enabled.
I'm going to have a look at this. As stated in bug 1357904 comment 8, I expected a fix here will fix bug 1357904 as well.
Assignee: nobody → botond
Attached file Testcase
Here's the testcase reduced a bit further and cleaned up.
This seems to have been broken ever since APZ was first enabled, so a regression range isn't going to be useful.
Attached file Display list dump
Attached is the display list dump for the page. Note that the AGR for the Text display item for "Some text" is the Canvas frame, and not the div's Block frame as we'd expect.

Markus, would you like to comment on this display list?
Flags: needinfo?(mstange)
Moving to Layout since the layers system and APZ are behaving reasonably given the display list they're being fed.
Component: Panning and Zooming → Layout
Blocks: 1357904
See Also: 1357904
(In reply to Botond Ballo [:botond] from comment #7)
> Attached is the display list dump for the page. Note that the AGR for the
> Text display item for "Some text" is the Canvas frame, and not the div's
> Block frame as we'd expect.

Markus pointed out that we only expect the AGR to be the scrollable div's Block frame once the subframe has been activated. The posted display list dump was taken an a time when the subframe was not activated.

Once the subframe is activated, the Text item gets the correct AGR, the scrolled content gets layerized, and it gets scroll metadata as expected - but still does not scroll. The plot thickens...
Flags: needinfo?(mstange)
(In reply to Botond Ballo [:botond] from comment #9)
> Once the subframe is activated, the Text item gets the correct AGR, the
> scrolled content gets layerized, and it gets scroll metadata as expected -
> but still does not scroll. The plot thickens...

There's a large layer in front of it, presumably created for the <span> elements containing the "X", that's eating all the events.
(In reply to Botond Ballo [:botond] from comment #10)
> There's a large layer in front of it, presumably created for the <span>
> elements containing the "X", that's eating all the events.

More specifically, the layer for "X" has a large hit region that's easting all the events.

This is coming from the <span class="class4"> which is sized to take up the whole screen. However, it's inside an overflow:hidden element (the <span class="class3">) which is sized to take up just one line, which is supposed to clip the larger element inside. It appears we're doing the clipping visually, but we're not clipping the event regions.
Based on discussion with Timothy, it appears that clipping event regions to overflow:hidden scrollframes is something we don't currently handle at all. We'll need to implement that to fix this bug.
(In reply to Botond Ballo [:botond] from comment #12)
> Based on discussion with Timothy, it appears that clipping event regions to
> overflow:hidden scrollframes is something we don't currently handle at all.

After further investigation, that's not quite accurate. We do clip event regions to the display list builder's current clip [1], which usually includes the clip for an enclosing overflow:hidden element.

However, in this test case, the element inside the overflow:hidden also has a transform. The way transforms are handled is that the current clip is applied to the nsDisplayTransform item, but not to the content inside the transform. As a result, the event regions, which are inside the trasnform, do not get clipped.

Instead, the transform and the associated clip are supposed to be applied in FrameLayerBuilder, when the event regions display items are combined into the event regions of the resulting layer. The transform is being applied here [2], but nowhere do we apply the associated clip - that is the bug.

[1] http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/layout/painting/nsDisplayList.cpp#4313
[2] http://searchfox.org/mozilla-central/rev/784ec1af1451fd479641b40efd7720a2f3f5781a/layout/painting/FrameLayerBuilder.cpp#3348
Attached patch WIP patch (obsolete) — Splinter Review
This patch fixes the problem for me locally.

It's a bit rough around the edges; in particular, it does not handle clips with rounded corners. Any suggestions on how to handle those would be appreciated.
Attachment #8863077 - Flags: feedback?(tnikkel)
Comment on attachment 8863077 [details] [diff] [review]
WIP patch

(In reply to Botond Ballo [:botond] from comment #14)
> It's a bit rough around the edges; in particular, it does not handle clips
> with rounded corners. Any suggestions on how to handle those would be
> appreciated.

Probably the same way nsDisplayLayerEventRegions::AddFrame handles it. If the clip is rounded we have to put the hit region into the maybe hit region.
Attachment #8863077 - Flags: feedback?(tnikkel) → feedback+
Attachment #8863077 - Attachment is obsolete: true
(In reply to Timothy Nikkel (:tnikkel) from comment #15)
> (In reply to Botond Ballo [:botond] from comment #14)
> > It's a bit rough around the edges; in particular, it does not handle clips
> > with rounded corners. Any suggestions on how to handle those would be
> > appreciated.
> 
> Probably the same way nsDisplayLayerEventRegions::AddFrame handles it. If
> the clip is rounded we have to put the hit region into the maybe hit region.

Thanks - the updated patch does this, and similar things for the other regions (e.g. rounded corners cause a vertical-pan region to be placed into the dispatch-to-content region). I also realized my original patch was forgetting to clip the maybe-hit and dispatch-to-content regions themselves, that's also fixed now.

I asked Markus to review to have an extra set of eyes on this code.
Duplicate of this bug: 1357904
Comment on attachment 8863430 [details]
Bug 1357903 - Clip event regions when combining them to a containing PaintedLayerData.

https://reviewboard.mozilla.org/r/135202/#review139324

::: layout/painting/FrameLayerBuilder.cpp:4723
(Diff revision 1)
>        entry->mContainerLayerGeneration = mContainerLayerGeneration;
>      }
>      if (tempManager) {
>        FLB_LOG_PAINTED_LAYER_DECISION(aLayerData, "Creating nested FLB for item %p\n", aItem);
>        FrameLayerBuilder* layerBuilder = new FrameLayerBuilder();
> -      layerBuilder->Init(mDisplayListBuilder, tempManager, aLayerData);
> +      layerBuilder->Init(mDisplayListBuilder, tempManager, aLayerData, &aItem->GetClip());

I think you should use aClip here instead of aItem->GetClip(), because we might have adjusted the clip for various reasons in ProcessDisplayItems and it makes more sense to use the adjusted clip.
Attachment #8863430 - Flags: review?(mstange) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd70682a7818
Clip event regions when combining them to a containing PaintedLayerData. r=mstange
Addressed review comment and landed.
https://hg.mozilla.org/mozilla-central/rev/cd70682a7818
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Mark 54 won't fix as this is late in Beta54.
Flags: qe-verify+
See Also: → 1374336
I have managed to reproduce the issue mentioned in comment 0 using Firefox 55.0a1 (BuildId:20170419030223).

This issue is verified fixed on Firefox 55.0b3 (Build Id:20170619071723) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.4.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1459696
You need to log in before you can comment on or make changes to this bug.