921468 - [tablet] Empty space at the top of about:home list views

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[Gian-Carlo Pascutto [:gcp]]

Attachment: Screenshot of glitch

Android 3.2, Galaxy Tab 10.1

Note the top part of the Twitter URL+favicon is cut off.

Comment 1

[Aaron Train [:aaronmt]]

I kind of see the same thing when viewing an about:home snippet banner, maybe related?

Comment 2

[Lucas Rocha (:lucasr)]

gcp, is the list scrolled a bit or is it just cut off when it's initially displayed?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Looks cut off to me, see screenshot.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

GCP, can you add STR?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret has a 10.1 device to try as well.

Comment 6

[:Margaret Leibovic]

I can reproduce this. I'll see if I can figure out how to fix it.

Comment 7

[:Margaret Leibovic]

Also reproducible on a Nexus 10.

Comment 8

[:Margaret Leibovic]

So, this same issue happens if you scroll any list in the tablet UI because we have a space at the top of the page. I agree it does look weird, but I don't know what the solution should be. Maybe instead of creating a thin gray border for the top item in the list, we should make sure that space just has a bottom gray border. However, this would only work for the regular list pages, not the page with thumbnails.

I'm not familiar with what we decided to do for tablets, maybe Sriram would know more.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #8)
> So, this same issue happens if you scroll any list in the tablet UI because
> we have a space at the top of the page. I agree it does look weird, but I
> don't know what the solution should be. Maybe instead of creating a thin
> gray border for the top item in the list, we should make sure that space
> just has a bottom gray border. However, this would only work for the regular
> list pages, not the page with thumbnails.
> 
> I'm not familiar with what we decided to do for tablets, maybe Sriram would
> know more.

If you want the top padding to go away when you scroll the list down, you could remove the padding from the listview itself...

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values-large-v11/styles.xml#65

...and move it to the top divider view (which we add a header view):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeListView.java#70

If you do that, it would be nice if we could set the "top divider's top padding" independently via styles.xml.

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> (In reply to :Margaret Leibovic from comment #8)
> > So, this same issue happens if you scroll any list in the tablet UI because
> > we have a space at the top of the page. I agree it does look weird, but I
> > don't know what the solution should be. Maybe instead of creating a thin
> > gray border for the top item in the list, we should make sure that space
> > just has a bottom gray border. However, this would only work for the regular
> > list pages, not the page with thumbnails.
> > 
> > I'm not familiar with what we decided to do for tablets, maybe Sriram would
> > know more.
> 
> If you want the top padding to go away when you scroll the list down, you
> could remove the padding from the listview itself...
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> values-large-v11/styles.xml#65
> 
> ...and move it to the top divider view (which we add a header view):
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeListView.java#70
> 
> If you do that, it would be nice if we could set the "top divider's top
> padding" independently via styles.xml.

Thanks for the suggestion, this appears to be a good solution. Setting padding on the top divider view doesn't seem to be working for me, but setting the height of the view to include the space we'd want for padding is working. However, I found that with this approach the space at the top of the top sites grid view disappears, so maybe HomeListView isn't the right place to be adding this space.

Aside: it's pretty non-intuitive that removing the top padding in the "Widget.BookmarksListView" style removes the top padding from all of the pages in about:home. Why is this?

Comment 11

[:Margaret Leibovic]

Attachment: WIP

Here's what I was working with.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #10)
> (In reply to Lucas Rocha (:lucasr) from comment #9)
> > (In reply to :Margaret Leibovic from comment #8)
> > > So, this same issue happens if you scroll any list in the tablet UI because
> > > we have a space at the top of the page. I agree it does look weird, but I
> > > don't know what the solution should be. Maybe instead of creating a thin
> > > gray border for the top item in the list, we should make sure that space
> > > just has a bottom gray border. However, this would only work for the regular
> > > list pages, not the page with thumbnails.
> > > 
> > > I'm not familiar with what we decided to do for tablets, maybe Sriram would
> > > know more.
> > 
> > If you want the top padding to go away when you scroll the list down, you
> > could remove the padding from the listview itself...
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > values-large-v11/styles.xml#65
> > 
> > ...and move it to the top divider view (which we add a header view):
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> > HomeListView.java#70
> > 
> > If you do that, it would be nice if we could set the "top divider's top
> > padding" independently via styles.xml.
> 
> Thanks for the suggestion, this appears to be a good solution. Setting
> padding on the top divider view doesn't seem to be working for me, but
> setting the height of the view to include the space we'd want for padding is
> working. However, I found that with this approach the space at the top of
> the top sites grid view disappears, so maybe HomeListView isn't the right
> place to be adding this space.

What I had in mind was a new style attribute in HomeListView called inContentMarginTop or something. This would be a separate view from the divider. You'd add another header view before the current top divider one with the height defined by inContentMarginTop. This way you can set the in-content top margin for each pane without having to show a top divider (which is why the space at the top of top sites grid disappears with your patch). 

> Aside: it's pretty non-intuitive that removing the top padding in the
> "Widget.BookmarksListView" style removes the top padding from all of the
> pages in about:home. Why is this?

BookmarksListView should probably be renamed to something more generic. Please file a follow-up to track this.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 819174 [details] [diff] [review]
WIP

This is roughly the idea but defining a separate style attribute which controls a different header view is probably a better way to go.

Comment 14

[:Margaret Leibovic]

I haven't had time to work on this. IIRC, this bug only affects honeycomb, which makes me think we should just WONTFIX it.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Earlier it was said it reproduces on the Nexus 10, so it's not Honeycomb specific but affects all big tablets, AFAIK.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #15)
> Earlier it was said it reproduces on the Nexus 10, so it's not Honeycomb
> specific but affects all big tablets, AFAIK.

Margaret, could you actually reproduce this bug on a Nexus 10?

Comment 17

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #16)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #15)
> > Earlier it was said it reproduces on the Nexus 10, so it's not Honeycomb
> > specific but affects all big tablets, AFAIK.
> 
> Margaret, could you actually reproduce this bug on a Nexus 10?

wesj said he could reproduce this on a Nexus 10... and I see now I actually said that in comment 7 as well. I must be misremembering our past conversations, or maybe there was some confusion about what exactly the problem is here.

In any case, we should keep this open and try to fix it.

I'm busy with other things right now, but I'll mark it as a mentor bug because it's a well-scoped issue. However, anyone who wants to work on this bug will need a tablet to test.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This appears to have been fixed at some point.