1119161 - [Email]Tap "Cancel" button and go back to mail list editing view, the mails' checked icon have been cleared.

Status: RESOLVED WONTFIX

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

Description

[Shine(Leave from Mozilla)]

Attachment: logcat

[1.Description]:
[Flame v2.1& 2.2][Email] On Mail list editing view, select some mails and tap "Search mail" -> tap "Cancel" button and go back to mail list editing view, the mails' checked icon have been cleared.
See attachment: logcat_1710.txt & Video3.MP4
Found time: 17:10

[2.Testing Steps]: 
Pre: Log in a mail account on device.
1. Launch Mail.
2. Tap the Edit button in right-bottom corner.
3. Tap "Search mail".
4. Tap "Cancel" button.

[3.Expected Result]: 
4. It go back to mail list editing view, the mails' checked icon shouldn't be cleared.

[4.Actual Result]: 
4. It goes back to mail list editing view, but the mails' checked icon have been cleared.

[5.Reproduction build]: 
Flame 2.1 build:
Gaia-Rev        b04a8cb7b2482e0a44e6702b48c42283a00b5b1e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/99cea2c818f6
Build-ID        20150107001244
Version         34.0

Flame 2.2 build:
Gaia-Rev        69ac77cfa938fae2763ac426a80ca6e5feb6ad25
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/33781a3a5201
Build-ID        20150107010216
Version         37.0a1

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]:
Free test

Comment 1

[Mike Lien[:mlien]]

Hi James, if this is by design?

Comment 2

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

There's definitely an implementation bug as things currently exist.  The current UI logic depends on object-identity consistency for DOM state.  But since header_cursor only ever maintains a single slice, the message_list gets a new slice when the user returns to it from the search UI.  This can lead to some UI-action inconsistency since all of the messages the user previously selected are still actually selected; if the user hits an action button, those messages will still have the action applied.

That's not to say that the specific user flow is one that makes sense or should be supported.  It seems like we should either hide the search text box when we're in edit mode or exit the user from edit mode when we push a search card.  This would be a defensive change since more serious changes aren't really justifiable given the current 3.0 plans.

For 3.0/conversations we could then revisit what makes sense from a UX perspective and possibly include a helper abstraction for some of this.  message_list's selectedMessages is in this weird state because it wants the list both as actual object references (for deciding what the buttons should display based on tallies it computes), as well as a fast object-membership test (the indexOf that breaks and that would be better off being the message's "id" ("suid" on the backend side)).  We could provide a set abstraction that could provide summary statistics for button purposes plus the membership tests.

I'm leaving the needinfo active for James to figure out what he wants to do.  I'm commenting because the set/selection abstraction helper seems like it could be handy and merits further discussion.

Comment 3

[James Burke [:jrburke]]

I can see doing the following 3 changes, not in 2.x, but 3.0/conversations timeframe, except maybe the first one if QA deems this something to avoid in the 2.x timeframe. I think it is edge-casey harder to hit issue, and has existed in a few versions now:

1) Switching to search while in edit mode seems like an incorrect thing to allow. The user is more likely to accidentally hit this than want to actually go to search, and when in bulk edit mode, the focus should be on finishing the bulk edit or exiting, not jumping to different list-based UI. So search should just be disabled in the bulk edit case. I prefer just disabling using some sort of styling to indicate it is disabled vs display: none to avoid jumpy reflows.

2) header_cursor was introduced to reduce the complexity of data object acquisition that needs to be shared between cards, mainly message_list and message_reader, and knowing that depending on startup entry point, message_reader may be instantiated before message_list.

Message_reader's "back" is just at "cards go back one" operation, and it does not know if there is an existing card. cards.js relies on the app using it to provide a cards.pushDefaultCard that pushes the default card.

So that setup does not allow for passing around model data, and I have traditionally not wanted to pass that data around since it makes too much assumptions on the app flow.

However, clearly it is broken if message_list and message_list_search are sharing the same header_cursor object.

So we need a well-known place to get data acquisition that is outside explicit object passing between cards. My first impulse is to maybe leverage the BrowserContext refactoring (bug 937959) to store two different header_cursor instances (and converting header_cursor to just export a new-able thing), one for "default" and one for "search".

For message_reader, allow passing in a headerCursor instance to it, so that message_list_search can pass the one it uses, but if none is passed in, message_reader defaults to grabbing the "default" one from BrowserContext.

There is still some grayness on getting the BrowserContext from a well-known location. I favor using a module ID for that (and if a different app or subset of modules need a different one, using module loader config to configure that), but that can be solved in bug 937959.

I will add some notes to bug 937959 around accounting for multiple header_cursor objects, which can be the longer term fix for this #2 item.

3) The selectedMessages could be different, although I do not think it is burning priority, point 2 is a bigger fish. The design forces for selectedMessages:

We need to know IDs, and we need to know capabilities of the messages (is it read/starred, for example). If selectedMessages was just a set of string IDs, since slices can trim the messages by design, we cannot just use the string ID to look up in a slice and be guaranteed to have a real header object, it could have been trimmed.

Storing actual header objects is nice for the object capabilities, but the indexOf scan can take longer for the "is this header in the selected messages" tests. In practice, given the small UI and no "select all" mechanic, I do not think these tests are the slow part of UI.

I can see a case for creating a SelectedMessage object that uses a quick string ID object test with an array of header objects and manages those objects internally. Or, perhaps it just means using an es6 Map for selectedMessages since it has a "size" and "has" capabilities.

But I think that is only necessary once we get to something where bulk selection happens in more than one place or makes it easier to select lots of messages. Might also be good to wait for a gecko 38 if we want to make sure Set is more settled (pun intended!) -
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla#New_Map_and_Set_objects.2C_and_their_weak_counterparts

----

Summary, maybe track item 1) in this ticket (disable search while in bulk edit), 2) can be tracked in bug 937959, and 3) can be something we keep in mind for the 3.0 in general, maybe as part of the "start using fat arrow functions" and the other es6-ish migrations.

Comment 5

[BMO Automation]

Firefox OS is not being worked on