859683 - scrolling does not work in fullscreen mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[dragon]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.43 Safari/537.31

Steps to reproduce:

Made a website which uses javascript to enter fullscreen mode, and realized that scrolling does not work. Did a little research on the theme, and found a test page:
http://people.mozilla.org/~cpearce/synth-fullscreen.html
(and a related bug: 779286).


Actual results:

Scrolling did not work in fullscreen mode.


Expected results:

The content should have scrolled.

Comment 1

[Aaron Train [:aaronmt]]

I was able to reproduce on trunk (mozilla-central, 04/09).

In console I see: I/GeckoSubdocumentScrollHelper( 3563): Got message: Panning:Override on attempts to scroll the content in the <div>

Steps to reproduce, on the test-URL: http://people.mozilla.org/~cpearce/synth-fullscreen.html, click the third button to enter full-screen and then attempt to scroll the <div>

Comment 2

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

By "third button" do you mean the "container" button? Because that doesn't work on desktop either. Only the last button (document.documentElement) fullscreen scrolling works on desktop.

Comment 3

[fl.devol]

One year later, bug is still there. Firefox for Android v.29

Comment 4

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

According to https://bugzilla.mozilla.org/show_bug.cgi?id=779286#c28 only if document.documentElement is fullscreened should it be scrollable. This is the 5th button on the test page. I am able to scroll in desktop but not on Fennec, so this is indeed a Fennec bug. All the other buttons behave the same way on Fennec as they do on desktop.

I did some quick testing in nightly and when the document.documentElement goes fullscreen, the document.body has is scrollable (in that it has overflow:visible and a nonzero scrollTopMax), and the document.compatMode is "BackCompat", so the document.body is the root scrolling element. This means we should be scrolling it async from Java land. I also verified that getRootBounds() on the window returns a large size, so the Java code should be detecting it as scrollable as well. The only hitch is the fullscreen-specific check at [1] which was added in bug 775511 to prevent scrolling in fullscreen mode.

We might need to make that check a little smarter and distinguish between fullscreening the document element and fullscreening any other element.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=8f33ef762433#479

Comment 5

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

Waiting on a build, I might as well do this. Doesn't look too hard.

Comment 6

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

Attachment: Patch

There's a existing problem with the JS code sending multiple DOM:FullScreen messages to Java (one of the XUL doc and one or more for the content doc) but I couldn't see an easy way to get rid of that and still fix this bug. Might be worth a follow-up if you really want to trim away those extra messages.

Comment 7

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

Also try push: https://tbpl.mozilla.org/?tree=Try&rev=ee1347793826

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8415454 [details] [diff] [review]
Patch

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

I'm making up names as I go. Maybe you have better ones?

::: mobile/android/base/gfx/LayerView.java
@@ +688,5 @@
>          super.onFocusChanged(gainFocus, direction, previouslyFocusedRect);
>          GeckoAccessibility.onLayerViewFocusChanged(this, gainFocus);
>      }
>  
> +    public void setFullScreen(boolean fullScreen, boolean rootElement) {

I think I'd rather pass an enum:

enum ElementType {
  ROOT,
  NORMAL,
  NONE
}

@@ +693,2 @@
>          mFullScreen = fullScreen;
> +        mFullScreenRoot = rootElement;

And then I'd store the elementType

@@ +697,5 @@
>      public boolean isFullScreen() {
>          return mFullScreen;
>      }
>  
> +    public boolean isFullScreen(boolean rootElement) {

And then we can add a getFullscreenElementType() function here to return the elementType.

When we query these then, Instead of:

if (mTarget.isFullScreen(false))

we can do:

if (mTarget.isFullScreen(false) && mTarget.getFullscreenElementType() != ElementType.ROOT)

Comment 9

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

Attachment: Patch (v2)

Went with a variation of what you said. Thanks for minusing the first patch, I didn't realize how unreadable it was until I had to convert it to this new version :)

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8416506 [details] [diff] [review]
Patch (v2)

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

Heh. I thought about this right after I posted it too. Looks better.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +1181,5 @@
>      }
>  
>      @Override
>      public boolean onScale(SimpleScaleGestureDetector detector) {
> +        if (mTarget.getFullScreenState() == FullScreenState.ROOT_ELEMENT)

I think we want to do this regardless of the element (i.e. != FullScreenState.NONE)?

Comment 11

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

(In reply to Wesley Johnston (:wesj) from comment #10)
> I think we want to do this regardless of the element (i.e. !=
> FullScreenState.NONE)?

You're right. At least that's how Chrome behaves on the test page. I'll update the patch and land it.

Comment 12

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

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/1b39c3a55620

https://tbpl.mozilla.org/php/getParsedLog.php?id=39148902&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=39147501&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=39148015&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=39148071&tree=Fx-Team

Comment 14

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

Sigh. Looks like my streak of bouncing patches continues. There shouldn't really be any functional changes between the version that landed and the try push I did in comment 7. At least none that get exercised in the tests.

Comment 15

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

Attachment: Patch (v3)

I forgot to initialize mFullScreenState in LayerView, so isFullScreen() was returning true initially. Initializing it to FullScreenState.NONE fixed the tests for me locally. Updated patch, carrying r+ because it was a stupid simple fix. New try push at https://tbpl.mozilla.org/?tree=Try&rev=2b69ce1aadc6

Comment 16

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

Above try run is about as green as things are right now, so landed on fx-team:

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

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/865919c5d41a