Closed
Bug 1169589
Opened 8 years ago
Closed 8 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•8 years ago
|
||
That might be a feature (though I'd prefer the old behaviour), WDYT James? Thanks!
Flags: needinfo?(jrburke)
Assignee | ||
Comment 2•8 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•8 years ago
|
Flags: needinfo?(jrburke)
Comment 3•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8613797 -
Flags: review?(m)
Comment 4•8 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•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8613831 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Keywords: checkin-needed
Comment 6•8 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•8 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: 8 years ago
status-b2g-master:
--- → fixed
Resolution: --- → FIXED
Comment 8•8 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•8 years ago
|
||
Updated•8 years ago
|
QA Whiteboard: [MGSEI-Triage+]
Reporter | ||
Comment 10•8 years ago
|
||
Reporter | ||
Comment 11•8 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•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•8 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: 8 years ago → 8 years ago
Flags: needinfo?(bugmail)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•