1118934 - [Gallery] - Thumbnail list bottom thumbnails are overlapped by the toolbar

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Punam Dahiya [:pdahiya]]

STR:

1. Open Gallery app.

2. Scroll to the bottom of thumbnail list

3. Last thumbnails are partially hidden behind toolbar

Expected: Last thumbnail should not be hidden behind toolbar.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1100268#c6

Comment 1

[Punam Dahiya [:pdahiya]]

On debugging margin-bottom is not getting applied on last parent li element because it's children are floated parent li has no height. 

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/style/gallery.css#L264

Add a clearing mechanism using overflow:hidden makes margin-bottom on last li element work.

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: Patch with fix of Bug 1118934

Hi David

Please review attached patch with css fixes to apply margin-bottom 4.5rem to last li element in thumbnail select and list view. 

We already have a margin on #thumbnails > li:last-child but because li children div's  group-thumbnail-header and thumbnail-group-container are floated, parent li has no height and collapses behind it's children and margin doesn't work.

Explicitly adding a clear mechanism using overflow:hidden to li gives it height and make margin-works. Thanks

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8548415 [details] [review]
Patch with fix of Bug 1118934

Do you remember why we're using a float property on the thumbnails? That seems weird to me. What happens if we just get rid of the float (and the rtl code that overrides it with float:right)?  (Git blame tells me that you added the float:left line in bug 925179)

Basically, I don't understand why the float is there, and I don't understand why overflow:none clears the float, so I'm worried that this patch is adding unnecessary complexity.  On the other hand, it could just be that I don't understand. 

If the float really is needed and the overflow:none is needed here, then I'm okay with landing this patch as it is, but I'd prefer it if we could fix this with something that made the css simpler rather than more complex.

Comment 4

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #3)
> Comment on attachment 8548415 [details] [review]
> Patch with fix of Bug 1118934
> 
> Do you remember why we're using a float property on the thumbnails? That
> seems weird to me. What happens if we just get rid of the float (and the rtl
> code that overrides it with float:right)?  (Git blame tells me that you
> added the float:left line in bug 925179)
> 

thumbnail being div block element, unless floated will always start a new line and will stack vertically. 
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/style/gallery.css#L219

In 1.3 we added group headers by month (925179), that wraps original floated thumbnail list in group container and group header.

> Basically, I don't understand why the float is there, and I don't understand
> why overflow:none clears the float, so I'm worried that this patch is adding
> unnecessary complexity.  On the other hand, it could just be that I don't
> understand. 
> 
> If the float really is needed and the overflow:none is needed here, then I'm
> okay with landing this patch as it is, but I'd prefer it if we could fix
> this with something that made the css simpler rather than more complex.

Looking back at 925179 patch, I realized it's below change that caused this regression.
https://github.com/mozilla-b2g/gaia/pull/13409/files#diff-7799e388381b4a1abd55fcf1b815f99dR246

Reverting back margin-bottom to last thumbnail fixes this bug.

Comment 5

[Punam Dahiya [:pdahiya]]

Comment on attachment 8548415 [details] [review]
Patch with fix of Bug 1118934

Hi David
Thanks for pointing to 925179, I have updated patch to set margin-bottom on last thumbnail item as it was originally written. Since we don't have toolbar in pick view, margin-bottom is not needed there and patch sets margin-bottom for thumbnail list and select view only. Please review.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8548415 [details] [review]
Patch with fix of Bug 1118934

This looks like a cleaner patch. Thanks.

I didn't notice the use of float when we converted to have the date headers. I don't think we were doing that before the date headers were added, and I would have thought that display: inline-block would have worked correctly (including for rtl).  But that is a separate issue.

Comment 7

[Punam Dahiya [:pdahiya]]

Thanks David for review. Fix landed on master

https://github.com/mozilla-b2g/gaia/commit/d9f6648b7931cd4213a8beb01b0358160ec327e9