960159 - Change back/home/switcher controls to ambient dots in Reader mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Arun Balachandran Ganesan [:abc]]

When entering full screen mode in Reader mode, the three touch buttons (back/home/switcher) should transform into dots.

Comment 1

[Wesley Johnston (:wesj)]

I have no idea how/if we can do this.

Comment 2

[Wesley Johnston (:wesj)]

Talked to abc about this a bit. Its fullscreen thing:
http://marketingland.com/wp-content/ml-loads/2011/12/buttons-300x236.jpg

that went away with the prettier fullscreen in KitKat:
https://www.dropbox.com/s/fkxojg15sgyald6/Screen%20Shot%202014-01-15%20at%204.43.27%20PM.png
https://www.dropbox.com/s/91tmmtnxsqgb0or/2014-01-16%2000.48.10.png

It would be interesting to play with both these interactions in reader and see if we can improve the webapis somehow. i.e. Can reader detect swipes at the top and turn off fullscreen? If it does, can we make the transition nice? Can/should reader pop into fullscreen when it launches? Can/should it pop back into fullscreen when we hide the toolbars?

Comment 3

[Josh Dover [:jdover]]

Attachment: Screenshot

Dimmed statusbar and navigation bar buttons (back, home, switcher). They dim when you enter reader mode and return to normal on exiting.

Comment 4

[Arun Balachandran Ganesan [:abc]]

@Josh: As wesj summarized above, we are considering two options here now.

Initial thoughts (will circle back with more thoughts soon)

1. Dimming the system bars
https://developer.android.com/training/system-ui/dim.html

Straightforward interaction here. On scrolling down, buttons are dimmed and scroll back makes them normal again.

2. Immersive Full-screen mode
https://developer.android.com/training/system-ui/immersive.html

Introduced with Kitkat, this allows navigation bar and the status bar to slide away along the edges when an app enters full screen mode to make for an immersive experience. Good, so far. Bringing back the bar is described as follows:

"To return to immersive mode, users can touch the screen outside of the bar bounds or wait for a short period for the bars to auto-hide. For a consistent user experience, the new gesture also works with previous methods of hiding the status bar."

----

1. Can we have the bars reappear (along with the URL bar) when users scroll back in addition to what's described?

2. Does the positives (improved reading experience with larger screen estate and clutter free reading) justify the cost (having to exit full screen mode to access navigation bar)? 

3. As a aside, I & ibarlow have also briefly discussed having reader mode controls appear & disappear along with the URL bar while scrolling instead of having to tap center to make them appear/disappear. Will post an update soon.

Comment 5

[Josh Dover [:jdover]]

Attachment: Bug 960159: Dim System UI when scrolling down in reader mode

Dims system UI when scrolling down in reader mode, un-dims when scrolling back up. Having issues with building Fennec when targeting API 19 (KitKat), so using Immersive mode with have to happen in another patch: bug 961148.

@margaret if there is a better place to put this scrolling code, let me know.

https://drive.google.com/file/d/0BwqqgfwYmRg4RWpFZ1E1WHZHaFk/edit?usp=sharing

Comment 6

[:Margaret Leibovic]

Comment on attachment 8361838 [details] [diff] [review]
Bug 960159: Dim System UI when scrolling down in reader mode

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

Clearing review, since I think we need to think more about where we're going to put this code. Also flagging wesj for feedback in case he has the time :)

::: mobile/android/base/BrowserApp.java
@@ +977,5 @@
>      }
>  
> +    @Override
> +    public void onScrollDown(boolean down) {
> +        boolean readerMode = AboutPages.isAboutReader(Tabs.getInstance().getSelectedTab().getURL());

So, we're going to end up doing this check every time and page is scrolled vertically... that seems sub-optimal. Is there any way we can just listen for scroll events in the reader mode JS code, then send a message to Java to do this system UI change when appropriate?

@@ +986,5 @@
> +            } else {
> +                mLayerView.setSystemUiVisibility(View.SYSTEM_UI_FLAG_VISIBLE);
> +            }
> +        }
> +    }

I hate how everything ends up in BrowserApp, but I guess there's not a better place for this right now :/

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +889,5 @@
>  
>      public interface OnMetricsChangedListener {
>          public void onMetricsChanged(ImmutableViewportMetrics viewport);
>          public void onPanZoomStopped();
> +        public void onScrollDown(boolean down);

This seems like an odd API... if we do go this route, maybe this should just be onScroll(dx, dy), then the consumer can check dy to see if it's up or down.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8361838 [details] [diff] [review]
Bug 960159: Dim System UI when scrolling down in reader mode

Actually, make that lucasr :)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8361838 [details] [diff] [review]
Bug 960159: Dim System UI when scrolling down in reader mode

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

Thanks for the patch, Joshua. Unfortunately, this patch adds an unnecessary overhead whenever the user scrolls any page because you're doing this reader page check on every scroll event. I'd prefer a reader-specific solution where the Reader UI explicitly requests the app to turn the system UI on and off according to its state. So, in practice, I'd go with a SystemUI:On/SystemUI:Off kind of Gecko message that is sent whenever the Reader UI hides/shows its toolbar. Somewhere around here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#488

Comment 9

[Josh Dover [:jdover]]

Attachment: Dim System UI when scrolling down in reader mode

Listens for the scroll event in aboutReader.js and only sends message to Java if the direction has changed. Also reveals the reader toolbar when scrolling up per Arun's (:abc) request.

Comment 10

[Josh Dover [:jdover]]

Comment on attachment 8363876 [details] [diff] [review]
Dim System UI when scrolling down in reader mode

Whoops marked the wrong patch obsolete...

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8363876 [details] [diff] [review]
Dim System UI when scrolling down in reader mode

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

Looks nice, just needs some final tweaks.

::: mobile/android/base/GeckoApp.java
@@ +2819,5 @@
>              }
>          });
>      }
> +
> +    private void showSystemUI(final boolean visible) {

showSystemUI -> setSystemUIVisible() or setSystemUIVisibility()

::: mobile/android/chrome/content/aboutReader.js
@@ +265,5 @@
>            this._toggleToolbarVisibility();
>          break;
>        case "scroll":
>          if (!this._scrolled) {
> +          let up = this._scrollOffset > aEvent.pageY;

up -> isScrollingUp for clarity

@@ +267,5 @@
>        case "scroll":
>          if (!this._scrolled) {
> +          let up = this._scrollOffset > aEvent.pageY;
> +          this._setToolbarVisibility(up);
> +          this._scrollOffset = aEvent.pageY;

Isn't this 'scrollOffset' more like a 'lastScrollY' or something?

Comment 12

[Josh Dover [:jdover]]

Attachment: Dim System UI when scrolling down in reader mode

(In reply to Lucas Rocha (:lucasr) from comment #11)
> showSystemUI -> setSystemUIVisible() or setSystemUIVisibility()

Used setSystemUIVisible to differentiate from View's setSystemUIVisibility which takes a constant.

> up -> isScrollingUp for clarity

Done.

> @@ +267,5 @@
> >        case "scroll":
> >          if (!this._scrolled) {
> > +          let up = this._scrollOffset > aEvent.pageY;
> > +          this._setToolbarVisibility(up);
> > +          this._scrollOffset = aEvent.pageY;
> 
> Isn't this 'scrollOffset' more like a 'lastScrollY' or something?

'aEvent.pageY' is the total offset from the top of the page, so I don't think 'lastScrollY' makes sense. There was a variable called '_scrollOffset' already being initialized, but never used and the name made sense here.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8364511 [details] [diff] [review]
Dim System UI when scrolling down in reader mode

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

Nice, thanks.

(In reply to Josh Dover from comment #12)
> Created attachment 8364511 [details] [diff] [review]
> Dim System UI when scrolling down in reader mode
> 
> (In reply to Lucas Rocha (:lucasr) from comment #11)
> > showSystemUI -> setSystemUIVisible() or setSystemUIVisibility()
> 
> Used setSystemUIVisible to differentiate from View's setSystemUIVisibility
> which takes a constant.

Good point.

> > up -> isScrollingUp for clarity
> 
> Done.
> 
> > @@ +267,5 @@
> > >        case "scroll":
> > >          if (!this._scrolled) {
> > > +          let up = this._scrollOffset > aEvent.pageY;
> > > +          this._setToolbarVisibility(up);
> > > +          this._scrollOffset = aEvent.pageY;
> > 
> > Isn't this 'scrollOffset' more like a 'lastScrollY' or something?
> 
> 'aEvent.pageY' is the total offset from the top of the page, so I don't
> think 'lastScrollY' makes sense. There was a variable called '_scrollOffset'
> already being initialized, but never used and the name made sense here.

Fair enough.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f6273498c0b