865298 - Dynamic toolbar refactor fix-ups

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P1)

Description

[Chris Lord [:cwiiis]]

Sorry for the vague title, but a full title was proving difficult;

The refactor has changed the ownership of various properties, and some old behaviour was not reinstated.

These were:

- The toolbar previously would stay hidden after being hid until you lifted your finger
- The toolbar would remain pinned if the page was shorter than the screen

The second I plan on reinstating as is, the first I would like to do something a little better that the new encapsulation makes much easier.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Allow multiple touch interceptors on LayerView

To encapsulate this nicely, the LayerMarginsAnimator needs to be able to listen to touch events. This patch lets us register multiple interceptors. All interceptors are called, but any interceptor swallowing the event will cause the event not to propagate.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Introduce an 'active' area for exposing margins

This stops margins from being exposed unless you're pulling from near that margin, or the viewport is at the edge of the page. This is the behaviour that was specified quite a long time ago by ibarlow but I never got round to... Much easier to implement now :)

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Don't scroll margins on small pages

This restores the previous behaviour that the margins remain pinned on pages that are smaller than the screen.

Comment 4

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

Comment on attachment 742343 [details] [diff] [review]
Part 1 - Allow multiple touch interceptors on LayerView

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

::: mobile/android/base/gfx/LayerView.java
@@ +60,5 @@
>      private SurfaceView mSurfaceView;
>      private TextureView mTextureView;
>  
>      private Listener mListener;
> +    private ArrayList<TouchEventInterceptor> mTouchInterceptors;

Make this final and add a comment here that it should only be touched on the java UI thread

@@ +138,3 @@
>          // this gets run on the gecko thread, but for thread safety we want the assignment
>          // on the UI thread.
>          post(new Runnable() {

Remove this comment (it will be replaced by the one at the mTouchIntercepters declaration)

Comment 5

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

Comment on attachment 742344 [details] [diff] [review]
Part 2 - Introduce an 'active' area for exposing margins

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

Looks ok but i'd like to see a refactored version.

::: mobile/android/base/gfx/LayerMarginsAnimator.java
@@ +49,5 @@
>          // Assign member variables from parameters
>          mTarget = aTarget;
>  
> +        // Listen to touch events, for auto-pinning
> +        aView.addTouchInterceptor(this);

I'd rather this happened at the bottom of the constructor for more robustness.

@@ +173,5 @@
>                // Scrolling right.
>                float marginDx = Math.max(0, aDx - overscroll.left);
>                newMarginLeft = aMetrics.marginLeft - Math.min(marginDx, aMetrics.marginLeft);
> +              if (mTouchStartPosition.x < aMetrics.getWidth() - activeAreaX) {
> +                  // In the situation that the touch started outside of the

These if conditions are getting pretty big on duplicated code. I'd like to see this code refactored into a helper function instead of repeated 4 times. You might need some ugly code to deal with the multiple return values (e.g. newMarginLeft and newMarginRight both need updating) but I still think it will be better with the code refactored.

@@ +225,5 @@
>          return aMetrics.setMargins(newMarginLeft, newMarginTop, newMarginRight, newMarginBottom).offsetViewportBy(aDx, aDy);
>      }
> +
> +    /** Implementation of TouchEventInterceptor */
> +    public boolean onTouch(View view, MotionEvent event) {

Add @Override

@@ +230,5 @@
> +        return false;
> +    }
> +
> +    /** Implementation of TouchEventInterceptor */
> +    public boolean onInterceptTouchEvent(View view, MotionEvent event) {

Add @Override

Comment 6

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

Comment on attachment 742346 [details] [diff] [review]
Don't scroll margins on small pages

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

I assume the ignore-whitespace diff on this is tiny; I didn't do a line-by-line comparison.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Introduce an 'active' area for exposing margins

As suggested.

I've still got a separate case for negative/positive scrolling, but the axes are now unified. Removing the dimensions I think makes the code a bit more understandable, as of course does the lack of duplication :)

Comment 8

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

Comment on attachment 742815 [details] [diff] [review]
Part 2 - Introduce an 'active' area for exposing margins

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

LGTM, thanks.

Comment 9

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a193912cf7f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee172f363a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df1b378f017a

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/0a193912cf7f
https://hg.mozilla.org/mozilla-central/rev/6ee172f363a8
https://hg.mozilla.org/mozilla-central/rev/df1b378f017a

Comment 11

[Mathieu Pellerin]

since the m-i merge that included this fix, scrolling upwards fails to show toolbar. hope this is an unforseen bug rather than a design decision as the scroll up for toolbar was a nice behaviour.

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Mathieu Pellerin from comment #11)
> since the m-i merge that included this fix, scrolling upwards fails to show
> toolbar. hope this is an unforseen bug rather than a design decision as the
> scroll up for toolbar was a nice behaviour.

This is a design decision - to show the toolbar, scroll up from the top quarter of the screen, or scroll to the top of the page.

Comment 13

[Mathieu Pellerin]

I see. Clever refinement although a bit hard to discover though. :)

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Mathieu Pellerin from comment #13)
> I see. Clever refinement although a bit hard to discover though. :)

A bit, perhaps :) The idea is that you might think that you have to scroll all the way to the top, and at some point, you'll accidentally expose the toolbar and realise how it works (it's also similar to how it works on stock Android, though Chrome's recent adoption of the feature lets you pull from anywhere)

Perhaps the active area could be adjusted to make it more likely to accidentally expose the toolbar - if it feels like an issue, file a bug and cc me and :ibarlow for UX comment.

Comment 15

[Mathieu Pellerin]

Chris, what's the argument for having only the top quarter activating toolbar when scrolling up? I find it quite natural to see toolbar when scrolling upwards, no matter where my finger starts. People read downwards, so when moving upwards, the lost of space taken by toolbar doesn't really impact on readability.

Comment 16

[Chris Lord [:cwiiis]]

(In reply to Mathieu Pellerin from comment #15)
> Chris, what's the argument for having only the top quarter activating
> toolbar when scrolling up? I find it quite natural to see toolbar when
> scrolling upwards, no matter where my finger starts. People read downwards,
> so when moving upwards, the lost of space taken by toolbar doesn't really
> impact on readability.

We found that when you scroll down to focus a paragraph, you often scroll back up a little bit because you overshoot - usually enough to expose the toolbar again, which is annoying. Similarly, if you were linked to a particular anchor in a page and wanted to scroll upwards, you're left with the toolbar on the screen.

At least doing it like this, you have an option - the top quarter is still quite a large area, and if you're scrolling to expose the toolbar, that's a conscious decision so it's not much of a leap to have it only appear from a particular area.

Persionally, I like it because I can now scroll around the page in fullscreen at all times now, and it's pretty rare that I use the toolbar. That said, I'm open to suggestion - cc'ing :ibarlow for comment.