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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: autra, Assigned: asuth)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.0 unaffected, b2g-v2.1 affected, b2g-v2.2 affected, b2g-master verified)

Details

Attachments

(6 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
That might be a feature (though I'd prefer the old behaviour), WDYT James? Thanks!
Flags: needinfo?(jrburke)
(Assignee)

Comment 2

4 years ago
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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
status-b2g-v2.0: --- → unaffected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Summary: [E-mail] Folder depth calculation is wrong when depth level exceed 1. → [email/backed] Folder depths are wrong because of mismatched argument call lists and type coercion
(Assignee)

Updated

4 years ago
Flags: needinfo?(jrburke)
Created attachment 8613797 [details] [review]
[gaia-email-libs-and-more] asutherland:bug1169589-folder-depths > mozilla-b2g:master
(Assignee)

Updated

4 years ago
Attachment #8613797 - Flags: review?(m)
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.
Attachment #8613797 - Flags: review?(m) → review+
Created attachment 8613831 [details] [review]
[gaia] asutherland:email-bug1169589 > mozilla-b2g:master
(Assignee)

Updated

4 years ago
Attachment #8613831 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Keywords: checkin-needed
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.
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
status-b2g-master: fixed → verified
QA Whiteboard: [MGSEI-Triage+]
(Reporter)

Comment 10

3 years ago
Created attachment 8646200 [details]
gmail_folder_layout.png
(Reporter)

Comment 11

3 years ago
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).
Flags: needinfo?(bugmail)
(Reporter)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

3 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Flags: needinfo?(bugmail)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.