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

VERIFIED FIXED in Firefox 55

Status

()

Core
Layout
P2
normal
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: 684sigma, Assigned: botond)

Tracking

52 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox53 wontfix, firefox54 wontfix, firefox55 verified)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
Created attachment 8859765 [details]
issue with scrollbar, by reporter.htm

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
(Reporter)

Updated

7 months ago
See Also: → bug 1357904

Comment 1

7 months ago
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
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
Ever confirmed: true
Priority: -- → P2
Whiteboard: [gfx-noted]
I can repro on OS X, seems to only happen with APZ enabled.
(Assignee)

Comment 4

7 months ago
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
(Assignee)

Comment 5

7 months ago
Created attachment 8861226 [details]
Testcase

Here's the testcase reduced a bit further and cleaned up.
(Assignee)

Comment 6

7 months ago
This seems to have been broken ever since APZ was first enabled, so a regression range isn't going to be useful.
(Assignee)

Comment 7

7 months ago
Created attachment 8861239 [details]
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)
(Assignee)

Comment 8

7 months ago
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
(Assignee)

Updated

7 months ago
Blocks: 1357904
See Also: bug 1357904
(Assignee)

Comment 9

7 months ago
(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)
(Assignee)

Comment 10

7 months ago
(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.
(Assignee)

Comment 11

7 months ago
(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.
(Assignee)

Comment 12

7 months ago
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.
(Assignee)

Comment 13

7 months ago
(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
(Assignee)

Comment 14

7 months ago
Created attachment 8863077 [details] [diff] [review]
WIP patch

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+
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8863077 - Attachment is obsolete: true
(Assignee)

Comment 17

7 months ago
(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.
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1357904

Comment 19

7 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 21

7 months ago
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
(Assignee)

Comment 22

7 months ago
Addressed review comment and landed.

Comment 23

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd70682a7818
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Mark 54 won't fix as this is late in Beta54.
status-firefox54: affected → wontfix
Flags: qe-verify+

Updated

5 months ago
See Also: → bug 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
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.