800402 - [email] UI: Show the number of unread messages in the message-list header after the folder name

Status: VERIFIED FIXED

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

Description

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

If you are scrolled down and new messages show up in the list as a result of a refresh, their existence may not be obvious.  This may also be relevant for automated periodic synchronization; in that case we will try and generate system notifications, but there will be no additional visual feedback in the UI in the folder.

As part of bug 798708 (which was more about a specific scroll offset bug), I proposed the following options for improving this scenario:

1) Icon in the header.  Works even if we are scrolled waaaay down the folder.
2) Glow at the top of the list in the background or something ambient-ish like that in the list.  Works when we are down the folder, although it may be confusing; varying the glow on distance may not be intuitive.
3) When we the message list is up-to-date (slice.atTop === true) and the newest message is currently visible, have the header show up so no scrolling is required, possibly using animation to make it less jarring to the user.
3b) Do enough so that the lip of the new message is visible.
4) Generate system-notifications in the 'refresh' icon case.  (Currently, all user-triggered synchronizations do not generate system-notifications because it is believed to be redundant.)
5) Use some type of in-app notification, possibly our banner mechanism, to allow the user to jump back up to the top of the list or something like that.

Comment 1

[Casey Yee]

As per IxD docs, we should have the number of new messages indicated on the header with the folder name.

If you receive a new email while far down in the list in your Inbox for example, you should see:  Inbox (1)

This should also be displayed on the folder name in the drawer view as well.

I would also expect that we hear some kind of new email notification sound when a new email is received.

Comment 2

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

(In reply to Casey Yee [:cyee] from comment #1)
> As per IxD docs, we should have the number of new messages indicated on the
> header with the folder name.
> 
> If you receive a new email while far down in the list in your Inbox for
> example, you should see:  Inbox (1)

Can you specify how we should do ellipsis in the IxD for the name + number?  There are going to be folder names wider than the display area, and something will need to get clipped.  Also, what should we do if the number gets really large?  Should we just do 99+, 999+, etc. or always write out the whole number?  For now, 

(I don't see anything on https://wiki.mozilla.org/Gaia/Design/Patterns or https://wiki.mozilla.org/Gaia/Design/BuildingBlocks on ellipses idioms.)

> This should also be displayed on the folder name in the drawer view as well.

Note that we out-of-scoped the periodic scan (using STATUS to get UNSEEN for IMAP) of folders to update these counts for v1, so we (probably) don't want to do this for v1 either since people could be misled into thinking there is nothing new in a folder.

> I would also expect that we hear some kind of new email notification sound
> when a new email is received.

As per my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=811704#c4, I think we really want/need a clear definition of 'new' here.  (Noting that in some cases, like trying to cheaply figure out how many new messages are in a folder without syncing it, we really need to just make do with the number of unread messages since \Recent is entirely unreliable.)

Comment 3

[ayman maat :maat]

[UX-P1] Ok this bug is outstanding and is a major issue because:

1) User actions a CTA (refresh email)
2) System has a positive response to CTA action (new email is in coming)
3) However user has no way of knowing that new email has been incoming since refresh.

-----
I am going to open another relational bug because from a design PoV there is an extended problem: If the user is looking at a list of emails contained in a sub folder and they press refresh there is no way for them to know that new mail has come into the inbox or any other folder. bugs 823466 raised to address this

Comment 4

[ayman maat :maat]

RFI to Steve (VD Lead). Referencing comment #2. Could you assign someone within Visual Design to provide Andrew with the information he is requesting in order to move forward with this bug. His requests are more pertinent to visual design than Interaction Design, but naturally Casey and I will be on hand to deliver further information the visual designer requires.

Comment 5

[Casey Yee]

(In reply to Andrew Sutherland (:asuth) from comment #2)
> Can you specify how we should do ellipsis in the IxD for the name + number? 
> There are going to be folder names wider than the display area, and
> something will need to get clipped.  

Our current ellipse pattern is to use "..." for strings longer than available area.

> Also, what should we do if the number
> gets really large?  Should we just do 99+, 999+, etc. or always write out
> the whole number?  For now, 

999+ is probably as large as we ever want to go.   This would probably be quite rare though i suspect?

> 
> > This should also be displayed on the folder name in the drawer view as well.
> 
> Note that we out-of-scoped the periodic scan (using STATUS to get UNSEEN for
> IMAP) of folders to update these counts for v1, so we (probably) don't want
> to do this for v1 either since people could be misled into thinking there is
> nothing new in a folder.

noted.

> 
> > I would also expect that we hear some kind of new email notification sound
> > when a new email is received.
> 
> As per my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=811704#c4,
> I think we really want/need a clear definition of 'new' here.  (Noting that
> in some cases, like trying to cheaply figure out how many new messages are
> in a folder without syncing it, we really need to just make do with the
> number of unread messages since \Recent is entirely unreliable.)

The "new" email definition I have defined in the bug 811704 is for audible incoming email alerts.   When viewing email folders the numeric tag should represent the number of unread emails.

Comment 6

[Dylan Oliver [:doliver]]

This will be fixed by the user story for notifications -- bug 892521.

Comment 7

[James Burke [:jrburke]]

Visual design is available in the attachments in bug 951070.

Comment 9

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

Juwei, I see from the visual design doc :jrburke referenced in bug 951070 that the "Total Unread Number" is the thing we want to show, consistent with :cyee's comment 5.

However, I want to make sure we all understand that until we implement the back-end infrastructure required to implement bug 858899 the number we can display is "the number of currently synchronized messages on the device in this folder that are (locally marked) unread".  As opposed to "the number of unread messages in the folder on the server including ones we haven't synchronized."

The back-end work required for this bug regardless of what we do on bug 858899 requires us to build and maintain a derived statistic that is the number of unread messages.  This is because if we do bug 858899 we'd probably still want to update that number in real-time as we do things locally.  Also, POP3 has no server we can rely on, so that means it's up-to-us or we need to not support the feature on POP3 (which moves the complexity to the UI, which is probably even worse.)

We already have a clear location to do this in the header add/update/remove methods in mailslice.js' FolderStorage.  The main question is how to efficiently and safely keep the number accurate.  Right now local_do_modtags directly manipulates the header out of the block cache so updateMessageHeader can't efficiently perform a delta computation itself unless we change our header-returning methods to make a copy.

So, options that jump out at me, thinking "aloud":

A) Have calls to updateMessageHeader give explicit hints in the form of { addedFlags, deletedFlags, ... } and have the statistic thing check this and use it.

B1) Make defensive copies of the headers we expose from FolderStorage (so we can diff)

B2) Make (deeply?) frozen, immutable copies of the headers we expose from FolderStorage.  Note that there may currently be some performance impacts with freeze, but we could also only actually do that during tests/debug builds/etc. if that is the case.  (Although an exploding proxy might be better then if we're not confident of our ability to notice all the errors.)
B2a) Callers would need to explicitly call a function that returns a mutable copy and then mess with that (so we can diff, although we could 
B2b) Callers need to call a mutation function with an object that specifies exactly the changes that need to be made to the header.  This would basically amount to the same thing that the hint does above.  This may be desirable for our kill-mutexes bug.

B3) Make wrapped versions of the headers via Object.create(theActualHeader) so that writes are obvious and don't affect the underlying thing.  Practically speaking this won't work since flags are on a list which the trick doesn't work with and is inadvisable.

B4) Use a membrane-style proxy to log all writes.  This is not remotely efficient and never will be efficient and seems crazy-making.

C) Have do_local_modtags be part of our formal API and have it be responsible for poking things as appropriate.  Any other code that wants to modify the tags on a header needs to schedule a job to do it.

Note that we already have an attempt to normalize creation of headers in mailapi/db/mail_rep.js so adding a method there to perform mutation would not be a huge change.


My personal inclination after having typed this is to go with B2b, possibly even without the freezing.  Stop directly manipulating headers, instead call an effectively-atomic operation.  Overall this seems like a good architectural change in support of our kill-mutex goal.  It does make FolderStorage somewhat more complex (or at least larger), but it potentially should simplify some of the existing jobs and their boilerplate.

NB: :mcav/:awiss.  Given all the above complexity, I think this might not be a great early email bug to pursue after all.  It might be better to start with the signature bug (which also promises some back-end hacking, have no fear! :), and do this after I get back, etc.  Especially since I don't think this bug is especially pressing.

Comment 10

[Alex Wissmann [:awiss]]

Attachment: GELAM patch

Comment 11

[Alex Wissmann [:awiss]]

Attachment: Gaia frontend patch

Comment 12

[James Burke [:jrburke]]

Comment on attachment 8442426 [details] [review]
GELAM patch

Flipping off review for now, to avoid psychic pressure for teammates on vacation, and we will reconvene on the bug once we are all back and the other 2.0 bug work is a bit further down the line. 

This is not scheduled for 2.0, and we will need some l10n strings to complete it anyway, which definitely will put it out of the 2.0 window.

Comment 13

[James Burke [:jrburke]]

Comment on attachment 8442428 [details] [review]
Gaia frontend patch

Flipping off review until a little bit later time, see previous comments. I left some review comments for the front end pieces in the pull request.

Comment 14

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

Comment on attachment 8442426 [details] [review]
GELAM patch

(Since there are no unit tests, it's probably better to characterize this patch state as a feedback requests.)  It definitely does seem like the direct manipulation by existing \\Seen-affecting call-sites doesn't end up being that much code, which is good!  The options I discussed in comment 9 were seeming disproportionately expensive for this single feature level, although we are going to have to go down those roads eventually.

Of course, the flip-side for this approach is that by not consolidating the logic into a single place, it means we absolutely cannot skimp on the test coverage.  (Unread counts can be a tricky thing and one that people definitely notice when it becomes incorrect.  This was a big Thunderbird problem for years.)

These are the tests I would like to see for a patch for this:

* User/locally initiated changes to read status.
** Changes due to message reading (through use of modtags)
*** This can probably be covered by adding checks/assertions to test_mutation before and after the local mutation modifications.
** Changes due to (local) message moves
*** test_mutation has some coverage for this already
** Changes due to (local) message (purge) deletion.  Specifically, deleting a message from the trash results in an immediate purge which is distinct from move-to-trash.
*** test_mutation I think have coverage too
*** Probably also in test_mutation too?
* Changes based on server synchronization
** Synchronize and the read status has changed on the server
*** I think the specific sync protocol tests are where we get this coverage from right now, not 100% sure but I don't have time to look at this exactly.
** Synchronize and the message is no longer on the server or at least no longer visible to us.  (For ActiveSync, if a message falls out of the sync filter's time window, it is the same as it being deleted as far as we are concerned.)
*** (We have some code to locally fake deletion in common test code, but it's not appropriate to use for this patch.)
*** Same deal, I think
* Tests for any domain implementation features that would be cross-client and are not simply Gaia phone email app UX decisions and so should be in GELAM
** How do we express on the folder that the folder has simply never been synchronized?

Test architecture-wise for asserting on the unread state, options are:
- to add it into the overall do_refreshFolderView/do_viewFolder dictionary.  If specified, it gets logged or is part of a dictionary that we log/expect.  So existing tests wouldn't need to be instrumented with unread counts (until a regression is caused and we get free-ish coverage by doing that).
- Add an additional check/expect helper or something like that.

:mcav likely has some good thoughts on that, especially since he may ideally how a vision of how that works in the promise-y future.  :mcav is also an invaluable resource for dealing with the whole test infrastructure which is arguably quite non-trivial as these things go.


One other thing I raised in my pull request comments was how to deal with this upgrade-wise.  We definitely do want to deal with this.  Broadly:
* Do our existing blow-away upgrade, but add explicit migration logic for any saved drafts / messages-in-outbox
* Do the thing where each folder info indicates a current version state and we upgrade the folder on demand.
** If we were super fancy, this could include briefly/safely traversing the folder contents on first use to sum up the unread count.
** Alternately we could just use this as a basis for discarding the folder state and recomputing from scratch.

It may make sense to spin much of the upgrade mechanism off into a separate bug since it's a complicated-ish thing.  The argument against that is that to test that code we really need a reason to upgrade.

Comment 15

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

For next review steps once tests/etc. exist, I don't need to be involved in the front-end review, but for GELAM/the back-end, I'd suggest that for the next round let's point the review at :mcav and then once he signs off r? me I'll also do a review pass.

Comment 16

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8442426 [details] [review]
GELAM patch

The backend changes look good to me on the latest PR, with two caveats: There are two POP3 tests which seem to consistently fail now (as :awiss has pointed out to me before), perhaps due to race conditions or something else weird:

test_mailapi_contacts
test_nonimap_sync_general

Both failures appear to be directly related to "delete messages on the server, then refresh" steps. That isn't really something that POP3 supports, so to my eyes it's almost a fluke that they passed in the first place. If I recall correctly, I probably just mashed them into working for POP3 rather than actually fully grokking whether that makes sense or not.

My inclination on those would be to bail out of those specific parts of the tests for POP3, but let's get :asuth's thoughts on that when he gets back.

The latest patch looks good, with only a couple more nitty comments from me. I like the folder_info_rep abstraction. feedback+, when :asuth gets back let's flag him for review.

Comment 17

[Juwei Huang[:juwei]]

I guess the best approach is to make the unread number precisely, but I think it still acceptable if we show the number of currently downloaded message that are locally marked as unread.

Comment 18

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

Comment on attachment 8442426 [details] [review]
GELAM patch

I think you're pretty much there!  Unfortunately, as noted on the pull request, you have run afoul of yet some more horrible technical debt (or at least documentation debt) but I think the next rev will wrap this up.  I want to reiterate how awesome it is that you are digging into this ugly stuff and my hat goes off to you for not throwing your hands up and hacking on a more sane app instead! ;)

I'm marking this r- just for tracking purposes.  I think it was probably just a glitch that you set two r?'s to me and then set one to an r-, but I want to make sure that we're on the same page regarding the review flags.  Their only purpose is to make sure we all understand what the current state of the patch is and we have consistent expectations about next steps.  r? against someone means they should be reviewing it.  r+ means they reviewed it and we're done and probably ready to land (modulo minor fix-ups).  r- means they reviewed it and another round of fixes is required (unless the bug gets marked WONTFIX/etc.) and another review.  There's no need to track history like a pull request previously got an r-.  (When using diffs/patches we do leave the r- on the patch and obsolete it since patches/attachments are immutable.)

(Historically I would sometimes clear an r? flag instead of setting it to r- in order to not discourage the contributor, but especially with the many people looking at bugs and patches in the email app these days, I'm moving to using r- but trying to make sure I express positive feedback.  So in case it's not clear from the above and my other comments, I think you're doing great!)

Please set my r- back to r? :asuth when you get the next rev up.

Comment 19

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

Comment on attachment 8442426 [details] [review]
GELAM patch

r=asuth with the changes in the commit at https://github.com/asutherland/gaia-email-libs-and-more/commit/66d8ab0764d70a4c1200d2d0c0e7d6b39ccda6ef applied to your patch.  You can squash in with your changes or not; I don't need the history segregated.  See dependency notes down at the bottom, though.


One request for future reviews against me is to please avoid squashing or rebasing during the review process unless I explicitly request it.  I kept thinking we were basically at the end of the line, so I didn't mention it during the process.  The benefit I get out of this is that I can just look at the deltas you have made in response to my requests or other bugs you have fixed.  Otherwise I basically need to review the pull request in its entirety from scratch each time.  I think this makes sense, but other people's preferences may vary, so you may need to just chalk this up to knowing and humoring your reviewer. :)

NB: Although it is our explicit gaia repo policy to make sure to squash and rebase before landing, it's fine to likewise accumulate commits during the review process.  Also, for gaia-email-libs-and-more, we do not have the squash policy.  It's largely up to the developer unless the reviewer (probably me) sometimes requests things not to be squashed.  (Usually when there's churn in a patch and I'm worried maybe there was something useful in the history, or when I go and end up doing some review fixups and I want to be able to disentangle the changes I made in case it turns out I got confused/had a different world view.)


My changes are:
- I added some comments for my review/future benefit and slightly tightened the checks so that we check both after the local operation and after the server operation.
- I added some small missing test coverage (see below)
- Fixed a POP3 test bug that your changes to update unread count on header deletion surfaced.  Specifically, the wrong type of object was being passed to the deletion API so the id we wanted to be the number 3 was instead the SUID string '1/2/3' which turned out badly.  This was causing the POP3 test failures in test_nonimap_sync_general and test_mailapi_contacts.


And here I've just broken out my requests from before to make sure they were addressed:

(In reply to Andrew Sutherland [:asuth] from comment #14)
> * User/locally initiated changes to read status.
> ** Changes due to message reading (through use of modtags)

Added in test/unit/test_mutation.js

> ** Changes due to (local) message moves

Added in test/unit/test_mutation.js

> ** Changes due to (local) message (purge) deletion.  Specifically, deleting
> a message from the trash results in an immediate purge which is distinct
> from move-to-trash.

The pull request was missing this, I've added coverage now based on the other coverage you added.  So it's in test/unit/test_mutation.js now too.

> * Changes based on server synchronization
> ** Synchronize and the read status has changed on the server

Added in test/unit/test_sync_server_changes.js (covers IMAP, ActiveSync)

> ** Synchronize and the message is no longer on the server or at least no
> longer visible to us.  (For ActiveSync, if a message falls out of the sync
> filter's time window, it is the same as it being deleted as far as we are
> concerned.)

Covered by the same coverage in test/unit/test_sync_server_changes.js (covers IMAP, ActiveSync)


> * Tests for any domain implementation features that would be cross-client
> and are not simply Gaia phone email app UX decisions and so should be in
> GELAM
> ** How do we express on the folder that the folder has simply never been
> synchronized?

We're not currently dealing with this and that's probably fine.


> One other thing I raised in my pull request comments was how to deal with
> this upgrade-wise.  We definitely do want to deal with this.  Broadly:
> * Do our existing blow-away upgrade, but add explicit migration logic for
> any saved drafts / messages-in-outbox
> * Do the thing where each folder info indicates a current version state and
> we upgrade the folder on demand.
> ** If we were super fancy, this could include briefly/safely traversing the
> folder contents on first use to sum up the unread count.
> ** Alternately we could just use this as a basis for discarding the folder
> state and recomputing from scratch.

After being recently reminded by some corner cutting fallout why it's a bad idea and since I don't think this code is at risk of any meaningful bit-rot, I think we should defer landing this until we have the upgrade problem addressed.  Bug 888955 already has comments and links to some similar discussion to this.  I'll mark that as blocking this.  Realistically, either you will end up implementing this or it might be a bit before someone else can get to it...

Do note there is some bit-rot on this patch.  You will need to rebase and address the conflicts.  They will hopefully be minor.


And now that I think we've got this in hand, it looks like there are outstanding action items from the gaia patch to be addressed, so please do address those.  I'll clear your request against :jrburke for the time being until they are addressed.

Comment 20

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

:flod, :gandalf, Q for you:  For this feature we display the number of unread messages in a header.  If it's >999 we want to show "999+", at least in English.  We're not really sure what the right l10n thing is here.  If this was l20n.js we could leave it up to macro/plural rules, but for Gaia's l10n.js I don't think we have that option and I feel like any guess I make is likely to be wrong.

(My guess is have a string like "one-thousand-or-more" and set it to 999+ and probably no locale needs to change it?  Hardcoding the string '999+' in the code seems likely to end up wrong.  Other options include having a string that we parse as a number to know what our cut-off point should be.)

Comment 21

[Francesco Lodolo [:flod]]

I was discussing this with :stas a few days ago for bug 1022445: I always considered the X+ format to be universally understood/clear, but discovered it sounds unnatural in Polish. My only confusion about this form is if it includes the number or not (i.e. ">999" or ">=999"?). For example, I see that Gmail uses the same pattern (number)+ to display the number of unread messages in the tab.

I guess there will be an issue with space, so something like "more than 999" won't fit. If you make it localizable, you need to be ready to cope with long strings. I also don't think plural rules would be relevant here, since it's not "more than 999 messages", but "more than 999".

The safest solution is probably to have a localizable string "999+" like you proposed, with a clear explanation of what it is, where it's used, and what kind of space constraints it has.

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Localizable string "999+" sounds like a good compromise here.

Comment 23

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

:awiss, :mcav, Juwei, and myself just discussed the header centering issue.  The solution we came up with that seems to work without requiring JS manipulation is using nested flex-boxes.  (Although what we're actually doing doesn't necessarily need to use flexbox; inline-blocks could be used if we can force layout to happen from right-to-left.)  The inner container combines the folder name and the number bubble.  Its sibling node is an element that contains a duplicate copy of the number and is "visibility: hidden;".  And as noted, they are laid out right-to-left.  Everyone gets centered, and once the folder name gets big enough, the extra spacer falls off the row and ends up on the next (hidden) row.  Then the things being centered are just the folder name and the label.

To restate our goals:
- When the folder name is not too big, we want the folder name centered visually on the screen with the number bubble to its right.
- When the folder name (and its bubble) get too big, we want the folder name and its bubble centered as a pair.

Here's the HTML/inline styling I ended up clobbering in with the devtools in collaboration with the other peeps:


<h1 style="margin-right: 0px; margin-left: 0px; display: flex; align-items: center; justify-content: center; font-size: 20px; flex-flow: row-reverse wrap;" class="msg-list-header-folder-label header-label">
  <span style="display: flex;">
    <span style="overflow: hidden; text-overflow: ellipsis; display: inline-block;">Inboxengrubenstein</span>
    <span style="margin-left: 10px; padding: 5px; background-color: blue; border-radius: 13px;">999+</span>
  </span>
  <span style="margin-right: 10px; padding: 5px; overflow: hidden;">fff+</span>
</h1>

Comment 24

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

the back-end parts are now landed, the front-end bits need to be reviewed by jrburke after they're in a review-ready state.  (maybe they are already?).

Back-end landings, also from bug 888955:

landed on gaia-email-libs-and-more/master:
(note, different PR for expediency, all commits correctly attributed, etc)
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/328
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/9a185f67d55de1301c200f54e7fe3bb1d742cd33

landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/23058
https://github.com/mozilla-b2g/gaia/commit/9d60607435e6dc6aec5fedfabc50a2dfdc4c877a

Comment 25

[James Burke [:jrburke]]

Comment on attachment 8442428 [details] [review]
Gaia frontend patch

Just one of us needs to review, and given the small change in this one just related to the front end I will take it.

Comment 26

[James Burke [:jrburke]]

Attachment: count-centering.png

In comment 23, it mentioned working out the UI such that the unread count is off to the right of the folder name, but then centers the name and count bubble together once the title gets "too long".

My impression is that the general desire is to treat the unread as a kind of "badge" next to the folder, something like a superscript[1]. But if the title is too long, keep showing the count, but at this point, since the title and count take up the fully allotted space, center them as a unit.

While I do appreciate the non-JavaScript solution that was worked out in comment 23, looking at it on-screen, it seem to weight the header down to the right, to the point where the asymmetry sticks out. The wider icon for Compose vs the slimmer menu drawer icon contribute to this effect.

Also, as-is, the patch does not quite meet the desired effect (long names just end up leading to clipping of the left side of the folder name and the right side of the bubble). The solution seems complicated, and the phantom element makes me cringe.

So I am wondering if we can just always center the name and count as a unit. The attached image shows what that would look like.

[1] http://en.wikipedia.org/wiki/Subscript_and_superscript

Comment 27

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

:awiss had a follow-up and I included a review fix to improve the tests so we would notice if that fix wasn't there.

landed in GELAM/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/330
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/eb0094338fcbc6e5bd6302998e072068b2d358e3

landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/23132
https://github.com/mozilla-b2g/gaia/commit/fd879952bd9d22afba5c2c78ffe3c62c17ea5be8

Comment 28

[Juwei Huang[:juwei]]

Hi James, consider about your idea so I talked to Fang & Harly.
I think we're fine with centering the name+count as a unit in both short and long folder name if it's easier for implementation.
In the meantime, we are going to align the title in the center of the full-width header (rather than align to the exist space of header) in 2.2. So I think it would be more consistence to put the name and count together in this case.

Comment 29

[James Burke [:jrburke]]

It has taken me a bit longer to review this since we can simplify the markup based on Juwei's feedback, but even before that, we seem to lose the shared/js/font_size_util.js resizing logic by using a flexbox inside the header. I have been trying to figure out a combo that might still work, but no luck so far. What I tried:

h1 element with two spans, no flexbox. Almost works, get some resizing for the first folder name that is inserted, but other names later do not seem to trigger a resize. Since this approach does not use block-like divs with trimming ellipsis overflow, content, even the unread bubble, can be cut off. Also the unread count bubble seems to be higher vertically than what is desired.

At the very least, we should be able to do the following in the pull request:

* get rid of the fake div, just center both things together.
* For the CSS, avoid using !important and adding more things to csslint/xlint. Instead use a more specific CSS selector. The only csslint/xlint increase I want to allow from now on are font-size: 0 things for accessibility, and I want to address that at some point properly.
* Sizes should be in rem and not px. The nice thing is that 10px == 1rem for our layout, so should be an easy conversion.
* We should try not to set a specific font size in the elements, since the shared header.css and shared/js/font_size_util.js do some work for us.

This is what I have gotten to work so far given the above, and using flexbox. So it sacrifices the font_size_util.js resizing logic and uses ... overflow instead, but reliably shows the unread count:

https://github.com/jrburke/gaia/compare/bug800402-email-unread-messages

Diff from existing pull request:

https://github.com/jrburke/gaia/commit/b1d1fdff0b0808e134242d008e6174e6282e7365

I want to do more investigation into the internals of the font resize logic, but if we want reliable display of this count without doing mods to the shared JS, we may need to go for ... trimming in the message list in the interim.

Or, I am open to other ideas if other people have them. I plan on looking at the font resize logic more, but I also encourage awiss to look around, and at the very least start to adjust the approach so that we do not need the fake div.

Comment 30

[James Burke [:jrburke]]

This commit just brute forces the header sizing updates:

https://github.com/jrburke/gaia/commit/e0b0bd5a2c624ae958427ca88a7368a46d5b69c2

It is in the branch I mention in my previous comment.

Let's go that route for now, but I will an open a ticket for font_size_util to see if it makes sense to change that base library. 

The issue is font_size_util uses a MutationObserver listener and sets "childList: true" in the options to the observer. This apparently just watches for changes in the children as far as ordering or adding/removing, and not subtree changes to those child node properties or content. 

Passing "subtree: true" then sees our text changes in our flexbox. However, the listener in font_size_util expects the mutated node to be the h1 element, but with subtree, it is actually the child node. So the lib needs to be sure it has an h1 before doing the sizing work. The patch I worked up for the library:

diff --git a/shared/js/font_size_utils.js b/shared/js/font_size_utils.js
index 52a72f1..228e535 100644
--- a/shared/js/font_size_utils.js
+++ b/shared/js/font_size_utils.js
@@ -105,7 +105,7 @@
     _observeHeaderChanges: function(element) {
       var observer = this._getTextChangeObserver();
       // Listen for any changes in the child nodes of the header.
-      observer.observe(element, { childList: true });
+      observer.observe(element, { childList: true, subtree: true });
     },

     /**
@@ -115,8 +115,13 @@
      * @param {HTMLElement} header h1 text inside header to reformat.
      */
     _reformatHeaderText: function(header) {
+      // Since subtree listening is in effect, find the actual header.
+      while(header && !this.getAllowedSizes(header).length) {
+        header = header.parentNode;
+      }
+
       // Skip resize logic if header has no content, ie before localization.
-      if (header.textContent.trim() === '') {
+      if (!header && header.textContent.trim() === '') {
         return;
       }

With that change, we get resizing, but it is a bit noisy, since it catches the change to the folder name, as well as each change to the unread count. This could be an initial clearing of the count to zero, then another when we update once the count is known.

By brute forcing the call, we can reduce the amount of work since we can coalesce and just call the resize once. The brute force commit above is not very smart about it at the moment, but we at least avoid one extra recalculation since we wait for both the folder name and on unread count update before triggering the resize calculation.

I will work with awiss in IRC to wrap up the change for this ticket, and will comment back once I have filed the ticket for font_size_utils. We can file a follow-on ticket to remove the brute force if font_size_utils ends up taking the change above.

Comment 31

[James Burke [:jrburke]]

Filed bug 1057102 for the possible font_size_utils.js change.

Comment 32

[James Burke [:jrburke]]

Comment on attachment 8442428 [details] [review]
Gaia frontend patch

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/8b2890aabfd5241ae74ac342a0c3280a8bce5d33

from pull request:
https://github.com/mozilla-b2g/gaia/pull/20709

Comment 33

[James Burke [:jrburke]]

Comment on attachment 8442428 [details] [review]
Gaia frontend patch

Juwei: I am sorry, I did not follow the correct procedure for this ticket -- In my eagerness to land before :awiss' last day, I landed it without ui-review+. The changes should now be in the nightly builds, so you can use a nightly build to test how it operates.

If you find issues, then I will file a fixup bug and I will be sure to get the work done before 2.1 ends.

Comment 34

[Juwei Huang[:juwei]]

No problem!
Yet the text size of the folder name somehow smaller than before?

Comment 35

[Juwei Huang[:juwei]]

Need info Fang for confirmation.

Comment 36

[Fang Shih [:fang]]

Hi guys,
I just checked the patch, the current header name size is 21px which is smaller than what we had. I think we need to change back to 23px to match with other apps. And for the longer name we should also stay in 23px as well. Thanks!

Comment 37

[Edward Chen[:edchen]]

[Environment]
Gaia      e424c85eda87a40c0fa64d6a779c3fa368bf770b
Gecko     https://hg.mozilla.org/mozilla-central/rev/daa84204a11a
BuildID   20140824160205
Version   34.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

[Result]
PASS

Comment 38

[Edward Chen[:edchen]]

The hender size is 17px for ".msg-list-header-folder-name" and ".msg-list-header-folder-unread" now, so please make sure what size you want changed. Thanks.

Comment 39

[James Burke [:jrburke]]

Attachment: header23.png

There is a helper script, font_size_utils, that is in use by the gaia apps that resizes the text based on the text length, so that it can fit as much in without needing to truncate with ellipsis.

It appears to be miscalculating the size for the email header. It looks like the flexbox use introduces some textContent whitespace on the outside of the content. I believe this is something that font_size_utils should address, and I filed bug 1060020 to track that fix.

The left side of this screen capture shows how it would look if that bug is fixed. The folder name is in 23px. The unread count has a `font-size: smaller` so that it is not as emphasized as much as the header text. `smaller` was used instead of a hard-coded size to allow the font_size_utils sizing to work correctly for longer folder name text and for the header count to look proportionally smaller relative to that final size determination.

I included what it would look like to keep the folder count at the same size as the folder name in the attached picture, that is the right side of the picture.

I believe the left side is the desired outcome though: for text that comfortably fits in the header, it should be 23px for the folder name and slightly smaller text size used for the font count.

So asking for ui-review? to confirm this is the expectation. If so, then we can use bug 1060020 to track the landing of that fix. Following guidance for this bug from Juwei, just setting ui-review? to the visual designer, :peko.

Comment 40

[Peko Chen [:pekochen]]

Comment on attachment 8480835 [details]
header23.png

Hi James,

Thanks for your work!
I agree with your opinion.
Let's keep left side.
Thanks!