Closed Bug 1271432 Opened 3 years ago Closed 3 years ago

Scrolling root frame instead of scrollable subframe on this page

Categories

(Core :: Panning and Zooming, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

()

Details

(Whiteboard: gfx-noted)

Attachments

(4 files)

STR:
 1. Go to http://janeswalk.org/canada/toronto and wait for it to load completely.
 2. Scroll down to the part where there's a map on the right and a list of walks in a scrollable subframe on the left.
 3. Move your mouse into the right column of the scrollable subframe.
 4. Scroll.

APZ scrolls the whole page instead of the subframe.

If the mouse is in the left 40% of the subframe, we successfully scroll the subframe.

The X position threshold seems to be aligned with the left edge of the star in the "Lead a Walk!" button above the subframe, but I'm not sure if that means anything.
Whiteboard: gfx-noted
It looks like there's an anchor element which has an ::after thingy that is positioned rather far away. They get squashed onto the same layer which overlaps the scrollable element. Here's a pretty reduced test case, although some of the CSS properties are probably still redundant. Turn on layer borders to see how the layerizing is happening.
More importantly, the hit region of that layer is the entire layer, and so APZ targets that during hit-test. If you load the reduced test case with layers dumping enabled and then paste the layers dump into dvander's visualizer [1] it's pretty straightforward to see what's happening. I'm not entirely sure which part of the chain is the bug though.

[1] http://people.mozilla.org/~danderson/layers/
From the display list dump the event regions look ok, but the in the layers dump the two small regions have been merged into a giant area. I suspect http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=2ccaac8616e4#3285 is the culprit. I'll dig into this, I'm poking around in the event regions code for touch-action anyway.
Assignee: nobody → bugmail.mozilla
Attached patch Part 1 - FixSplinter Review
The basic fix is to keep the regions rather than using the bounding rect. We need to do this for all the event regions that are not safe to expand arbitrarily. Note that in the cases where the transform does expand it (because it's a non-axis-aligned transform) we merge that area into the dispatch-to-content hit region, so that's still ok.
Attachment #8753156 - Flags: review?(tnikkel)
The previous patch can potentially result in many calls to recompute the same transform matrix where we were just doing it once before. This optimization allows the caller to cache the transform matrix.
Attachment #8753157 - Flags: review?(tnikkel)
Attached patch Part 3 - TestSplinter Review
This is the reduced test case turned into a mochitest.
Attachment #8753158 - Flags: review?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> I have a fix and test, waiting for try run at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=64dece1af217

It might be worth throwing in some talos runs; we've had performance issues with event region accumulation code in the past.
(In reply to Botond Ballo [:botond] from comment #8)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > I have a fix and test, waiting for try run at
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=64dece1af217
> 
> It might be worth throwing in some talos runs; we've had performance issues
> with event region accumulation code in the past.

Yes, I'm definitely worried about that with this patch.
Attachment #8753156 - Flags: review?(tnikkel) → review+
Comment on attachment 8753157 [details] [diff] [review]
Part 2 - Optimization

Since we use the same matrix a bunch of times in FrameLayerBuilder in a row, it might make sense to let FrameLayerBuilder use this matrix cache approach, as a further optimization.
Attachment #8753157 - Flags: review?(tnikkel) → review+
Attachment #8753158 - Flags: review?(tnikkel) → review+
I did a talos push at [1] with results at [2]. So far it's looking like there's not much impact, although a lot of the results are falling in the "uncertain" category. Certainly no obvious regressions, though.

(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Since we use the same matrix a bunch of times in FrameLayerBuilder in a row,
> it might make sense to let FrameLayerBuilder use this matrix cache approach,
> as a further optimization.

This is a good point, I can do that as well.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b5e5d60664&group_state=expanded
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=d0be57e84807&newProject=try&newRevision=38b5e5d60664&framework=1&showOnlyImportant=0
Oh, we can probably also use RegionBuilder to help here. I'll do it in a follow-up.
Comment on attachment 8753156 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: in some rare cases, if elements on the page are arranged so that they are on the same transparent layer that overlaps a subframe, the subframe may not be scrollable properly
[Describe test coverage new/current, TreeHerder]: test is included in patchset
[Risks and why]: fairly low risk correctness-wise. there were perf concerns but talos hasn't reported any perf regressions either, and it should have if there was something.
[String/UUID change made/needed]: none
Attachment #8753156 - Flags: approval-mozilla-aurora?
Attachment #8753157 - Flags: approval-mozilla-aurora?
Attachment #8753158 - Flags: approval-mozilla-aurora?
Comment on attachment 8753156 [details] [diff] [review]
Part 1 - Fix

Improve APZ, taking it.
Attachment #8753156 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8753157 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Kats,

the test can land with a=test-only but it has conflicts

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 04f2534e0260
grafting 345117:04f2534e0260 "Bug 1271432 - Add a mochitest. r=tnikkel"
remote changed gfx/layers/apz/test/mochitest/test_group_wheelevents.html which local deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? c
merging gfx/layers/apz/test/mochitest/mochitest.ini
warning: conflicts while merging gfx/layers/apz/test/mochitest/mochitest.ini! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

can you take a look, thanks!
Flags: needinfo?(bugmail.mozilla)
Looks like the test would need to be rebased across the refactoring done in bug 1264017.
Botond, could you take care of this? Thanks
I'll do it today.
Flags: needinfo?(bugmail.mozilla)
Attachment #8753158 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.