Bug 862801 (home-banner)

Create Java UI for home banner

RESOLVED FIXED in Firefox 26

Status

()

P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ibarlow, Assigned: sriram)

Tracking

(Depends on: 3 bugs)

unspecified
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec26+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
This is the bug to track work on the new snippet design and placement on about:home. The snippet will be pulled out of page content and instead exist as a footer bar that hides when the user starts scrolling. 

The footer used in the Android Google Search UI is a good example of this kind of interaction. 

Mockups to follow shortly.
(Assignee)

Updated

6 years ago
Assignee: nobody → sriram
(Reporter)

Comment 1

6 years ago
Marking this as P1, as I want to make sure we don't lose this in the move from our current about:home to new about:home. 

Wireframe from initial spec -- http://cl.ly/image/3j0i1w2x1s1x

I can follow up with visual designs soon.
Priority: -- → P1
(Reporter)

Comment 2

6 years ago
OMG. Ship it ship it ship it <3
(Reporter)

Comment 3

6 years ago
Oh whoops. That was meant for another bug. However I am still enthusiastic about the snippet too :P
(Assignee)

Comment 4

6 years ago
Posted patch WIP (obsolete) — Splinter Review
I tried a small patch to get the basic swiping working. There are few issues compared to what google search does.

Basically their layout is like
<FrameLayout>
  <CoScrollContainer>
     <WebView>
  </CoScrollContainer>
  <SearchBox />
  <BottomBar />
</FrameLayout>

From my assumptions on how it works:
1. The CoScrollContainer intercepts all touch events to the webview. And since the web view is its only child, it also passes down the touch event to the webview directly. (Note: CoScrollContainer is some dummy container).
2. SearchBox and BottomBar get to attach to CoScrollContainer to get the touch events.
Everyone is happy.

In our case:
<FrameLayout>
   <ViewPager>
      <FrameLayout>
          <ListView />
      <FrameLayout>
      <FrameLayout>
           <TabHost>
              <ListView />
           </TabHost>
      <FramLayout>
   </ViewPager>
   <Snippet />
</FrameLayout>

The issues here are:
1. ViewPager can start intercepting the events, but return false. This way it would get all the events.
2. But (1) fails when the ListView starts scrolling. At the point of first ACTION_DOWN on list view, it starts intercepting the event by returning true. So, ViewPager doesn't get anymore events! So, ViewPager cannot intercept events directly.
3. Every page is a fragment. So we need some kind of mechanism to attach a listener to the ListViews only when there they are attached.
4. The outermost FrameLayout should know if a ListView is scrolling and pass it down to the Snippet.

This approach is not cleaner (as the patch shows :P).
Is there any other way to handle this?

I tried the other approach ViewPager intercepting all events by returning true. But in this case, it needs to find the view that can handle the event and pass it down to that event. This approach would fail if we try to find the View right under the finger -- as the View would be some TextView in a row in a ListView and not the ListView itself. (This problem is what mitigated by android by intercepting the events in ListView by default).
Attachment #787095 - Flags: feedback?(lucasr.at.mozilla)
(Assignee)

Comment 5

6 years ago
Posted patch WIP 2: Use ViewTreeObserver (obsolete) — Splinter Review
This is same as previous patch in that, we need to attach a touch listener to the list-view and pass it to the snippet.

The changes are:
1. HomePager has a ViewTreeObserver. That way, whenever any view is added to it, it can query for a HomeListView and attach a touch listener. This is less error-prone that doing at fragment level.
2. HomeSnippet mentions the id of the view to attach to. There is something weird on in that attach. May be I should expose an interface like, "CoScrollView", and make HomePager implement it, and HomeSnippet attach to that "CoScrollView"'s listener. (I was a bit bored to do it :P).

Other than these weird techniques, I can't think of any.

Roman Nurik uses a similar approach: https://plus.google.com/+RomanNurik/posts/1Sb549FvpJt
Attachment #787796 - Flags: feedback?(lucasr.at.mozilla)
I think I need design clarification before giving feedback on this patches.

Ian, how does the swiping to hide the snippet interact with the lists in about:home? Does the snippet stay on top of about:home independently of the selected page? If that's the case, how does this snippet interact with the bottom tabs in the History page?
Flags: needinfo?(ibarlow)
Comment on attachment 787095 [details] [diff] [review]
WIP

Clearing f? flag until I get a design input from ibarlow.
Attachment #787095 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 787796 [details] [diff] [review]
WIP 2: Use ViewTreeObserver

Clearing f? flag until I get a design input from ibarlow.
Attachment #787796 - Flags: feedback?(lucasr.at.mozilla)
(Reporter)

Comment 9

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #6)
> I think I need design clarification before giving feedback on this patches.
> 
> Ian, how does the swiping to hide the snippet interact with the lists in
> about:home? Does the snippet stay on top of about:home independently of the
> selected page? If that's the case, how does this snippet interact with the
> bottom tabs in the History page?

To avoid overlapping other about:home UI (like the tabs on the History panel), we should only show the snippet on the Bookmarks panel. The snippet will still have a great deal of visibility since the Bookmarks panel is the default panel that is shown on startup and whenever a new tab is created.

Scrolling down the page hides the snippet from view, and scrolling up reveals it. We should also provide a small "x" close button to hide it from view for the user's current session.
Flags: needinfo?(ibarlow)

Updated

6 years ago
Blocks: 905262
(Assignee)

Comment 10

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Ian is happy with this behavior. I am requesting feedback for the approach I've used here. This patch is not complete. The XML has some random text. The designs aren't final too.

The thing I want to finalize before proceeding further is "how to take the scrolling from the HostListView and pass it to the HomeSnippet". In my approach, the HomeSnippet can attach to a CoScrollContainer by specifying an id in the layout file. Once it attaches to the window, it tries to find such a container and attach to it. The purpose of this container is to pass the touch event from a HomeListView to this HomeSnippet. This is done by attaching a ViewTreeObserver to this container. Since we use fragment and a HomeListView can be added anytime, the ViewTreeObserver does the job of piping the onTouchEvent from the HomeListView to the HomeSnippet. To mitigate the issue of "history tabs shouldn't allow snippet", I have introduced an attribute to HomeListView by which it can convey if it wants to pass the touch events to something else or not. This is configurable in the layout file.

Things to do:
1. Change the UI based on the design.
2. Show/hide the home_container (or just the snippet) base on the page shown.
Attachment #787095 - Attachment is obsolete: true
Attachment #787796 - Attachment is obsolete: true
Attachment #791153 - Flags: feedback?(lucasr.at.mozilla)
(Assignee)

Comment 11

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Ooooh.. sooo cleaaan! :)
Restricting to Bookmarks alone made things feel a lot lot better.
Attachment #791384 - Flags: feedback?(lucasr.at.mozilla)
(Assignee)

Updated

6 years ago
Attachment #791153 - Attachment is obsolete: true
Attachment #791153 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 791384 [details] [diff] [review]
Patch

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

Looks fine. The only change I'd like to see is: the code to slide the snippet in/out doesn't really belong to HomeSnippet imo. This is an implementation detail of how snippets are presented specifically in the BookmarksPage. So, remove the OnCoScrollListener entirely and handle the translation bits directly in BookmarksPage. HomeSnippet should only have to deal with its inner logic and views.

::: mobile/android/base/home/HomeSnippet.java
@@ +60,5 @@
> +
> +                mEvent = MotionEvent.obtain(event);
> +                break;
> +
> +            case MotionEvent.ACTION_UP:

You probably need to handle ACTION_CANCEL too.
Attachment #791384 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 13

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 791384 [details] [diff] [review]
> Patch
> 
> Review of attachment 791384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine. The only change I'd like to see is: the code to slide the
> snippet in/out doesn't really belong to HomeSnippet imo. This is an
> implementation detail of how snippets are presented specifically in the
> BookmarksPage. So, remove the OnCoScrollListener entirely and handle the
> translation bits directly in BookmarksPage. HomeSnippet should only have to
> deal with its inner logic and views.
> 
> ::: mobile/android/base/home/HomeSnippet.java
> @@ +60,5 @@
> > +
> > +                mEvent = MotionEvent.obtain(event);
> > +                break;
> > +
> > +            case MotionEvent.ACTION_UP:
> 
> You probably need to handle ACTION_CANCEL too.

What needs to be done in ACTION_CANCEL? Do you want me to do the same thing as ACTION_UP?
Comment on attachment 791384 [details] [diff] [review]
Patch

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

::: mobile/android/base/home/HomeSnippet.java
@@ +62,5 @@
> +                break;
> +
> +            case MotionEvent.ACTION_UP:
> +                final float y = ViewHelper.getTranslationY(this);
> +                final PropertyAnimator animator = new PropertyAnimator(100);

Can you pop this out into a constant? I'd like to try and unify our animation timing values at some point (bug 906749) although I'm wondering if maybe using Android's is a better choice? i.e. https://developer.android.com/reference/android/R.integer.html#config_shortAnimTime
(Assignee)

Comment 15

6 years ago
(In reply to Wesley Johnston (:wesj) from comment #14)
> Comment on attachment 791384 [details] [diff] [review]
> Patch
> 
> Review of attachment 791384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeSnippet.java
> @@ +62,5 @@
> > +                break;
> > +
> > +            case MotionEvent.ACTION_UP:
> > +                final float y = ViewHelper.getTranslationY(this);
> > +                final PropertyAnimator animator = new PropertyAnimator(100);
> 
> Can you pop this out into a constant? I'd like to try and unify our
> animation timing values at some point (bug 906749) although I'm wondering if
> maybe using Android's is a better choice? i.e.
> https://developer.android.com/reference/android/R.integer.
> html#config_shortAnimTime

Oh nice. But this was just for testing. I had a weird calculation like "amount to displace * 2". But not sure if we want that. Say the user scrolled 80%, and the rest 20% should be animated. This shouldn't take the same time as 50% or 70%. So, using a constant like 100ms didnt feel good.

On the contrary, using anything below 250ms, users aren't sensitive. So, we could use 100ms. And I use 100ms, I'll move this to a constant or use short time.
Drive-by naming bikeshed comment: I feel like this should be named HomeBanner, rather than HomeSnippet, since that better describes what this actual UI widget is. This would also make things clearer if we use the word "snippet" to describe a piece of content that rotates through this banner (something I'm dealing with in bug 905262).
(In reply to :Margaret Leibovic from comment #16)
> Drive-by naming bikeshed comment: I feel like this should be named
> HomeBanner, rather than HomeSnippet, since that better describes what this
> actual UI widget is. This would also make things clearer if we use the word
> "snippet" to describe a piece of content that rotates through this banner
> (something I'm dealing with in bug 905262).

Agreed. Technically, this can be used for non-snippet stuff too. HomeBanner is a better description of the UI.
(In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> (In reply to Lucas Rocha (:lucasr) from comment #12)
> > Comment on attachment 791384 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 791384 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks fine. The only change I'd like to see is: the code to slide the
> > snippet in/out doesn't really belong to HomeSnippet imo. This is an
> > implementation detail of how snippets are presented specifically in the
> > BookmarksPage. So, remove the OnCoScrollListener entirely and handle the
> > translation bits directly in BookmarksPage. HomeSnippet should only have to
> > deal with its inner logic and views.
> > 
> > ::: mobile/android/base/home/HomeSnippet.java
> > @@ +60,5 @@
> > > +
> > > +                mEvent = MotionEvent.obtain(event);
> > > +                break;
> > > +
> > > +            case MotionEvent.ACTION_UP:
> > 
> > You probably need to handle ACTION_CANCEL too.
> 
> What needs to be done in ACTION_CANCEL? Do you want me to do the same thing
> as ACTION_UP?

Pretty much.
(Assignee)

Comment 19

6 years ago
Posted patch Patch (obsolete) — Splinter Review
This adds all the bits for the banner (we've renamed it :D). The BookmarksPage takes care of setting the banner's translation. Currently the banner doesn't show anything. This will be powered from the js side through api's. Hence I've made the banner "gone". The click on the close button should close the banner for this user's session. So, it just makes the banner "gone" (again), which takes care of it.

I've made checks to avoid overscroll creeping into the banner's scroll. Usually its avoided. But once in 10 times, there is a slight chance that android does something stupid, and triggers a scroll for the list's overscroll.

I couldn't avoid scrolling the banner with the list is too small to scroll. I tried attaching an OnScrollListener, but it doesnt help while rotating. I don't have a straightforward idea to block the banner from scrolling in this case. If we want, we can do some hacks around it.
Attachment #793175 - Flags: review?(lucasr.at.mozilla)
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → All
tracking-fennec: ? → 26+
Comment on attachment 793175 [details] [diff] [review]
Patch

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

Looks good overall. Just needs to cover my suggested changes and questions. Make sure ibarlow reviews a test build.

::: mobile/android/base/home/BookmarksPage.java
@@ +80,5 @@
>  
>      // Grid of top bookmarks.
>      private TopBookmarksView mTopBookmarks;
>  
> +    // Banner.

Just a bit more context, please :-)

@@ +207,5 @@
>                                  .commitAllowingStateLoss();
>          }
>      }
>  
> +    private void onListTouchEvent(MotionEvent event) {

I'd rename this to handleListTouchEvent. The 'on' prefix is more for listeners and standard component hooks.

@@ +209,5 @@
>      }
>  
> +    private void onListTouchEvent(MotionEvent event) {
> +        // Don't pass the event if the banner is hidden for this session.
> +        if (mBanner.getVisibility() == View.GONE) {

I'd prefer a more explicit toggle in HomeBanner instead. Something like mBanner.isDimissed() or something. Using view visibility to track dismissal state can lead to bugs if we change the UI in the future.

@@ +221,5 @@
> +             }
> +
> +            case MotionEvent.ACTION_MOVE: {
> +                if (mListTouchEvent == null) {
> +                    mListTouchEvent = MotionEvent.obtain(event);

How is it possible to start dragging without a previous ACTION_DOWN? What situation is this 'if' covering for?

@@ +229,5 @@
> +                final float delta = mListTouchEvent.getRawY() - event.getRawY();
> +                mSnapBannerToTop = delta > 0.0f ? false : true;
> +
> +                final float height = mBanner.getHeight();
> +                float totalDelta = ViewHelper.getTranslationY(mBanner) + delta;

I'd rename this to newTranslationY. totalDelta doesn't seem to match the intent of the variable.

@@ +230,5 @@
> +                mSnapBannerToTop = delta > 0.0f ? false : true;
> +
> +                final float height = mBanner.getHeight();
> +                float totalDelta = ViewHelper.getTranslationY(mBanner) + delta;
> +

Add a comment here explaining that you're clamping the value to expected ranges.

@@ +238,5 @@
> +                    totalDelta = height;
> +                }
> +
> +                if ((mSnapBannerToTop && totalDelta == 0.0f) ||
> +                    (!mSnapBannerToTop && totalDelta == height)) {

Is this really necessary? You're clamping the value to expected ranges anyway. The setTranslationY will be a no-op anyway.

@@ +244,5 @@
> +                    return;
> +                }
> +
> +                ViewHelper.setTranslationY(mBanner, totalDelta);
> +                mListTouchEvent = MotionEvent.obtain(event);

This is potentially creating a new object on each ACTION_MOVE event which will happen a lot while scrolling. Not ideal. The only thing you actually need from the MotionEvent is its rawY. You can simply store the rawY in an mListTouchY and set it back to -1 (screen position is never negative anyway) on ACTION_UP/ACTION_CANCEL.

::: mobile/android/base/home/HomeBanner.java
@@ +26,5 @@
> +        LayoutInflater.from(context).inflate(R.layout.home_banner, this);
> +    }
> +
> +    @Override
> +    public void onAttachedToWindow() {

nit: we have traditionally been setting up inner views in the View's constructor. You'd have a stronger case for doing it in onAttachedToWindow() if you had to release resources accordingly in onDettachedFromWindow() or something. Anyway, I don't feel strongly about it.

@@ +31,5 @@
> +        super.onAttachedToWindow();
> +
> +        // Tapping on the close button will ensure that the banner is never
> +        // showed again on this session.
> +        final ImageButton close = (ImageButton) findViewById(R.id.close);

nit: closeButton for clarity?

@@ +32,5 @@
> +
> +        // Tapping on the close button will ensure that the banner is never
> +        // showed again on this session.
> +        final ImageButton close = (ImageButton) findViewById(R.id.close);
> +        close.getDrawable().setAlpha(127);

What's this about? Add a comment explaining.

::: mobile/android/base/resources/drawable/home_banner.xml
@@ +13,5 @@
> +
> +                <shape android:shape="rectangle" >
> +                    <stroke android:width="2dp"
> +                            android:color="#FFE0E4E7" />
> +                    <solid android:color="#FFC5D0DA" />

Are these colours possibly going to be used elsewhere? If your answer is yes, define them in colors.xml

::: mobile/android/base/resources/layout/home_bookmarks_page.xml
@@ +13,5 @@
> +            android:layout_height="fill_parent"/>
> +
> +    <org.mozilla.gecko.home.HomeBanner android:id="@+id/home_banner"
> +                                       style="@style/Widget.HomeBanner"
> +                                       android:layout_width="fill_parent"

Should we limit the width of the banner on tablets somehow? Otherwise we'll have these super-long banners at the bottom of the screen. Check with ibarlow and update the patch accordingly.

::: mobile/android/base/resources/layout/home_pager.xml
@@ +23,5 @@
>                                                    gecko:tabIndicatorColor="@color/text_color_highlight"
>                                                    android:textAppearance="@style/TextAppearance.Widget.HomePagerTabStrip"/>
>  
>      </org.mozilla.gecko.home.HomePager>
> +</merge>

Unrelated?

::: mobile/android/base/resources/values-large-land-v11/styles.xml
@@ +51,5 @@
>      </style>
>  
> +    <style name="Widget.HomeBanner">
> +        <item name="android:paddingLeft">170dp</item>
> +        <item name="android:paddingRight">170dp</item>

Hmm, now I see that you're limiting the size on tablets with padding but maybe what we want is a fixed size instead?
Attachment #793175 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 21

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Fixed all the comments.
Attachment #793175 - Attachment is obsolete: true
Attachment #795680 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 22

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #20)
> Comment on attachment 793175 [details] [diff] [review]
> Patch
> 
> Review of attachment 793175 [details] [diff] [review]:
> -----------------------------------------------------------------
>  
> @@ +221,5 @@
> > +             }
> > +
> > +            case MotionEvent.ACTION_MOVE: {
> > +                if (mListTouchEvent == null) {
> > +                    mListTouchEvent = MotionEvent.obtain(event);
> 
> How is it possible to start dragging without a previous ACTION_DOWN? What
> situation is this 'if' covering for?

Sometimes the list doesn't give us the first touch down event. May be that's used by the GridView and consumed by the ListView -- but not given to any touch listeners. In such cases, this "if" will make the first move event to be treated as down.

> 
> @@ +238,5 @@
> > +                    totalDelta = height;
> > +                }
> > +
> > +                if ((mSnapBannerToTop && totalDelta == 0.0f) ||
> > +                    (!mSnapBannerToTop && totalDelta == height)) {
> 
> Is this really necessary? You're clamping the value to expected ranges
> anyway. The setTranslationY will be a no-op anyway.

Nope. This happens during overcroll. The banner tends to jump to a displacement and then start from there. This helps in avoiding the scroll during overscroll.

> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +26,5 @@
> > +        LayoutInflater.from(context).inflate(R.layout.home_banner, this);
> > +    }
> > +
> > +    @Override
> > +    public void onAttachedToWindow() {
> 
> nit: we have traditionally been setting up inner views in the View's
> constructor. You'd have a stronger case for doing it in onAttachedToWindow()
> if you had to release resources accordingly in onDettachedFromWindow() or
> something. Anyway, I don't feel strongly about it.

We've started moving the onClickListeners to onAttachedToWindow(). Since we don't need a reference to close button as a private variable, I'm getting a reference and setting a listener in onAttachedToWindow(). I didn't change this part.

> 
> @@ +32,5 @@
> > +
> > +        // Tapping on the close button will ensure that the banner is never
> > +        // showed again on this session.
> > +        final ImageButton close = (ImageButton) findViewById(R.id.close);
> > +        close.getDrawable().setAlpha(127);
> 
> What's this about? Add a comment explaining.

We re-use tabs tray's close button -- which is dark. Hence 50% opacity as per ian's suggestion.

> 
> ::: mobile/android/base/resources/drawable/home_banner.xml
> @@ +13,5 @@
> > +
> > +                <shape android:shape="rectangle" >
> > +                    <stroke android:width="2dp"
> > +                            android:color="#FFE0E4E7" />
> > +                    <solid android:color="#FFC5D0DA" />
> 
> Are these colours possibly going to be used elsewhere? If your answer is
> yes, define them in colors.xml

Nope. We don't use it anywhere else. Hence didn't feel like moving it to colors.xml.

> 
> ::: mobile/android/base/resources/layout/home_bookmarks_page.xml
> @@ +13,5 @@
> > +            android:layout_height="fill_parent"/>
> > +
> > +    <org.mozilla.gecko.home.HomeBanner android:id="@+id/home_banner"
> > +                                       style="@style/Widget.HomeBanner"
> > +                                       android:layout_width="fill_parent"
> 
> Should we limit the width of the banner on tablets somehow? Otherwise we'll
> have these super-long banners at the bottom of the screen. Check with
> ibarlow and update the patch accordingly.

As per Ian, the width is going to be as wide as the device. But the text inside will fit in line with the grid/lists.

> 
> ::: mobile/android/base/resources/layout/home_pager.xml
> @@ +23,5 @@
> >                                                    gecko:tabIndicatorColor="@color/text_color_highlight"
> >                                                    android:textAppearance="@style/TextAppearance.Widget.HomePagerTabStrip"/>
> >  
> >      </org.mozilla.gecko.home.HomePager>
> > +</merge>
> 
> Unrelated?

Yup. I don't know why this shows up. There doesn't seem to be any change though.

> 
> ::: mobile/android/base/resources/values-large-land-v11/styles.xml
> @@ +51,5 @@
> >      </style>
> >  
> > +    <style name="Widget.HomeBanner">
> > +        <item name="android:paddingLeft">170dp</item>
> > +        <item name="android:paddingRight">170dp</item>
> 
> Hmm, now I see that you're limiting the size on tablets with padding but
> maybe what we want is a fixed size instead?

No. Not fixed size. It's device-width with padding for the text alone -- to make it align with grids/lists.
Comment on attachment 795680 [details] [diff] [review]
Patch

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

Looks nice but I still don't follow the reason behind the early return on the ACTION_MOVE handling code.

::: mobile/android/base/home/BookmarksPage.java
@@ +111,3 @@
>  
> +        mList = (BookmarksListView) view.findViewById(R.id.bookmarks_list);
> + 

nit: remove trailing whitespace.

@@ +114,3 @@
>          mTopBookmarks = new TopBookmarksView(getActivity());
> +        mList.addHeaderView(mTopBookmarks);
> + 

nit: remove trailing whitespace.

@@ +214,5 @@
>          }
>      }
>  
> +    private void handleListTouchEvent(MotionEvent event) {
> +        // Don't pass the event if the banner is hidden for this session.

Don't pass -> Ignore

@@ +235,5 @@
> +                }
> +
> +                final float curY = event.getRawY();
> +                final float delta = mListTouchY - curY;
> +                mSnapBannerToTop = delta > 0.0f ? false : true;

nit: enclose the ternary conditional with parens.

@@ +249,5 @@
> +                }
> +
> +                if ((mSnapBannerToTop && newTranslationY == 0.0f) ||
> +                    (!mSnapBannerToTop && newTranslationY == height)) {
> +                    // It's already displaced to the maximum, don't translate it again on overscroll.

I still don't understand why you need this early return here. If you return here, you won't update mListTouchY for as long as the banner is either fully on-screen or fully off-screen, which seems wrong given that the whole dragging logic here uses deltas between the the y position of previous MotionEvent and the current one (delta = mListTouchY - curY). 

nit: I'd move this comment to be before the 'if'.

::: mobile/android/base/resources/layout/home_bookmarks_page.xml
@@ +14,5 @@
> +
> +    <org.mozilla.gecko.home.HomeBanner android:id="@+id/home_banner"
> +                                       style="@style/Widget.HomeBanner"
> +                                       android:layout_width="fill_parent"
> +                                       android:layout_height="@dimen/home_banner_height"

Should this actually be min_height? What if the text is too long and doesn't fit in the fixed height?

::: mobile/android/base/resources/values-large-land-v11/styles.xml
@@ +51,5 @@
>      </style>
>  
> +    <style name="Widget.HomeBanner">
> +        <item name="android:paddingLeft">170dp</item>
> +        <item name="android:paddingRight">170dp</item>

Add a comment explaining where these dimensions are coming from. These padding values have to match BookmarkItemView padding + BookmarkListView padding (left and right). It's a bit unfortunate that you're not defining these dimensions once and reusing it everywhere which would be a bit less error-prone for future changes. But I don't feel strongly about it.
Attachment #795680 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 24

6 years ago
Posted patch PatchSplinter Review
Made all required changes. Please check my replies for the previous patch's reviews comments.. in my next comment.
Attachment #795680 - Attachment is obsolete: true
Attachment #796276 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 25

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 795680 [details] [diff] [review]
> Patch
> 
> 
> @@ +249,5 @@
> > +                }
> > +
> > +                if ((mSnapBannerToTop && newTranslationY == 0.0f) ||
> > +                    (!mSnapBannerToTop && newTranslationY == height)) {
> > +                    // It's already displaced to the maximum, don't translate it again on overscroll.
> 
> I still don't understand why you need this early return here. If you return
> here, you won't update mListTouchY for as long as the banner is either fully
> on-screen or fully off-screen, which seems wrong given that the whole
> dragging logic here uses deltas between the the y position of previous
> MotionEvent and the current one (delta = mListTouchY - curY). 
> 
> nit: I'd move this comment to be before the 'if'.

Agreed. The error was in ACTION_UP/ACTION_CANCEL. In my previous version, I was returning early if the translation is already snapped -- either to 0 or height. In that case, I wasn't resetting the mListTouchY to -1. So the next new event directly coming ACTION_MOVE (as ACTION_DOWN gets skipped due to Grid in a List issue), which not be caught by the | if (mListTouchY == -1) | check. So, the delta will be huge as it will be the actual rawY. So, the banner will displace to some huge Y and then starts crawling back. In this patch I'm resetting the value to -1 in ACTION_UP/ACTION_CANCEL. So, it would be caught in the ACTION_MOVE check and we don't need the above lines of code.

> 
> ::: mobile/android/base/resources/layout/home_bookmarks_page.xml
> @@ +14,5 @@
> > +
> > +    <org.mozilla.gecko.home.HomeBanner android:id="@+id/home_banner"
> > +                                       style="@style/Widget.HomeBanner"
> > +                                       android:layout_width="fill_parent"
> > +                                       android:layout_height="@dimen/home_banner_height"
> 
> Should this actually be min_height? What if the text is too long and doesn't
> fit in the fixed height?

No. This is a fixed sized banner. It's up to the addon devs to make sure the text is small and sweet (:P) and fits within 3 lines. If it goes over 3 lines, we show ellip...sis. UX Approved (TM) ;)

> 
> ::: mobile/android/base/resources/values-large-land-v11/styles.xml
> @@ +51,5 @@
> >      </style>
> >  
> > +    <style name="Widget.HomeBanner">
> > +        <item name="android:paddingLeft">170dp</item>
> > +        <item name="android:paddingRight">170dp</item>
> 
> Add a comment explaining where these dimensions are coming from. These
> padding values have to match BookmarkItemView padding + BookmarkListView
> padding (left and right). It's a bit unfortunate that you're not defining
> these dimensions once and reusing it everywhere which would be a bit less
> error-prone for future changes. But I don't feel strongly about it.

I would be happy to define in dimens and use it. However we don't have a "+" operator with dimens. If not this would be like

<dimen name="home_banner_padding">@dimen/bookmark_list_padding + @dimen/two_line_page_row_padding</dimen>

Since we don't have that option, it doesn't feel good to redirect one time values to a dimens file.
Comment on attachment 796276 [details] [diff] [review]
Patch

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

Nice.

::: mobile/android/base/home/BookmarksPage.java
@@ +257,5 @@
> +            case MotionEvent.ACTION_CANCEL: {
> +                mListTouchY = -1;
> +                final float y = ViewHelper.getTranslationY(mBanner);
> +                final float height = mBanner.getHeight();
> +                if (y == 0.0f || y == height) {

nit: I'd prefer something like:

if (y > 0.0f && y < height) {
    final PropertyAnimator animator = new PropertyAnimator(100);
    animator.attach(mBanner, Property.TRANSLATION_Y, mSnapBannerToTop ? 0 : height);
    animator.start();
}

::: mobile/android/base/resources/layout/home_pager.xml
@@ +23,5 @@
>                                                    gecko:tabIndicatorColor="@color/text_color_highlight"
>                                                    android:textAppearance="@style/TextAppearance.Widget.HomePagerTabStrip"/>
>  
>      </org.mozilla.gecko.home.HomePager>
> +</merge>

Try to remove this blank change before pushing?
Attachment #796276 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a1d682f3e569
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Updated

6 years ago
Depends on: 920317

Updated

6 years ago
Depends on: 920791
Depends on: 921557
Depends on: 921668

Updated

5 years ago
Blocks: 937820
Depends on: 961523

Updated

5 years ago
Depends on: 961501
I know this bug was fixed long ago, but I'm updating the summary to make it more clear what this did (snippets are a different thing than the home banner). Especially since we're hanging home banner dependencies off of this bug.
Alias: home-banner
Depends on: 963561
Summary: About:home Snippet → Create Java UI for home banner

Updated

5 years ago
Attachment #791384 - Attachment is obsolete: true

Updated

5 years ago
Depends on: 934678

Updated

5 years ago
Depends on: 960359

Updated

5 years ago
Depends on: 967085

Updated

5 years ago
Depends on: 974723

Updated

5 years ago
Depends on: 976175

Updated

5 years ago
Depends on: 976176

Updated

5 years ago
Depends on: 966047

Updated

5 years ago
No longer depends on: 976175

Updated

5 years ago
Depends on: 976175
Depends on: 976670
Depends on: 976680

Updated

5 years ago
Depends on: 977155

Updated

5 years ago
Depends on: 977394

Updated

5 years ago
Depends on: 976185

Updated

5 years ago
Depends on: 982891
(Reporter)

Updated

5 years ago
Depends on: 984873
You need to log in before you can comment on or make changes to this bug.