Closed
Bug 1271432
Opened 8 years ago
Closed 8 years ago
Scrolling root frame instead of scrollable subframe on this page
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mstange, Assigned: kats)
References
()
Details
(Whiteboard: gfx-noted)
Attachments
(4 files)
4.64 KB,
text/html
|
Details | |
4.58 KB,
patch
|
tnikkel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
7.12 KB,
patch
|
tnikkel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
9.78 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: gfx-noted
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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/
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
I have a fix and test, waiting for try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=64dece1af217
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
This is the reduced test case turned into a mochitest.
Attachment #8753158 -
Flags: review?(tnikkel)
Comment 8•8 years ago
|
||
(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.
Comment 9•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8753156 -
Flags: review?(tnikkel) → review+
Comment 10•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8753158 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/585842c2cc5e https://hg.mozilla.org/integration/mozilla-inbound/rev/a268bd0cc56c https://hg.mozilla.org/integration/mozilla-inbound/rev/04f2534e0260
Assignee | ||
Comment 13•8 years ago
|
||
Oh, we can probably also use RegionBuilder to help here. I'll do it in a follow-up.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/585842c2cc5e https://hg.mozilla.org/mozilla-central/rev/a268bd0cc56c https://hg.mozilla.org/mozilla-central/rev/04f2534e0260
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 15•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #8753157 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8753158 -
Flags: approval-mozilla-aurora?
Comment 16•8 years ago
|
||
Comment on attachment 8753156 [details] [diff] [review] Part 1 - Fix Improve APZ, taking it.
Attachment #8753156 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8753157 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab48a2ac5cda https://hg.mozilla.org/releases/mozilla-aurora/rev/3fa0f58b2339
Comment 19•8 years ago
|
||
Looks like the test would need to be rebased across the refactoring done in bug 1264017.
Comment 20•8 years ago
|
||
Botond, could you take care of this? Thanks
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f96ed625945
Assignee | ||
Updated•8 years ago
|
Attachment #8753158 -
Flags: approval-mozilla-aurora?
You need to log in
before you can comment on or make changes to this bug.
Description
•