Closed
Bug 1124099
Opened 9 years ago
Closed 9 years ago
Scrolling overflowing iframes doesn't work on osx + apzc.
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox36 | --- | unaffected |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: paul, Assigned: kats)
References
Details
Attachments
(2 files, 2 obsolete files)
4.97 KB,
text/plain
|
Details | |
1.10 KB,
patch
|
botond
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In this configuration: > div { > background-color: #6F0; > height: 250px; > overflow: scroll; > } > > iframe { > height: 250px; > } > > h1 { > height: 50px; > margin: 0; > } > <div> > <h1>hi</h1> > <iframe src="http://paulrouget.com"></iframe> > </div> … scrolling with the mouse (wheel events) doesn't work (can't scroll div or iframe). If the iframe doesn't overflow (s/250px/450px), I can scroll the iframe. If I disable APZC, I can scroll the iframe and the div.
Reporter | ||
Comment 1•9 years ago
|
||
Markus, BenWa, is that even supposed to work? If not, can you point me to any relevant bug? (maybe bug 1011833?)
Flags: needinfo?(mstange)
Flags: needinfo?(bgirard)
Comment 2•9 years ago
|
||
Is this desktop? If so it's probably a dupe of the trackings bug we have for APZ on desktop. AFAIK we support APZ in all cases as of recently where the out of flow APZ landed. However APZ on desktop is being activity worked on by kats & dvander. You may want to test with both e10s enabled and disabled since that has a big impact on how APZ events are routed. dvander can you comment on the state of APZ if you enable it?
Flags: needinfo?(mstange)
Flags: needinfo?(dvander)
Flags: needinfo?(bgirard)
Comment 3•9 years ago
|
||
This bug concerns OS X, so mstange is a better person to ask (dvander works on APZ support for Windows).
Flags: needinfo?(dvander) → needinfo?(mstange)
Assignee | ||
Comment 4•9 years ago
|
||
I was under the impression that subframe APZ scrolling was working fine on OSX but it's been a few months since I tried it. I'll roll a master build and see what's going on.
Reporter | ||
Comment 5•9 years ago
|
||
With these prefs: - pref("layers.async-pan-zoom.enabled", true); - pref("apz.subframe.enabled", true); - pref("dom.browser_frames.useAsyncPanZoom", true); And this page: http://jsbin.com/pupijelide Doesn't work here. Maybe I missed an important pref.
Assignee | ||
Comment 6•9 years ago
|
||
This is from your example page. For one thing here the clip rect is wrong on the 0x11f844380 hit-test node. With that fixed I found another bug in the non-event-regions codepath. I'll put up a patch that lets you work around it for now, but I'm on PTO this afternoon so I can't clean up until tomorrow.
Assignee | ||
Comment 7•9 years ago
|
||
Assignee: nobody → bugmail.mozilla
I don't think we're looking to ship APZ on Mac first but this looks like the kind of bug that would affect both.
Assignee | ||
Comment 10•9 years ago
|
||
When I wrote this I thought the clip would apply to all the LayerMetrics instances. However it makes it simpler if we only apply the clip on the bottommost layer because then the hit-test tree looks more like it would with actual container layers rather than multi-framemetrics (see comment 6).
Attachment #8552646 -
Flags: review?(botond)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8552569 -
Attachment is obsolete: true
Attachment #8552647 -
Flags: review?(botond)
Reporter | ||
Comment 12•9 years ago
|
||
Patch appears to work! Thanks a lot for looking at this.
Comment 13•9 years ago
|
||
Comment on attachment 8552647 [details] [diff] [review] Part 2 - Make the code to skip over scrollinfo layers actually work Review of attachment 8552647 [details] [diff] [review]: ----------------------------------------------------------------- Heh, good catch!
Attachment #8552647 -
Flags: review?(botond) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8552646 [details] [diff] [review] Part 1 - Fix LayerMetricsWrapper::GetClip Review of attachment 8552646 [details] [diff] [review]: ----------------------------------------------------------------- Discussed this with Kats on IRC. First, it turns out the patch isn't needed to solve the problem - Part 2 by itself does so. Second, there's nothing conceptually wrong with two hit-test nodes associated with the same APZC having different clips, as the clip is a property of the layer, not of the APZC. If a hit-test fails on one node because of its clip, we still have the opportunity to hit another node belonging to the same APZC.
Attachment #8552646 -
Flags: review?(botond) → review-
Assignee | ||
Comment 15•9 years ago
|
||
Thanks! Landed the part 2 patch by itself: https://hg.mozilla.org/integration/mozilla-inbound/rev/92582779bc79
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92582779bc79
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Attachment #8552646 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8552647 [details] [diff] [review] Part 2 - Make the code to skip over scrollinfo layers actually work Approval Request Comment [Feature/regressing bug #]: bug 1109873 [User impact if declined]: On desktop FF if APZ is enabled, then iframes don't scroll. [Describe test coverage new/current, TreeHerder]: fixed a few weeks ago, no known regressions [Risks and why]: low risk, small patch. Can be skipped because APZ is not enabled on desktop by default, but we do have some users who have been running with it enabled and it would be nice to not regress the behaviour. [String/UUID change made/needed]: none
Attachment #8552647 -
Flags: approval-mozilla-aurora?
Comment 18•9 years ago
|
||
Comment on attachment 8552647 [details] [diff] [review] Part 2 - Make the code to skip over scrollinfo layers actually work Although this isn't absolutely necessary for 37, it is a very small fix for an issue that some early APZC testers will hit. 37 still have ~2 weeks on Aurora so I think we can reasonably take this. Aurora+
Attachment #8552647 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bf8bbdbabd94
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•