Closed Bug 849847 Opened 8 years ago Closed 8 years ago

Make about:home scrollable with the analog stick

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: kats, Assigned: bnicholson)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → bnicholson
This also fixes a bug in BrowserApp where the focus logic for mLayerView/mAboutHome is reversed.
Attachment #744382 - Flags: review?(chrislord.net)
Comment on attachment 744382 [details] [diff] [review]
Make about:home scrollable with the analog stick

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

I'd like to see this factored out into a separate class - it might also be nice to factor out frame animation into a separate class, but perhaps that ought to be a separate bug (like bug 711959).

::: mobile/android/base/BrowserApp.java
@@ +356,3 @@
>                          mAboutHome.requestFocus();
> +                    } else {
> +                        mLayerView.requestFocus();

Nice catch :)

::: mobile/android/base/widget/AboutHomeView.java
@@ +18,5 @@
>  public class AboutHomeView extends ScrollView implements LightweightTheme.OnChangeListener {
>      private LightweightTheme mLightweightTheme;
> +    private Timer mScrollTimer;
> +    static final long MS_PER_FRAME = 1000 / 60;
> +    static final float SCROLL_FACTOR = 0.075f;

This needs to be documented.

@@ +63,5 @@
>          super.onLayout(changed, l, t, r, b);
>          onLightweightThemeChanged();
>      }
> +
> +    private class ScrollRunnable extends TimerTask {

I think we should factor out all of these changes into a separate reusable class that we can use on any widget, rather than just about:home (e.g. tabs switcher, awesome-screen)

@@ +83,5 @@
> +                    return true;
> +                }
> +
> +                mY = (int) (event.getAxisValue(MotionEvent.AXIS_Y) *
> +                        (SCROLL_FACTOR * GeckoAppShell.getDpi()));

It'd be nice to document what this statement is doing. Something like 'Scroll with a speed relative to the screen DPI'.
Attachment #744382 - Flags: review?(chrislord.net) → review-
Added comments and factored out scrolling logic.
Attachment #744382 - Attachment is obsolete: true
Attachment #744877 - Flags: review?(chrislord.net)
Comment on attachment 744877 [details] [diff] [review]
Make about:home scrollable with the analog stick, v2

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

Nice :)
Attachment #744877 - Flags: review?(chrislord.net) → review+
Looks like the bustage was caused by using View.OnGenericMotionListener on < API 12. 
ScrollAnimator implements this interface, so I added an API guard where it's instantiated/used.

Try: https://tbpl.mozilla.org/?tree=Try&rev=931156d4482e
https://hg.mozilla.org/mozilla-central/rev/4491b40b0fe0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.