1150835 - [Flame][Homescreen]Add a bookmark to Home screen in landscape mode, go back to Home screen, the app icons change into two columns and the size of bookmark icon is too big.

Status: VERIFIED FIXED

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

Description

[Shine(Leave from Mozilla)]

Attachment: logcat_1212.txt

[1.Description]:
[Flame][v2.2&v3.0][Homescreen]Launch Browser app and switch to landscape mode, add a bookmark to Home screen, go back to Home screen, the app icons on Home screen will change into two columns and the size of bookmark icon you added is too big. 
See attachment: logcat_1212.txt & Video1.MP4
Found at: 12:12

[2.Testing Steps]: 
1. Launch Browser app and switch to landscape mode.
2. Link to a website and tap menu button.
3. Select "Add to home screen".
4. Tap "Done" to add it to Home screen.
5. Press Home button.

[3.Expected Result]: 
5.  The app icons on Home screen shouldn't be changed into two columns and the size of bookmark icon you added should be normal.

[4.Actual Result]: 
5.  The app icons on Home screen change into two columns and the size of bookmark icon you added is too big.

[5.Reproduction build]: 
Device: Flame 2.2 (Affected)
Build ID               20150402002500
Gaia Revision          1ceca464053dee4a8bf10ea5abeef724d68c2ff2
Gaia Date              2015-04-01 09:49:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/427b4da96714
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150402.035057
Firmware Date          Thu Apr  2 03:51:09 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 (Affected)
Build ID               20150402160202
Gaia Revision          62042ffcc8c6cca0f51ad23f5c2b979fc153b5a7
Gaia Date              2015-04-02 16:01:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/513265a4cbc2
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150402.192125
Firmware Date          Thu Apr  2 19:21:37 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

[8.Note]:
How to recovery: Reboot device.

Comment 1

[Shine(Leave from Mozilla)]

Attachment: Video1.MP4

Comment 2

[Gregor Wagner [:gwagner]]

Kevin, can you take a look?

Comment 3

[Chris Lord [:cwiiis]]

I'm on PTO (and away from any computers) for the next week, but I expect this is a regression from bug 1146825, and specifically this section: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/grid_layout.js#L86 - The grid width is calculated based on its container size only once, and never updated. So if the homescreen restarts in the background while the foreground app is landscape and something causes it to render, the grid width will be calculated incorrectly and remain that way until it's restarted in portrait mode.

I think this optimisation is a bit suspect, but also, we probably oughtn't be rendering the grid while it's in the background either.

Comment 4

[Chris Lord [:cwiiis]]

On further thought, there are probably paths outside of rendering that request layout.gridWidth too.

I don't think we can sensibly only calculate the width once - either we add code to invalidate it, or we just don't cache it. I'm not sure that just getting the offsetWidth each time would be a huge issue, it would only cause extra work if that extra work needed to be done, but given the grid has a fixed width, that shouldn't be an issue.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

Chris or Dale - here's a patch which constrains the size to the minimum of the screen height and width, which should work regardless of orientation. This seemed like the easiest fix to make, with the lowest amount of risk. I extensively tested this, but would not be surprised if I missed something. Please take a look if you have time. Thanks!

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

I'm not keen on doing it like this - I think this would break the homescreen on tablets, for example, where you'd end up with the grid leaving a large blank area on the right.

I'm also not convinced this would really work correctly for the search screen either, assuming that the orientation can change while the grid is visible.

A one-line fix that I think would have fewer issues would be to call this.layout.calculateSize() at the start of gridView.render().

Comment 8

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #7)
> Comment on attachment 8588356 [details] [review]
> [gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master
> 
> I'm not keen on doing it like this - I think this would break the homescreen
> on tablets, for example, where you'd end up with the grid leaving a large
> blank area on the right.

I didn't test it, but I don't think that's correct. We actually already adjust the size for tablets somewhere in a similar fashion, otherwise the icons are way too big. (I also don't think we should worry about tablets right now as they aren't supported).

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #7)
> I'm also not convinced this would really work correctly for the search
> screen either, assuming that the orientation can change while the grid is
> visible.

I tested and search works fine because it uses the smaller measurement always since we're using the minimum (320px or so for the flame).

> A one-line fix that I think would have fewer issues would be to call
> this.layout.calculateSize() at the start of gridView.render().

This doesn't work because we're calculating the size incorrectly currently. The element width is too large due to it being in landscape, and I think we don't call render() when returning to the home screen.


Chris - I'm out of ideas and tend to think my original patch is the best thing to do for now, unless you have ideas.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #9)
> (In reply to Chris Lord [:cwiiis] from comment #7)
> > I'm also not convinced this would really work correctly for the search
> > screen either, assuming that the orientation can change while the grid is
> > visible.
> 
> I tested and search works fine because it uses the smaller measurement
> always since we're using the minimum (320px or so for the flame).
> 
> > A one-line fix that I think would have fewer issues would be to call
> > this.layout.calculateSize() at the start of gridView.render().
> 
> This doesn't work because we're calculating the size incorrectly currently.
> The element width is too large due to it being in landscape, and I think we
> don't call render() when returning to the home screen.
> 
> 
> Chris - I'm out of ideas and tend to think my original patch is the best
> thing to do for now, unless you have ideas.

I'm ok for this to go in if we need a quick fix - what worries me is the change of the layout code - the layout is specifically height-for-width, it doesn't make sense in all cases to switch out the minimum of width and height and your patch also doesn't use the element's size anymore, it still assumes a lot about how the grid is contained (which perhaps is fair as we only use it in two places and the limitation was even more severe before Dale's patch).

I suppose what we need is for the grid to recalculate its width and re-render on orientation change. If the icons are too big in landscape on the search page, it should be adding appropriate horizontal margins in landscape mode so that the width decreases. If we need the icons to have rectangular boundaries rather than square (ignoring the title), then we should add something for that too, rather than making assumptions based on window size.

I still think the correct fix is similar to what I suggest - recalculate the width on render, then add resize callbacks where it's embedded to just call render. The search app may also need horizontal margins, or to use the aforementioned set-icon-width function, I don't know anything about how it's laid out at the moment.

If we need a quick fix, let's go with this, but if we do, let's at least open a bug to either replace grid or make it usable under conditions where it doesn't occupy the full window width.

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

(In reply to Chris Lord [:cwiiis] from comment #10)
> I'm ok for this to go in if we need a quick fix - what worries me is the
> change of the layout code - the layout is specifically height-for-width, it
> doesn't make sense in all cases to switch out the minimum of width and
> height and your patch also doesn't use the element's size anymore, it still
> assumes a lot about how the grid is contained (which perhaps is fair as we
> only use it in two places and the limitation was even more severe before
> Dale's patch).

Right - we use it in three places right now (search, collection, home screen), and we have a fairly good idea about how we use it which is why I think these changes are fairly safe to make.

> I suppose what we need is for the grid to recalculate its width and
> re-render on orientation change. If the icons are too big in landscape on
> the search page, it should be adding appropriate horizontal margins in
> landscape mode so that the width decreases. If we need the icons to have
> rectangular boundaries rather than square (ignoring the title), then we
> should add something for that too, rather than making assumptions based on
> window size.
> 
> I still think the correct fix is similar to what I suggest - recalculate the
> width on render, then add resize callbacks where it's embedded to just call
> render. The search app may also need horizontal margins, or to use the
> aforementioned set-icon-width function, I don't know anything about how it's
> laid out at the moment.

I totally agree that we need to make these changes. We have great tests for functionality, but unfortunately we have extremely poor tests for layout and UI. This makes grid changes extremely prone to breaking things (as we have seen in Dale's patch). While I would love to find a long term solution in the future I think that this is a non-trivial and fairly large patch. Trying to do this in the last days of FC is asking for trouble.

> If we need a quick fix, let's go with this, but if we do, let's at least
> open a bug to either replace grid or make it usable under conditions where
> it doesn't occupy the full window width.

Thanks. I'm re-requesting a review of you and Dale - whoever gets to it first. I will open a new bug about the grid layout problems - though I'm wondering if we shouldn't re-examine the architecture with that child manager you were proposing before.

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

There changes are fine by me, I couldnt find any regressions while testing and considered a similiar approach when I was doing the search test.

I agree that it would be best if gaia-grid was contained and sized correctly given the context it is placed in, but mostly for the search app I will be removing its use next time we need to change that layout since its fairly inappropriate to be using it there.

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

Thanks. If we do need to display the same sized icons with similar visual treatment, I think it would probably make sense to re-use some of the icon helpers. I agree that using the entire grid is probably not necessary for search at this point.

Comment 14

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/689ab15d34c8d41182e0f83ebe59832fa4bba9e2

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8588356 [details] [review]
[gaia] KevinGrandon:bug_1150835_gaia_grid_size_constrain > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Believed to be a regression of some homescreen/search layout work, bug 1146825.
[User impact] if declined: Edge casey, but poor UX if you add a bookmark in landscape mode.
[Testing completed]: Manual testing, we don't have proper desktop infra support for an integration test here.
[Risk to taking this patch] (and alternatives if risky): Moderate risk. This is kind of a hot area when it comes to the code, as the layout stuff is quite tricky. That said, existing functional tests are passing, and we've had at least two developers come over a variety of use cases. Maybe let this bake for a day or two on master, but I think it should be safe for uplift.
[String changes made]: None.

Comment 16

[bhavana bajaj [:bajaj]]

Brian, can you get someone to verify this?

Comment 17

[Norry.L.F(Leave from Mozilla)]

Hi Shine,

Could you help verify this bug? thanks.

Comment 18

[Shine(Leave from Mozilla)]

Attachment: Flame3.0_verify_video.MP4

This problem is verified pass on latest build of Flame 3.0 by STR in comment 0.
Actual result: After added bookmark to Home screen, the app icons on Home screen haven't changed into two columns(When the layout of app icons on Home screen is four columns, added bookmark to Home screen, the app icons on Home screen haven't changed into three columns) and the size of bookmark icon you added is normally.
See attachment: Flame3.0_verify_video.MP4
Rate: 0/10

Device:Flame 3.0(Pass)
Build ID               20150406160205
Gaia Revision          834385f4c834238a4306bf87cc4be41615d91ff0
Gaia Date              2015-04-06 19:41:47
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a530b5c3b713
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150406.194015
Firmware Date          Mon Apr  6 19:40:27 EDT 2015
Bootloader             L1TC000118D0

Leaving verifyme for Flame 2.2 uplift & verification.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for Gaia v2.2 approval when you get a chance.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Please nominate this patch for Gaia v2.2 approval when you get a chance.

Nominated in comment 15.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/d8a640b2b1b37801a77bec7b4b7478f218219761

Comment 22

[Shine(Leave from Mozilla)]

This problem is verified pass on latest build of Flame 2.2 by STR in comment 0.
Actual result: After added bookmark to Home screen, the app icons on Home screen haven't changed into two columns(When the layout of app icons on Home screen is four columns, added bookmark to Home screen, the app icons on Home screen haven't changed into three columns) and the size of bookmark icon you added is normally.
Rate: 0/10

Device: Flame 2.2(Pass)
Build ID               20150407162504
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f86ddb7f719
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.195227
Firmware Date          Tue Apr  7 19:52:39 EDT 2015
Bootloader             L1TC000118D0