943206 - Images and Videos scroll when they shouldn't when in frames with overflow-y: hidden

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

http://www.pbs.org/newshour/rundown/2013/11/innovative-justice-program-sweeping-the-usa.html

Go to the above page and pan on the map graphic. It'll pan out of its frame. I've seen this on multiple pages recently.

Comment 1

[Aaron Train [:aaronmt]]

I'm able to pan the map out of the frame on all channels with your test URL, so I don't think this is anything new. This might be a dupe of a known bug. Kats?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This isn't a dupe of any bug I'm aware of. The iframe containing the image has overflow-y: hidden set so we really shouldn't be scrolling it.

Comment 3

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> This isn't a dupe of any bug I'm aware of. The iframe containing the image
> has overflow-y: hidden set so we really shouldn't be scrolling it.

Sounds like the sort of thing that should have been fixed by bug 942995, which just landed recently. Does this problem still occur with that fix?

Comment 4

[Aaron Train [:aaronmt]]

This is still reproducible on today's Nightly (12/02) with the URL in this bug.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #3)
> Sounds like the sort of thing that should have been fixed by bug 942995,
> which just landed recently. Does this problem still occur with that fix?

Fennec doesn't use APZC so the codepaths are different.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to Botond Ballo [:botond] from comment #3)
> > Sounds like the sort of thing that should have been fixed by bug 942995,
> > which just landed recently. Does this problem still occur with that fix?
> 
> Fennec doesn't use APZC so the codepaths are different.

Oh whoops, I'm so immersed in B2G I didn't realize this was a Fennec bug.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Botond, can you try reproducing with the non-java APZC? if it doesnt' reproduce then just mark this bug as depending on switching over.

Comment 8

[Botond Ballo [:botond]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Botond, can you try reproducing with the non-java APZC? if it doesnt'
> reproduce then just mark this bug as depending on switching over.

needinfoing myself so I don't forget

Comment 9

[Botond Ballo [:botond]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Botond, can you try reproducing with the non-java APZC? if it doesnt'
> reproduce then just mark this bug as depending on switching over.

I can't even get the image on the example page to fully load on my Samsung GT-I5510M... will try on a better device.

Comment 10

[Botond Ballo [:botond]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Botond, can you try reproducing with the non-java APZC? if it doesnt'
> reproduce then just mark this bug as depending on switching over.

The behaviour does not reproduce with the native APZC. I updated the bug's dependency accordingly.

Comment 12

[Aaron Train [:aaronmt]]

Unsimplified test-case:

http://people.mozilla.org/~atrain/mobile/tests/embed-youtube.html

You can scroll off that poster image on the HTML5 video including the big Play button.

This is reproducible on all channels.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

From what I can tell there is not a clear timeline for implementing the native APZC and this bug is an issue with the java one - so can we do something intermediate to fix this when using the java APZC?

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Not sure if Mark or Brad are in the best position to prioritize/assign this but since it's affecting more than just images I'd like to nudge up the criticality of this issue.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

I'm busy with B2G work at the moment but I can certainly mentor somebody through debugging this. The initial debug steps should be pretty straightforward - for some reason _findScrollableElement in browser.js is finding these things as scrollable when it shouldn't be, so figuring out why that's happening would be a good place to start.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

We'd really like to get this fixed as its a pretty bad experience. Kats would be the best assignee, but as he said in comment 15 he is otherwise tasked.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

OK, well that's enough to remove this from tracking consideration - constrained resources and all - but will put out a call for contributions and if there's a low risk fix at some point we can get it uplifted as high as possible.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this on desktop (Mac) on today's nightly.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #18)
> I can reproduce this on desktop (Mac) on today's nightly.

To be clear, this regressed recently on Mac, where as it regressed a few months ago on Android.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

nominating for tracking 29 because this regressed on Mac in that 29 (or perhaps 28, we noticed it in 29). Looking for a regression window to confirm.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Also moving this to Core->Graphics::Layers. Kats, is that the right place for core pan/zoom bugs?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

If it were an APZC issue then Core->Panning and Zooming would be the right component. But if it's happening on Mac (where APZ is disabled) then DOM/CSS/Layout seems more appropriate.

I looked at it quickly though to see if I could make an initial diagnosis. It looks like on Firefox, the body of the iframe containing the graphic is 645 pixels tall, and therefore scrollable because it exceeds the clientHeight of 389 pixels. On Chrome the body is 389 pixels tall. In both browsers the div just inside the body is 645 pixels tall, and I see no style set on the body element itself which would explain why it's shorter in Chrome than it is in Firefox. So actually to me this looks like a bug in Chrome, but I haven't checked other browsers to see what they do.

The only other thing I saw that might be relevant is that the <iframe> element itself has overflow-y: hidden set on it. I don't know what the specs say about setting overflow properties on iframe elements, and whether they're supposed to propagate down into the body of the subdocument. If that is the case then this is our bug (probably CSS). So I'm gonna move it there for people to look at.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> On Chrome the body is 389 pixels tall.

Is the document in the iframe in quirks mode, by any chance?

> I don't know what the specs say about setting overflow properties on iframe elements,

Nothing whatsoever that would affect the content.  We used to propagate them into the iframe until bug 943249 aligned us with the spec.  But this bug claims to be reproducible back to Firefox 25, so it's possible that something else is going on here instead of, or in addition to, the overflow propagation bits?  Or are people just mixing up multiple distinct issues in a single bug?

Comment 25

[Kartikaya Gupta (email:[e-mail])]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #24)
> > On Chrome the body is 389 pixels tall.
> 
> Is the document in the iframe in quirks mode, by any chance?

In Firefox on OS X the document in the iframe is in standards mode. (At least, document.compatMode returns "CSS1Compat").

> > I don't know what the specs say about setting overflow properties on iframe elements,
> 
> Nothing whatsoever that would affect the content.  We used to propagate them
> into the iframe until bug 943249 aligned us with the spec.  But this bug
> claims to be reproducible back to Firefox 25, so it's possible that
> something else is going on here instead of, or in addition to, the overflow
> propagation bits?  Or are people just mixing up multiple distinct issues in
> a single bug?

Yes, people are mixing up issues in a single bug. The bug was original filed for Fennec (and that's what the "affected" flags indicate) but then got silently repurposed for Mac OS X, which only regressed recently (see comment 18, but pending regressionwindow-wanted). It's quite likely that this was regressed by bug 943249 as the timeframe fits.

Based on this I think this is not a bug in our code but a tech evangelism issue, at least for Firefox on Desktop. The Fennec situation may be different since it was happening before bug 943249 landed.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

For the record, bz and I looked at this a bit further yesterday on IRC and discovered that the <div> inside the body of the iframe has a "zoom" property set on it, which accounts for the difference. We (I guess) don't support the zoom property, so they have a fallback that uses a scale transform instead. However zoom affects the layout height and the scale transform does not, which explains the difference in the height of the body between Chrome and Firefox.

Presumably if we support zoom this will problem will go away but otherwise it's a TE bug.

Comment 27

[Benjamin Kerensa [:bkerensa]]

Based on feedback in Comment 26 I don't think we need to track this.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Because this bug is confusing and the test case used to file this bug is actually a TE bug, I filed bug 988991 specifically for the original Fennec problem using Aaron's test case.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

And again, because this bug is confusing, I filed bug 989060 for the tech evangelism issue for the website in comment 0.

That covers all the issues in this bug so I'm going to close it out. If there are additional websites that are displaying this behaviour on Mac, please file another bug. If there are additional sites that display this behaviour on Fennec that are NOT fixed by the patch I put on bug 911574, then please file another bug.