Closed Bug 905273 Opened 7 years ago Closed 7 years ago

[Messages] Recipients list pull down animation is inefficient

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jugglinmike, Assigned: jugglinmike)

References

Details

Attachments

(1 file, 4 obsolete files)

Currently, the Recipients list pull down animation is implemented with a CSS transition over the `height` property. If we instead animate the `transform` property, the animation can be off-loaded to the GPU and will be smoother. This will likely require the addition of `max-height` to ensure that a scroll bar appears when the participants list grows too long.
Attached patch Bug-905273-pulldown.patch (obsolete) — Splinter Review
Hi Julien,

I asked Rick to look over this patch, and he experienced a bug. On his device, the entire screen shifting down when he pulled the recipients list down. I have not been able to reproduce this behavior, but I wanted you to be aware of it in case it occurs during your testing.
Assignee: nobody → mike
Attachment #790866 - Flags: review?(felash)
Rick was kind enough to record a video of the bug he experienced:

http://www.youtube.com/watch?v=I985ai_ofIc

I still haven't been able to reproduce it, though, so I'd appreciate verification from Julien.
Comment on attachment 790866 [details] [diff] [review]
Bug-905273-pulldown.patch

Review of attachment 790866 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good to me ! I love it that the most changes are in css :) And the animation definitely looks a lot smoother !

I can't reproduce Rick's behavior.

However I noticed some discrepancies:

* When there is only one line, the padding-top (or margin-top, I don't know) is a little too big compared to before. But I'm ok with keeping this 
for bug 905260

* when we have too many recipients so that the panel must be scrolled, several behaviors are not correct:
  + when sliding it down it should be scrolled to the end so that we see what we're currently typing. I don't know why/how this works on master but maybe calling scrollIntoView for lastElementChild could be enough ?
  + the padding/margin bottom is now 0 whereas there should be one (I don't have the correct value though, I'll let you chose something and tell you if it's ok after the fact ;) )

In some situations I've noticed we don't see the animation but I think it happens when the device is doing too many things, eg when hiding the keyboard, so I think we can't do much.

::: apps/sms/style/sms.css
@@ +712,5 @@
>    white-space: nowrap;
>    color: black;
>    font-size: 1.465rem;
>    float: left;
>    overflow: hidden;

is it a good moment to better organize these properties ?

@@ +725,1 @@
>    -moz-box-sizing: border-box;

I'd add the unprefixed version of this too. But maybe we can do this for all occurrences in another bug.
Attachment #790866 - Flags: feedback+
> when sliding it down it should be scrolled to the end so that we see what we're currently typing. I don't know why/how this works on master but maybe calling scrollIntoView for lastElementChild could be enough ?

I see that's what we do in recipients.js' visible function already. PRobably the "transitionend" event listener should be added on another element as we don't transition the #messages-to-field anymore.
Attached patch Bug-905273-pulldown-v2.patch (obsolete) — Splinter Review
Hi Julien,

I've addressed the issues that you requested, and I've updated the commit message to reflect this latest version.

>   + when sliding it down it should be scrolled to the end so that we see what
> we're currently typing. I don't know why/how this works on master but maybe
> calling scrollIntoView for lastElementChild could be enough ?

I believe this is a side-effect of transitioning the `height` versus simply setting `max-height` and `overflow-y`. We have logic that governs the scrolling behavior, so a simple modification ensures that we scroll the last child into view in all cases. I can't think of a case where we *shouldn't* scroll the last child in to view, so this seems safe.

>   + the padding/margin bottom is now 0 whereas there should be one (I don't
> have the correct value though, I'll let you chose something and tell you if
> it's ok after the fact ;) )

Resolving this bug required additional (unsemantic) structure. I've included the rationale in an inline comment in the markup to prevent future developers from accidentally "optimizing" this away.

> I'd add the unprefixed version of this too. But maybe we can do this for all
> occurrences in another bug.

This seems like the kind of change we could definitely make across all of Gaia in a dedicated bug. It would also warrant a quick e-mail to dev-gaia, explaining why we're doing it. But first, I'd like to understand :P Why should we include the unprefixed version?
Attachment #790866 - Attachment is obsolete: true
Attachment #790866 - Flags: review?(felash)
Attachment #793089 - Flags: review?(felash)
Blocks: 905260
Comment on attachment 793089 [details] [diff] [review]
Bug-905273-pulldown-v2.patch

Review of attachment 793089 [details] [diff] [review]:
-----------------------------------------------------------------

Mmm there seems to be a slight padding (?) problem: the internal section can be scrolled down by 1 or 2 pixels even if it has enough space to display all the recipients (eg: try with 2 or 3 lines).

Another problem I see: you can very easily get the cursor _inside_ a recipient. I get using the following steps:
* have 2 rows of recipients
* slide down the recipient panel
* tap the message composer to focus it

=> the recipient panel is slid up but the cursor is actually inside the last recipient, and typing the keyboard does nothing.

This does not happen on master.

Could "scrollIntoView" trigger this ? If yes we should file a bug on gecko... On master we don't do "scrollIntoView" if we don't "refocus" and that's probably why we don't have the bug on master :/

Will try to reproduce with a smaller testcase.

::: apps/sms/index.html
@@ +115,5 @@
> +            In these cases, white space described by CSS `padding-bottom` will
> +            not be automatically scrolled into view (because the element will
> +            only scroll far enough to bring the cursor into view). A container
> +            element is necessary to consistently render this vertical space
> +            (instead realized with CSS `margin-bottom`). -->

nit: since it's unsemantic markup, I'd rather have a div

::: apps/sms/js/recipients.js
@@ +602,2 @@
>        state.isTransitioning = false;
>        view.outer.removeEventListener('transitionend', te, false);

you're not removing it correctly here.

better use `this.removeEventListener` or `e.target.removeEventListener` (with the e passed in argument of course)
Attached patch 905273-pulldown-v2.patch (obsolete) — Splinter Review
Thanks for the review, Julien. The cursor and scrolling bugs may take me a little time to work out, so I thought I would get the trivial changes out of the way first. This isn't ready for review; this is just to track my latest work.
Attachment #793089 - Attachment is obsolete: true
Attachment #793089 - Flags: review?(felash)
Attached patch 905273-pulldown-v3.patch (obsolete) — Splinter Review
Hi Julien,

As we discussed in IRC, this patch will not address the cursor bug you described in comment 6.

In order to cleanly address the scrolling bug, I have simplified the rendering of the recipients list. Previously, the `margin-top` and `margin-bottom` attributes of each recipient was used to simulate the `padding` attribute of their parent. I suspect this was the root cause of the superfluous scroll bar. By removing these margins and replacing them with a true `padding-top` on the containing element, the CSS is simplified and the scroll bar is only rendered when necessary.

These box models changes may have resulted in subtle unintended position shifts. The component still renders correctly to my eye, but please tell me if anything looks amiss in this latest patch.
Attachment #793513 - Attachment is obsolete: true
Attachment #793555 - Flags: review?(felash)
Comment on attachment 793555 [details] [diff] [review]
905273-pulldown-v3.patch

Review of attachment 793555 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with adding box-sizing in the correct place and checking this still looks good, as we discussed on IRC
Attachment #793555 - Flags: review?(felash) → review+
Landed on `master` at commit c71f1e95b410e330a81d8bb448a6d52800055f54 (with final change applied)

Thanks for the review, Julien! This was a tricky one
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
After discussing with Corey, I have backed this patch out of `master` with commit fbef8b5c7ff5be9981cff0b58888c763c6855d1d

The patch introduced a regression in the rendering of so-called "zero-width" contacts. I will include a new patch that avoids that regression momentarily.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch avoids a regression introduced by the previous version. At Corey's suggestion, I have included a comment which explains the function of the CSS I had previously removed.
Attachment #793555 - Attachment is obsolete: true
Attachment #793699 - Flags: review?(gnarf37)
Comment on attachment 793699 [details] [diff] [review]
905273-pulldown-v4.patch

carry r=julienw here, interdiff r=me (fixes zero-width editable regression)
Attachment #793699 - Flags: review?(gnarf37) → review+
Landed on `master` at commit 1fd7a8623bb4ba26ae16abc6486c79972e592b52

Thanks for the quick turnaround on this, Corey
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
thanks guys !
Duplicate of this bug: 879785
You need to log in before you can comment on or make changes to this bug.