Closed Bug 859683 Opened 7 years ago Closed 6 years ago

scrolling does not work in fullscreen mode

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: nb0000, Assigned: kats)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
OS: Windows 7 → Android
Hardware: x86_64 → ARM
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>
Status: UNCONFIRMED → NEW
Component: General → Graphics, Panning and Zooming
Ever confirmed: true
Version: Firefox 20 → Trunk
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.
One year later, bug is still there. Firefox for Android v.29
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
Waiting on a build, I might as well do this. Doesn't look too hard.
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8415454 - Flags: review?(wjohnston)
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)
Attachment #8415454 - Flags: review?(wjohnston) → review-
Attached patch Patch (v2) (obsolete) — Splinter Review
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 :)
Attachment #8415454 - Attachment is obsolete: true
Attachment #8416506 - Flags: review?(wjohnston)
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)?
Attachment #8416506 - Flags: review?(wjohnston) → review+
(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.
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.
Attached patch Patch (v3)Splinter Review
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
Attachment #8416506 - Attachment is obsolete: true
Attachment #8418828 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/865919c5d41a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.