775511 - Panning is possible while html5 videos are in fullscreen if a zoom in action was performed before

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Paul Feher]

Firefox 15.0b1 build2(2012-07-17)
Device: Asus Transformer TF101
OS: Android 4.0.2

Steps to reproduce:
1. Load http://people.mozilla.com/~nhirata/html_tp/elephants-dream.webm
2. Double tap on the video.
3. Long tap and select the fullscreen option.

Expected:
Panning should not be possible in fullscreen.

Actual:
The page is scrollable, the scroll bars are present.

Comment 1

[Cristian Nicolae (:xti)]

This issue is reproducing on phones too.

--
Device: Galaxy Nexus
OS: Android 4.1.1

Comment 2

[Cristian Nicolae (:xti)]

Different STR:

1. Disable plugins from Settings
2. Go to desktop version of YouTube
3. Find and play a HTML5 video
4. Zoom in and tap on Full Screen button
5. Double tap anywhere on the video side

Actual result: Video screen is moving down to the bottom-right corner. Sometimes panning is possible as well

--
Firefox 16.0 RC (2012-10-08)
Device: Galaxy Note
OS: Android 4.0.4

Comment 3

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

I'm able to reproduce slightly different behaviour on the GN in FF20:

- Load the URL in comment 0
- Pinch zoom in
- Long tap and full-screen
- Try panning

The page now has scrollbars that indicate panning even though the video itself doesn't visibly move. However if you pan all the way to the "end" of the page the video demonstrates overscroll behaviour. While this is less bad than actually being able to pan the video as described in comment #0, it's still an issue.

Comment 4

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

Attachment: (1/2) Move the full screen flag into LayerView

It doesn't really belong in GeckoApp, since it's not the activity that's full screen, it's the content area. This patch should have no real functional changes; it just moves stuff around.

Comment 5

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

Attachment: (2/2) Don't pan or show scrollbars in full screen mode

I verified this has the expected behaviour given the STR I posted above. I also tested full-screen behaviour on a non-media element like at http://people.mozilla.org/~kgupta/bug/775511.html and tested mbrubeck's full-screen addon. They all work the way I would expect (i.e., the non-media element fullscreening is not scrollable, just like on desktop, and mbrubeck's full-screen allows panning/zooming of the page but just hides the top chrome).

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 684789 [details] [diff] [review]
(1/2) Move the full screen flag into LayerView

Review of attachment 684789 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable.

::: mobile/android/base/GeckoApp.java
@@ +986,5 @@
>                  toggleChrome(true);
>              } else if (event.equals("ToggleChrome:Focus")) {
>                  focusChrome();
>              } else if (event.equals("DOMFullScreen:Start")) {
> +                LayerView layerView = mLayerView;

Maybe add a note that you're doing this assignment for thread safety? (unless this is already commented on above somewhere)

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 684790 [details] [diff] [review]
(2/2) Don't pan or show scrollbars in full screen mode

Review of attachment 684790 [details] [diff] [review]:
-----------------------------------------------------------------

Blocking events like that in PanZoomController really seems unsafe to me, what happens if you fullscreen an element larger than the screen - should this be scrollable? I don't know, but just mentioning in case it hasn't been tested. r+ as you say it works fine and it's a tiny change.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +458,2 @@
>                  // the viewport or page changed, so show the scrollbars again
> +                // as per UX decision. don't do this if we're in full-screen mode though.

Nit, capital "d" for "don't".

::: mobile/android/base/ui/PanZoomController.java
@@ +314,5 @@
>              // second tap. ignore the move if this happens.
>              return false;
>  
>          case TOUCHING:
> +            if (mTarget.isFullScreen() || panDistance(event) < PAN_THRESHOLD) {

Would be good to have a comment here documenting why it's ok to not pan when the target is fullscreen (as I suppose that fullscreen here doesn't actually map to all fullscreen situations?)

Comment 8

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

(In reply to Chris Lord [:cwiiis] from comment #6)
> Maybe add a note that you're doing this assignment for thread safety?
> (unless this is already commented on above somewhere)

Done. Also fixed s/mLayerView/layerView/ inside the body of the if condition :)

(In reply to Chris Lord [:cwiiis] from comment #7)
> Blocking events like that in PanZoomController really seems unsafe to me,
> what happens if you fullscreen an element larger than the screen - should
> this be scrollable?

In that case the element should not be scrollable (it isn't on desktop, as tested using the URL I posted in comment #5). This change makes it non-scrollable in Fennec as well.

> Nit, capital "d" for "don't".

Done.

> Would be good to have a comment here documenting why it's ok to not pan when
> the target is fullscreen (as I suppose that fullscreen here doesn't actually
> map to all fullscreen situations?)

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0208584908be
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c67034ba39c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0208584908be
https://hg.mozilla.org/mozilla-central/rev/3c67034ba39c

Comment 10

[Cristian Nicolae (:xti)]

I cannot reproduce this issue on the latest Nightly build. However, I can reproduce the issue mentioned in comment #2. Is it related to this one or should I file a new bug for it?

--
Firefox 20.0a1 (2012-11-26)
Device: Galaxy S2
OS: Android 4.0.3

Comment 11

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

Please file a new bug, and include a specific HTML5 video URL. Thanks.

Comment 12

[Cristian Nicolae (:xti)]

(In reply to Kartikaya Gupta (:kats) from comment #11)
> Please file a new bug, and include a specific HTML5 video URL. Thanks.

Sure. Thank you.

Closing bug as verified fixed.