1169589 - [email/backed] Folder depths are wrong because of mismatched argument call lists and type coercion

Status: RESOLVED FIXED

Description

[Augustin Trancart [:autra]]

Created attachment 8612803 [details]
20150529_email_folder_depth.png

Device: Flame
Build id: 62025ac4
email domain: gmail.com

Steps:
- make a bunch of nested labels in your gmail account: test1 is top level. test2 is a child of test1. test3 is a child of test2... test8 is a child of test7
- Configure your gmail account.
- Click on the drawer icon and observe the folders hierarchy

Expected:
A nice tree that displays 6 levels of depth (there are five .fld-folder-depthn css class + one .fld-folder-depthmax class)

Actual:
We only have depth0, depth1 and depthmax, see screenshot

Comment 1

[Augustin Trancart [:autra]]

That might be a feature (though I'd prefer the old behaviour), WDYT James? Thanks!

Comment 2

[Andrew Sutherland [:asuth]]

Absolutely a bug, thank you for filing it!

So this is somewhat funny in a regrettable way.  It's a regression from bug 885110 when we upgraded to the email.js libraries so this is in v2.1 onwards.

This change happened:
-    function walkBoxes(boxLevel, pathSoFar, pathDepth, parentId) {
+    function walkBoxes(boxLevel, pathDepth, parentId) {

But this call kept its four arguments like so:
  walkBoxes(boxesRoot.children, '', 0, null);

The funny bit is that pathDepth ends up taking on the following values:
'' => '1' => '11' => '111', etc.

(Although unary encodings of numbers have their places, this is not one of them. ;)

Fix and test coverage for trunk coming up.

Comment 3

[GitHub Autolander Bot]

Created attachment 8613797 [details] [review]
[gaia-email-libs-and-more] asutherland:bug1169589-folder-depths > mozilla-b2g:master

Comment 4

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8613797 [details] [review]
[gaia-email-libs-and-more] asutherland:bug1169589-folder-depths > mozilla-b2g:master

Hooray for new test and all of the little fixes, as well as the detailed docs and fix for this.

Comment 5

[GitHub Autolander Bot]

Created attachment 8613831 [details] [review]
[gaia] asutherland:email-bug1169589 > mozilla-b2g:master

Comment 6

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#XNMKM9rdRumDDo7Iykvs0Q

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 7

[Andrew Sutherland [:asuth]]

The treeherder stuff from earlier was green.  Taskcluster seems like it's freaking out due to standard fare npm/marionette explosions.

landed:
https://github.com/mozilla-b2g/gaia/pull/30357
https://github.com/mozilla-b2g/gaia/commit/5c0344c91c27e8b651bc2b1c72b195a75ce3186f

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/390
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/a192307cd275b265b3457c2a0901c40d9fe41b8b

https://github.com/mozilla-b2g/mail-fakeservers/pull/33
https://github.com/mozilla-b2g/mail-fakeservers/commit/62f702b8cab54ee44054e8755fecf4ea09a0a12c

Comment 8

[Shally Li（leave from Mozilla）]

This bug has been verified as pass on latest Nightly build of Flame v3.0 and Nexus 5 v3.0 by the STR in Comment 0.

Actual results: The 7 levels of depth display normally in Email.
See attachment: verified_v3.0.png
Reproduce rate: 0/6


Device: Flame v3.0 build(Pass)
Build ID               20150604160205
Gaia Revision          e0fbadeb78a96137f071d9be7a47ef9fe882d17f
Gaia Date              2015-06-04 07:44:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5b4c240e1a36
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150604.192904
Firmware Date          Thu Jun  4 19:29:16 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 v3.0 build(Pass)
Build ID               20150604160205
Gaia Revision          e0fbadeb78a96137f071d9be7a47ef9fe882d17f
Gaia Date              2015-06-04 07:44:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5b4c240e1a36
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150604.192430
Firmware Date          Thu Jun  4 19:24:48 EDT 2015
Bootloader             HHZ12f

Comment 9

[Shally Li（leave from Mozilla）]

Created attachment 8615774 [details]
verified_v3.0.png

Comment 10

[Augustin Trancart [:autra]]

Created attachment 8646200 [details]
gmail_folder_layout.png

Comment 11

[Augustin Trancart [:autra]]

Created attachment 8646203 [details]
Wrong folder layout in fxOS

Reopening this because the layout is still wrong. Attaching gmail_folder_layout.png which shows how it displays in gmail (this is correct btw) and the way it is currently displayed in FxOs.

I don't understand the annotations on Shally Li's screenshots. Are some levels grouped on the same indentation? That would seem horrible to me... Anyway, even if it is the case, the shift should be consistent across all levels (which is not the case here).

Andrew, do you think you can do something about it? It might not be the same bug though (it looks like a combination of wrong level calculation and wrong css here).

Comment 12

[Andrew Sutherland [:asuth]]

Thanks for following up!  Good news: It's not the same bug, this is a (different) CSS bug.  As a general FYI, to avoid confusion we usually don't reopen a bug unless it has been backed out or it's right after the initial fix landed and it's clear it didn't fix things.

If we look at the CSS starting at https://github.com/mozilla-b2g/gaia/blob/master/apps/email/style/folder_cards.css#L286 we can see that:
- There's probably a typo on .fld-folder-depth4.  Based on the pattern, it wants to be 7.5rem but it's 6.5rem.
- There's not a huge difference between depth5 and depthmax, I'm not sure that's a big deal, but...
- There's some weird inconsistencies compared with the folder-picker variant at https://github.com/mozilla-b2g/gaia/blob/master/apps/email/style/common.css#L331 where we go up to 11rem.

We could add more folder depth levels with a complementary JS change.  The main concern with that is that we need to balance showing more visual depth with truncating folder names.  To some extent I would expect longer folder names to be a more common problem than very deeply nested folders.  (My impractical solution would be to use a logarithmic indentation level so we can still maintain some ability to differentiate, but I think that would look weird at lower levels.)

It's my bedtime, so if you could file a new bug, that would be great.  Even better is if you have some suggestions!  Note that we may still need to support 320x480 screens.  (However, I suppose we could have some minimal dynamic logic that decides how many folder classes to use rather than just using the fixed list we currently use.  Note that fld-folder-depthmax is just an arbitrary string in a hardcoded list.  (https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/folder_depth_classes.js is used by https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/folder_picker.js#L272)  It could have been called "fld-folder-puppy" just as easily.