Closed Bug 960159 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Reader View, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: abc, Assigned: jdover)

Details

Attachments

(2 files, 2 obsolete files)

When entering full screen mode in Reader mode, the three touch buttons (back/home/switcher) should transform into dots.
I have no idea how/if we can do this.
Assignee: nobody → jdover
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?
Attached image Screenshot
Dimmed statusbar and navigation bar buttons (back, home, switcher). They dim when you enter reader mode and return to normal on exiting.
Attachment #8361355 - Flags: feedback?(abc)
Status: NEW → ASSIGNED
@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.
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
Attachment #8361838 - Flags: review?(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.
Attachment #8361838 - Flags: review?(margaret.leibovic) → feedback?(wjohnston)
Comment on attachment 8361838 [details] [diff] [review]
Bug 960159: Dim System UI when scrolling down in reader mode

Actually, make that lucasr :)
Attachment #8361838 - Flags: feedback?(wjohnston) → feedback?(lucasr.at.mozilla)
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
Attachment #8361838 - Flags: feedback?(lucasr.at.mozilla) → feedback-
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.
Attachment #8363876 - Flags: review?(lucasr.at.mozilla)
Attachment #8363876 - Flags: feedback?(abc)
Attachment #8363876 - Attachment is obsolete: true
Attachment #8363876 - Flags: review?(lucasr.at.mozilla)
Attachment #8363876 - Flags: feedback?(abc)
Attachment #8361838 - Attachment is obsolete: true
Comment on attachment 8363876 [details] [diff] [review]
Dim System UI when scrolling down in reader mode

Whoops marked the wrong patch obsolete...
Attachment #8363876 - Attachment is obsolete: false
Attachment #8363876 - Flags: review?(lucasr.at.mozilla)
Attachment #8363876 - Flags: feedback?(abc)
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?
Attachment #8363876 - Flags: review?(lucasr.at.mozilla) → feedback+
(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.
Attachment #8363876 - Attachment is obsolete: true
Attachment #8363876 - Flags: feedback?(abc)
Attachment #8364511 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8364511 - Flags: review?(lucasr.at.mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f6273498c0b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.