Closed Bug 1124099 Opened 5 years ago Closed 5 years ago

Scrolling overflowing iframes doesn't work on osx + apzc.

Categories

(Core :: Panning and Zooming, defect)

x86
macOS
defect
Not set

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)

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.
Blocks: graphene
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)
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)
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)
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.
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.
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.
Attached patch WIP fix (obsolete) — Splinter Review
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.
Looks like kats is already on the task.
Flags: needinfo?(mstange)
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)
Patch appears to work! Thanks a lot for looking at this.
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 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-
https://hg.mozilla.org/mozilla-central/rev/92582779bc79
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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 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+
You need to log in before you can comment on or make changes to this bug.