950588 - [Email][V1.3] Cannot get the folder list while you want to move multiple mails

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[William Hsu [:whsu]]

Attachment: WP_20131216_010.mp4

* Description:
  This problem happened on v1.3 build.
  While you have two mail accounts on the email app and try to move multiple mails, email app cannot get folder list.
  Attaching the video (WP_20131216_010.mp4) and log (EmailMovement_20131216.log)

* Reproduction steps:
  1. Launch the email app
  2. Add two mail accounts on email app. ( Suggestion: One is POP3 and the other is IMAP )
  3. Select multiple mails then move mails into trash folder
  4. Switch to the other mail and select multiple mails then move mails into trash folder

* Expected result:
  You can move mails into trash folder

* Actual result:
  E-mail app cannot get folder list

* Reproduction build: V1.3 (Buri)
 - Gaia:     588a3e02c4ace3b3341ba1f6bb7274120b53b2b3
 - Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/031270be3702
 - BuildID   20131212004003
 - Version   28.0a2

Comment 1

[William Hsu [:whsu]]

Attachment: EmailMovement_20131216.log

Comment 2

[Jason Smith [:jsmith]]

Does this reproduce on 1.2?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

This is probably a display issue.  I believe there were recently some visual regressions related to the folder picker, this is likely either them or fallout from the fixes from one of them.  Basically, the fact that we see those divider lines means that we're creating DOM nodes, which means we know about the folders.  The actual names are probably wrapped into overflow space or something like that.  Attaching and using the developer tools would likely show the folder names are there but the bounding boxes have gone wacky/etc.

There is also nothing suspicious in the logs, not that we'd expect anything.

I think all the display issues were new from the 1.3 branch, unlikely to affect v1.2.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Er, let me restate, not the "folder picker", but the value_selector fork used to pick where to move folders to.  This is actually the "folderSelector" mechanism exposed by "Cards" in mail_common.js.

This does appear to be a serious 1.3 issue and I do agree it should be marked blocking.  If this is not present on gaia/master, then we just need to uplift a fix from gaia/master.  Otherwise we need to fix it on both.

QA, please try to reproduce on gaia/master.  If you are able to take a look at the DOM/styling situation in the event of the failure, that could be handy.

Comment 5

[William Hsu [:whsu]]

Hi, Andrew,

Thanks for your suggestion and prompt help!
Needinfo myself.

Have a nice day! :)


Best Regards,
William

Comment 7

[James Burke [:jrburke]]

I originally filed this at bug 948764 and was hoping to get to it this week, but I have duped that bug to this one as it has more QA flags set on it, and I will officially claim it. It fails on master.

In the other bug, it just happens any time we show the folder list before going to the move flow.

Comment 8

[James Burke [:jrburke]]

Attachment: GitHub pull request

The folder_cards.css was applying to the value_selector styles for folder depth, so the bug only shows up if the folder_picker is shown before going to edit (folder_cards.css is loaded when folder_picker is shown).

Doing a quick git blame does not point to obvious point of breakage in folder_cards.css, value_selector.js or shared/switches.css. These styles have been always unspecific, and suspect this could have been broken for a long time, even as early as April 2, 2013.

Only folder_picker and value_selector use these styles, so this specificity does not affect any other cards.

Comment 9

[James Burke [:jrburke]]

Speaking to the above comment: it might be good to confirm with QA when this actually broke, as my initial scan of when it might have happened indicates it could have been broken for a long time. So flagging William to see if he has a 1.2, even a 1.1 or 1.0 phone readily available to confirm how far back the breakage goes.

No worries though if it is not readily available, in either case the severity of the bug stands on its own, and the fix is really straight-forward: just extra CSS specificity.

Comment 10

[James Burke [:jrburke]]

Ran the 1.2 branch and this behavior is not in that branch. However, the indentation of the folder names is not present until the folder_picker is shown. So still an inconsistency in that branch, but at least it is usable.

So something probably related to new value_selector or switch styles is interacting with the margin set by the folder depth classes. Ran out of time tonight, but will debug further. End result though: 1.2 was usable, but master, 1.3 is not.

Comment 11

[James Burke [:jrburke]]

Comment on attachment 8348540 [details] [review]
GitHub pull request

Removing review request until more investigation done: likely will move these styles to the common style sheet once root style mismatch found.

Comment 12

[James Burke [:jrburke]]

Clearing needsinfo for whsu, looks like 1.2 was fine, or at least showed lists, it is an interaction with the newer styles added after 1.2 for value_selector/switches, and the margin indents used for the folder depth styles.

Comment 13

[James Burke [:jrburke]]

Comment on attachment 8348540 [details] [review]
GitHub pull request

Traced the code history more and found the root of the issue. Pull request updated to new fix based on that history.

Background on the issue:

The commit for bug 914892 changed the CSS styles for switches, such that the styles for pack-radio applied to span:after instead of the span:

https://github.com/mozilla-b2g/gaia/commit/59c5f36da1913f9834bcee3ef312575ca1be2077#diff-38

which meant that after that, the span in a pack-radio took up space in the layout in relation to the text that followed it because the span was no longer position:absolute. That, coupled with the margins used for folder depths pushed the text that followed the span under the visible area.

So the fix in the pull request was two-fold:

1) Use display:inline for those spans in email's custom value_selector.css
2) move the folder depth styles to email's common.css

Point 1 restores visibility to the folders in the list since they are effectively zero width and no longer cause a break that hides the text, and point 2 ensures they are consistently indented even if the folder picker is not loaded.

Flagging :eeejay for feedback to make sure the inline span style still fits the screen reader goals for pack-radio for bug 914892. I believe it does, as the custom value_selector.js in email does not use that span for anything, does not insert content for it, but the inline keeps it within the flow of the text that follows it.

Asking :gaye for review now, but will not push any fix to master until :eeejay gives feedback.

Comment 14

[Eitan Isaacson [:eeejay]]

Comment on attachment 8348540 [details] [review]
GitHub pull request

This is fine. The styling is not the issue here, as much as the markup. A friendly, non-a11y, suggestion is to move the text into the span element. That is what the original intent of the styling changes I made were.

Comment 15

[James Burke [:jrburke]]

:eeejay: thanks for the feedback, and looking more at it, it did seem odd that the text was outside the span. 

So I updated the pull request to put the text in the span and confirmed it still works as desired with the rest of the pull request. The display: inline style still allowed the text to appear with the indents. Removing the display:inline change still had problems, so will keep that style change as part of this change request.

Comment 16

[Josh Schmitt [Joshs]]

I am able to reproduce this issue by having one email account and going through the sub-folder e.g. 'drafts' 'trash'

1. Sign into one email account
2. Select move icon
3. Select the folder icon, notice the folders are there
4. Select the drawer
5. Select a sub-folder e.g 'drafts' 'trash'
6. Select the move icon
7. Select the folder icon

Comment 17

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8348540 [details] [review]
GitHub pull request

LGTM.

Comment 18

[James Burke [:jrburke]]

Merged to Gaia Master:
https://github.com/mozilla-b2g/gaia/commit/2c883c3195b3d632f3374221a5d4b9e3bdd579d2

From pull request:
https://github.com/mozilla-b2g/gaia/pull/14737

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 2c883c3195b3d632f3374221a5d4b9e3bdd579d2 to:
v1.3: 99e6900e6c4eaa33285de19133e55cee2e220c2a

Comment 21

[William Hsu [:whsu]]

Thanks for all your help!
I cannot reproduce this bug on latest V1.3 build.
Mark as "VERIFIED"


*Build Information:
 - Gaia      26e8a950f3ac703310ee8efcb741ff2418e9d190
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ecdc275e9741
 - BuildID   20140217004003
 - Version   28.0

* Test Result:
 - Cannot reproduce