Closed
Bug 1169589
Opened 10 years ago
Closed 10 years ago
[email/backed] Folder depths are wrong because of mismatched argument call lists and type coercion
Categories
(Firefox OS Graveyard :: Gaia::E-Mail, defect)
Tracking
(b2g-v2.0 unaffected, b2g-v2.1 affected, b2g-v2.2 affected, b2g-master verified)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | affected |
b2g-v2.2 | --- | affected |
b2g-master | --- | verified |
People
(Reporter: autra, Assigned: asuth)
Details
Attachments
(6 files)
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•10 years ago
|
||
That might be a feature (though I'd prefer the old behaviour), WDYT James? Thanks!
Flags: needinfo?(jrburke)
Assignee | ||
Comment 2•10 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•10 years ago
|
Flags: needinfo?(jrburke)
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8613797 -
Flags: review?(m)
Comment 4•10 years ago
|
||
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+
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8613831 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-master:
--- → fixed
Resolution: --- → FIXED
Comment 8•10 years ago
|
||
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•10 years ago
|
||
Updated•10 years ago
|
QA Whiteboard: [MGSEI-Triage+]
Reporter | ||
Comment 10•10 years ago
|
||
Reporter | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•10 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
Closed: 10 years ago → 10 years ago
Flags: needinfo?(bugmail)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•