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?

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

(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?

Actually this use case is covered (I've just tested it), so it's a 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.

triage: 1.3+ regression

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.

--- 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

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.

bug 949457 has also been specifically created for converting the Composer to flex layout. I'm adding a dependency!

master: 84998e3106d7b622dfa940959b7ecd682a3c99b9

Uplifted 84998e3106d7b622dfa940959b7ecd682a3c99b9 to: v1.3: 524332bdbf7c5575436e5dc3f7cc1248fd7948f0