Closed Bug 947977 Opened 6 years ago Closed 6 years ago

[Messages] Bug 936370 regressed the scrolling behavior when redacting a multi-line message

Categories

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

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

STR:
* open a SMS-only thread (threads with MMS have other issues)
* tap the composer area to focus it
* press "enter" several times

Expected:
* The scroll move up so that we can still see the last message

Actual:
* The scroll does not move but we can scroll it manually.

This means the "height" is correctly updated, but we miss a call to "scrollViewToBottom". Bug 936370 removed one of these calls, what a surprise ;)

Asking 1.3 as a regression.
QA Wanted - Can we confirm this doesn't reproduce on 1.1 & 1.2?
Keywords: qawanted
QA Contact: jschmitt
The issue does not occur on Buri 1.1 and 1.2. The last message scrolls up automatically so the user is able to see the last message.

Buri 1.1
Environmental Variables:
Device: Buri v1.1 COM RIL
BuildID: 20131209041202
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: 05117f42088f
Version: 18.0
RIL Version: 01.01.00.019.281
Firmware Version: V1.2_20131115

Buri 1.2
Environmental Variables
Device: Buri 1.2 COM RIL
BuildID: 20131209004003
Gaia: f615ae7acb6731d191b3094e10e314bc28359bbb
Gecko: f684b8f159a3
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: V1.2_20131115
Keywords: qawanted
(In reply to Julien Wajsberg [:julienw] from comment #0)
> STR:
> * open a SMS-only thread (threads with MMS have other issues)
> * tap the composer area to focus it
> * press "enter" several times
> 
> Expected:
> * The scroll move up so that we can still see the last message
> 
> Actual:
> * The scroll does not move but we can scroll it manually.
> 
> This means the "height" is correctly updated, but we miss a call to
> "scrollViewToBottom". Bug 936370 removed one of these calls, what a surprise
> ;)
> 
> Asking 1.3 as a regression.

Following the discussion here:
https://github.com/mozilla-b2g/gaia/pull/13512/files#r7601838
There is no need to track the scroll. Use case:
- You are answering to a message of 3 days ago (so there are a lot of messages in the middle) and you want to have the focus on that message for typing your answer. Why do we need to force the scroll for removing that focus and show last message? I would suggest to 'force' the scroll *only* if the composer is empty and you are showing the keyboard. Julien, Wdyt?
Flags: needinfo?(felash)
Actually this use case is covered (I've just tested it), so it's a WORKSFORME.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
(In reply to Borja Salguero [:borjasalguero] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > STR:
> > * open a SMS-only thread (threads with MMS have other issues)
> > * tap the composer area to focus it
> > * press "enter" several times
> > 
> > Expected:
> > * The scroll move up so that we can still see the last message
> > 
> > Actual:
> > * The scroll does not move but we can scroll it manually.
> > 
> > This means the "height" is correctly updated, but we miss a call to
> > "scrollViewToBottom". Bug 936370 removed one of these calls, what a surprise
> > ;)
> > 
> > Asking 1.3 as a regression.
> 
> Following the discussion here:
> https://github.com/mozilla-b2g/gaia/pull/13512/files#0
> There is no need to track the scroll. Use case:
> - You are answering to a message of 3 days ago (so there are a lot of
> messages in the middle) and you want to have the focus on that message for
> typing your answer. Why do we need to force the scroll for removing that
> focus and show last message? I would suggest to 'force' the scroll *only* if
> the composer is empty and you are showing the keyboard. Julien, Wdyt?

When you call "scrollViewToBottom", the scroll is actually done only if "isManuallyScrolled" is false. That means it won't actually scroll when the user is in the middle of the list, like you rightly say.

However, we _must_ do it when the user is at the bottom of the list. _This_ doesn't work.

Reopening. This _is_ a regression.
Status: RESOLVED → REOPENED
Flags: needinfo?(felash)
Resolution: WORKSFORME → ---
triage: 1.3+ regression
blocking-b2g: 1.3? → 1.3+
Attached patch patch v1 (obsolete) — Splinter Review
Github PR is https://github.com/mozilla-b2g/gaia/pull/14670

I want to add a unit test and also see if I can avoid doing some work when the values don't change, but maybe it's not possible to do because of all the dependent code in this area.
Attached patch patch v2Splinter Review
---
 apps/sms/js/thread_ui.js             |  5 +++--
 apps/sms/test/unit/thread_ui_test.js | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)


I tried to add a if-block comparing the old compose height with the new compose height but I couldn't make it work as expected. Therefore I'd rather focus on moving to flex box instead of trying to optimize this more.

Github PR is updated at https://github.com/mozilla-b2g/gaia/pull/14670
Attachment #8347476 - Attachment is obsolete: true
Attachment #8347670 - Flags: review?(schung)
Comment on attachment 8347670 [details] [diff] [review]
patch v2

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

r=me mainly for regression fixing, and I'm ok to focus compose height refactoring in bug 891029. Thanks for quick fixing.
Attachment #8347670 - Flags: review?(schung) → review+
bug 949457 has also been specifically created for converting the Composer to flex layout. I'm adding a dependency!
master: 84998e3106d7b622dfa940959b7ecd682a3c99b9
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Uplifted 84998e3106d7b622dfa940959b7ecd682a3c99b9 to:
v1.3: 524332bdbf7c5575436e5dc3f7cc1248fd7948f0
You need to log in before you can comment on or make changes to this bug.