1255127 - Decrease height of bookmark folders to match remote client item height

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

Review commit: https://reviewboard.mozilla.org/r/38981/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38981/

Comment 2

[Chenxia Liu [:liuche]]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38981/diff/1-2/

Comment 3

[Chenxia Liu [:liuche]]

Grisha, this is a tiny review (just dimension changes) that mcomella said he probably wouldn't get to for a while, so I thought I'd send it to you so you can learn a bit about reviewboard :)

Comment 4

[:Grisha Kruglov]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

https://reviewboard.mozilla.org/r/38981/#review35985

page_group_height value for landscape mode is actually the same as page_row_height (from what you were changing away). should we be doing something else for landscape here? otherwise "decrease height" is a bit of a misnomer.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38981/diff/2-3/

Comment 6

[Chenxia Liu [:liuche]]

Looks like that page_group_height was added mistakenly to the wrong file in bug 1129171 - that line should have been added to values-large-land instead just values-land/ so that it would affect just large tablets in landscape mode, not all devices in landscape mode.

Since we have three kinds of height situations (one smaller height for interleaved headers in single-pane phone/portrait mode, one for the the side headers of the dual-pane landscape tablet mode, and one for the page rows which should remain the same in all situations), I added another dimen to clarify that.

Good catch, Grisha!

Comment 7

[:Grisha Kruglov]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

https://reviewboard.mozilla.org/r/38981/#review36683

As far as I can tell, it's doing what you've intended!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d62999417a47

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d62999417a47

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

Approval Request Comment
[Feature/regressing bug #]: Home panels restyling
[User impact if declined]: Less differentiation between groups and links
[Describe test coverage new/current, TreeHerder]: local
[Risks and why]: low, updating dimensions of views, fixes a bug for landscape mode
[String/UUID change made/needed]: none

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728562 [details]
MozReview Request: Bug 1255127 - Decrease height of bookmark folders to match remote client item height. r=grisha

Seems like a row risk fix, taking it. Aurora47+

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Flagging for QE verification around bookmark folder height for mid-Aurora sign offs.

Comment 13

[Chenxia Liu [:liuche]]

STR:
On a phone, or in portrait mode of a tablet (there should be a single column of items, not two), open Bookmarks home panel

Expected: Bookmark folders* should be shorter than the link/history items (and be the same height as the Device lines in the "Remote Tabs" panel). On a phone, rotating to landscape should not cause the height of the folder items to grow. (When clicking into a bookmark folder, the "back" item should also be the same height as the folders, not the history/link items.)

* In order to get bookmark folders, you need to have synced bookmarks with a Desktop Firefox that has bookmarks

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8255638a3ede

Comment 15

[miralobontiu]

Hi,

I was not able to reproduce the issue.

Tested with:
Browser / Version: Firefox Mobile Nightly 63.0a1 (2018-08-19)
Operating System: Oneplus Two (Android 6.0.1)

Thank you!

Also, taking into consideration Comment 14, I will remove the qe-verify+ flag.