James Burke [:jrburke] Assignee
	Description • 10 years ago

Steps to reproduce:

* Have one account configured already with email.
* Make sure email app is closed/not running.
* From browser, click on a mailto: link. Example test page:
http://jrburke.com/work/gaia/mbad.html
* Tap Back, to exit email Compose, and select Discard

You will go back to the browser app at that point, but the email app should still be running in the background. If you switch back over to email, the message list will be blank.

I expect this to happen with any activity or notification trigger that starts the email app up in either compose or message reader as the first card, and if the user then goes "back" to the message list.

I also believe this to be a regression from bug 796474, the introduction of the virtual scrolling, so will ask for 1.4?

	James Burke [:jrburke] Assignee
	Comment 1 • 10 years ago

This bug was caused by the Cards.pushDefaultCard() behavior in the Cards implementation. mail_common, when finding that removing the current card would result in no cards displayed, calls Cards.pushDefaultCard().

That is implemented by mail_app, which pushes a message_list card. Then, mail_common, once the card has been pushed on the stack, removes the originally targeted card for removal. In these activity/notification cases, it would have been a compose or message_reader card.

So the issue is that the message_list card is pushed into the DOM, but not as the topmost, visible card. When message_list is calls vscroll's setData, it triggers the vscroll init, and vscroll's measurement of this.itemHeight is 0 because the card is hidden, and that causes havoc in the indexAtScrollPosition() method, causing a divide by 0, which leads to sadness.

So the fix was to only call setData() if the card is visible, and to also check if data needs to be set in message_list's onCardVisible() method.

Overall, I like this change because it means the vscroll avoids doing work until it is actually visible, which allows for things like other card animations to complete without the possibility of the vscroll work stealing computing resources.

In my ideal world, some of this could be hidden within vscroll, but in this case it would have mean vscroll knowing about the particulars of when cards are displayed to work, which would make vscroll more coupled to email app-specific code.

	Andrew Sutherland [:asuth] (he/him)
	Comment 3 • 10 years ago

Comment on attachment 8421312 [details]
GitHub pull request

This makes sense but it's not immediately clear to me how this interacts with the captureScreenMetrics() stuff I added for the search case.  (Which was a similar case, but things wouldn't be zero height.)  Would captureScreenMetrics be vulnerable to the same problem if triggered, but is not triggered in this non-search case?

If so, maybe it would be better to make vscroll notice when it is getting gibberish metrics and then defer its logic until the next time it is told to do something, at which point it will try again.  For example, an itemHeight of 0 (or computed style that indicates we are display: none) is pretty ambiguous that our DOM is not actually displayed/rendered/etc.  That seems a little safer to me than depending on our caller to determine whether the vscroll is visible based on global means of its own.  (Of course, captureScreenMetrics still depends on having global knowledge/suspicion of whether the keyboard is/will be displayed.  Although in that case we'll just have the screenHeight wrong, not the itemHeight.  The screenHeight being wrong is somewhat more survivable, especially since we can dynamically update that to be correct based on a resize event.)

	James Burke [:jrburke] Assignee
	Comment 4 • 10 years ago

Comment on attachment 8421312 [details]
GitHub pull request

Turning off review request, will try just resetting init state if _init finds bad measurements, and look at API entry points to see if that holds together, and may update the pull request based on that approach.

	James Burke [:jrburke] Assignee
	Comment 5 • 10 years ago

Replaces previous pull request.

Instead of doing more manual setData+visible tracking in message_list, vScroll instead tries not to do any work unless properly initialized, which means that this.itemHeight and this.innerHeight are non-zero values.

Still need a trigger from message_list when it knows it is visible because otherwise the vScroll may not get any signals from the this.list data that it needs to do work. Without that, the original bug is still there, the list is not displayed until the folder is changed or the app is restarted.

	Andrew Sutherland [:asuth] (he/him)
	Comment 6 • 10 years ago

Comment on attachment 8422043 [details]
GitHub pull request

Fixes the repro and may brain likes this strategy much more since it simplifies the analysis a lot (and the visibility hintings seem good/safe to spam).  r=asuth

	James Burke [:jrburke] Assignee
	Comment 7 • 10 years ago

Merged to gaia master:
https://github.com/mozilla-b2g/gaia/commit/0e1842f32a94dacad7ce742531d051b9b7af4d53

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

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 10 years ago

v1.4: https://github.com/mozilla-b2g/gaia/commit/a741cab432171759833f60616af58db4edde733d

	Edward Chen[:edchen]
	Comment 9 • 10 years ago

[Environment]
Gaia      917174ee3812a43758bf43f7ba5f9416dcdb2ca8
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a3a14fb5ad51
BuildID   20140519164002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140519.201203
ro.build.date=Mon May 19 20:12:10 EDT 2014

[Result]
PASS