918682 - HTML5 fullscreen does not work correctly if the video is zoomed before entering fullscreen

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Adrian Tamas (:AdrianT)]

Aurora 26.0a2 2013-09-19/Nightly 27.0a1 2013-09-19
Samsung Galaxy Tab 2 7.0 (Android 4.1.1)

Steps to reproduce:
1) Open a website with an HTML5 video
2) Pinch zoom in
3) Enter video full-screen mode

Expected results:
The video is displayed in fullscreen

Notes:
The issue is not reproducible on Firefox Mobile 25 beta 1

Actual results:
The video is not correctly displayed in fullscreen.

Comment 1

[Aaron Train [:aaronmt]]

Please provide a regression-window.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Regression window-wanted:

1.mc:
not affected build: 05-09-2013
affected build: 06-09-2013  
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=77ed46bf4c1a&tochange=ab5f29823236

2. inbound:
not affected build: 1378328537 
affected build : 1378331957
-pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ccebf05488c&tochange=5a86fde943e6

Comment 3

[Aaron Train [:aaronmt]]

Botond & Kats looks like bug 904533?

Comment 4

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

Can you provide a screenshot of what you're seeing? When I follow the steps in comment 0 with the given URL I just get a full-screen video that hasn't started playing. I can play/pause and it looks fine. Un-fullscreening, zooming around the page, and fullscreening again all work as expected.

Tested on a Nexus 4 with the latest nightly.

Comment 5

[Teodora Vermesan (:TeoVermesan)]

Using LG Nexus 4 (Android 4.2.2):
If you zoom and move the video to the left/right side of the page and fullscreen it, then the video is not  displayed correctly. If you put the video in the middle of the page and fullscreen it, then the video is displayed ok. 
Please look at the following video: http://www.youtube.com/watch?v=7vcimBNFVh4

Comment 6

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

We already have a regression window in comment 2.

Comment 7

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

This was definitely introduced by bug 904533. The problem now (which I thought bug 914825 was supposed to fix, but I guess doesn't fully fix) is that the composition bounds are larger than the widget bounds, so when the video goes "fullscreen" it gets sized to the widget bounds but the viewport is clamped to the composition bounds. This means the viewport is not moved to the top-left of the video, but is allowed to remain offset from the video top-left, as long as it remains within the composition bounds. This results in the observed behaviour.

Comment 8

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

Attachment: Patch

I'm not sure why the aForFrame->GetParent() null check was added originally (it appeared in the first version of Botond's patch on bug 914825 and there's no discussion on it) but in Fennec's case the root content document does have a parent frame, so that check is failing when I think we want it to pass. This patch fixes the bug on Fennec. I haven't tested anything on B2G to ensure it doesn't re-break anything there.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=4ed25222fca0

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Created attachment 810967 [details] [diff] [review]
> Patch
> 
> I'm not sure why the aForFrame->GetParent() null check was added originally
> (it appeared in the first version of Botond's patch on bug 914825 and
> there's no discussion on it) but in Fennec's case the root content document
> does have a parent frame, so that check is failing when I think we want it
> to pass.

We added it because we only wanted it to apply to the root scroll frame of the root content document (and not just any scroll frame in the root content document).

The root content document may have a parent document, but the root frame of any document should return null from GetParent (it's cross doc parent as retrieved from nsLayoutUtils::GetCrossDocParentFrame may not be null though).

Comment 10

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

So in this case aForFrame is this:

    Canvas(html)(-1)@0x735d18d8 {-9800,-12097,58800,79472} [state=0000006000000210] [content=0x89470e20] [sc=0x735d2458:-moz-scrolled-canvas]

and aForFrame->GetParent() is this:

  HTMLScroll(html)(-1)@0x735d1ab0 {0,0,58800,79472} [state=0000020000000010] [content=0x89470e20] [sc=0x735d14a8:-moz-viewport-scroll]

There's also a Viewport frame above the HTMLScroll. Is there a better check we can do instead of aForFrame->GetParent() == nullptr to cover this scenario?

Comment 11

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

Attachment: Debugging dump

On the page at cd.pn/b RecordFrameMetrics is called on two different frames. The first is the Canvas(html) of the root content doc and the second is the rootmost viewport frame of a doc that is not the root content doc (presumably the XUL document). Debug output attached.

Comment 12

[Timothy Nikkel (:tnikkel)]

Ah, ok, that makes sense. We probably want to check that aScrollFrame is the root scroll frame of the presshell instead of the parent is null then. Thanks for doing that debugging.

Comment 13

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

Attachment: Patch v2

That works, thanks!

Comment 14

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

https://hg.mozilla.org/integration/fx-team/rev/e69483402ee3

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e69483402ee3

Comment 16

[u421692]

Verified as fixed on:
Device: Samsung Galaxy Tab (Android 4.0.4)
Build: Nightly 27.0a1 (2013-09-29)

Comment 17

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

Comment on attachment 811299 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 904533 and friends
User impact if declined: more or less what the bug title says
Testing completed (on m-c, etc.): on m-c for a while now, verified
Risk to taking this patch (and alternatives if risky): medium risk. the code is known to be hard to get right, and this potentially touches all platforms
String or IDL/UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bd10c46501f