1042887 - Cannot scroll in messages app

Status: VERIFIED FIXED

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

Description

[Julian Schweinsberg]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611075517

Steps to reproduce:

1. Open the "Messages" app.
2. Open a thread which is longer than the screen is high.


Actual results:

You cannot scroll the content of the thread.  The message input is not shown.


Expected results:

The content should be scrollable and the message input should be shown.

Comment 1

[Julian Schweinsberg]

Environment:

Gaia      5458f73e319759543fddf7e96d7ece4d78318e32
BuildID   20140723193600
Version   34.0a1
ro.build.version.incremental=eng..20140505.045022
ro.build.date=Mon May  5 04:51:06 CST 2014

Comment 2

[Martin Shuman [:Marty]]

I also encountered this issue in today's 2.1 build.

Environmental Variables:
Device: Flame Master
Build ID: 20140723040201
Gaia: 01d86c8cc615658694b114ca5711763efc4ef205
Gecko: d0f6259e8446
Version: 34.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.

This issue did NOT occur in the 2.0 build.

Environmental Variables:
Device: Flame 2.0
Build ID: 20140723000201
Gaia: bf3fb88039843359d0a5759b2c0fb787abae544f
Gecko: 57c44a3f83f6
Version: 32.0 (2.0) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 3

[Martin Shuman [:Marty]]

This issue still occurred after disabling APZ.

Comment 4

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
2.0 to 2.1 regression of a core feature.

Requesting a regression window.

Comment 5

[mac]

I can confirm the issue.

Found on:
Alcatel One Touch Fire production (got from T-mobile Poland)
B2G version: 2.1.0.0-prerelease master
Platform version: 34.0a1
Build Identifier: 20140723040201
Git commit info: 2014-07-23 05:51:38 01d86c8c

Comment 6

[Julien Wajsberg [:julienw]]

Could be another regression of bug 1022612.

Waiting for the regression window.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

This breaks usability. It should be a blocker.

Comment 9

[Joshua Mitchell (Inactive)]

Mozilla Inbound Regression Window:

Last Working:
Device: Flame Master
Build ID: 20140722080925
Gaia: 649245c238a043af32acb109b2613f578323f8e1
Gecko: f604cdbc6e85
Version: 34.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken:
Device: Flame Master
Build ID: 20140722082826
Gaia: 649245c238a043af32acb109b2613f578323f8e1
Gecko: b489ff052163
Version: 34.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Last Working Gaia First Broken Gecko: Repro (indicating Gecko issue)
Gaia: 649245c238a043af32acb109b2613f578323f8e1
Gecko: b489ff052163

First Broken Gaia Last Working Gecko: No Repro
Gaia: 649245c238a043af32acb109b2613f578323f8e1
Gecko: f604cdbc6e85

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f604cdbc6e85&tochange=b489ff052163

Comment 10

[Joshua Mitchell (Inactive)]

Daniel - this pushlog is all your work, can you take a look?

Comment 11

[Daniel Holbert [:dholbert]]

Yup, I'll take a look. It's likely we just need a "min-height:0" somewhere.

(That pushlog was primarily about changing the default min-sizing behavior of flex items [which used to default to 0, but now doesn't necessarily], in response to a spec change.  I's expected that it'll break some content that uses flexbox, but hopefully not too much content, and in ways that should be workaroundable.)

Comment 12

[Daniel Holbert [:dholbert]]

I'm trying to reproduce in a desktop b2g build, and I did hit related bug 1043518, but I haven't been able to trigger this bug yet because I can't "view a thread which is longer than the screen is high" (from comment 0) because I can't send any messages, because the send-message button has no effect.

So, I have no threads.

Any way around this? I'm guessing this is because my desktop machine does not have a cell plan :) but I'd think we'd have a way of fake-sending SMS messages for testing... (I did build my gaia profile with DESKTOP_SHIMS=1, if it matters.)

Comment 13

[Julien Wajsberg [:julienw]]

Use a profile generated with DEBUG=1 DESKTOP=0, and load app://sms.gaiamobile.org directly in a firefox nightly using this profile.

But now that we know what the root cause is, we can probably take it from here if you prefer :)

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Use a profile generated with DEBUG=1 DESKTOP=0, and load
> app://sms.gaiamobile.org directly in a firefox nightly using this profile.

Whoah, that is snazzy!

With that, I can verify that adding "min-height:0" on #composer-messages-container fixes this.

Alternately (and perhaps more-correctly), you can move "overflow:auto" from #messages-container and up to #composer-messages-container.  I'd probably lean towards that solution, myself, as long as it doesn't break anything.

(Does #messages-container actually add any value? Perhaps that element should just be removed.)

(As noted in the spec[1], flex items only get special min-sizing behavior if they have overflow:visible (the default).  And in this case, we've currently got a flex item that does have overflow:visible, though it has a child with "overflow:auto; height:100%".  If we bump that overflow:auto up to the flex item (#composer-messages-container), we'll be OK.)

[1] http://dev.w3.org/csswg/css-flexbox/#min-size-auto

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This fixes the problem for me, and per comment 14, I like this a bit better than relying on "min-height:0". (since it more semantically declares that this flex item is allowed to overflow, which tells the container not to bother imposing a special minimum size on it)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix v1 as pull request

Here's a pull request of my attached fix. (Not sure if you're the right person to ask for review; feel free to punt to someone else if not. :))

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8461833 [details] [review]
fix v1 as pull request

I'm one of the right persons but I redirect to Oleg because I'm busy with other blockers these days.

Thanks a lot !

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This makes the phone unusable for dogfooding.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This needs automated testing.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: fix v1 updated pull request, with r=fabrice in commit message

(I got some unrelated commits merged in when trying to edit the commit message in the pull request to note r=fabrice, so I just created a new pull request.)

Comment 21

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/db4fe0b2b4d327e4e219cdebbf4f8377bdc3832e

Comment 22

[Gregor Wagner [:gwagner]]

Julien, can we add a test that this can't regress again?

Comment 25

[Oleg Zasypkin [:azasypkin]]

Reopening this bug since we tracked scroll position for "#messages-container" previously to lazily load messages by chunks. This patch removed "overflow: auto" from this element so scroll-dependent logic is broken now and user will see only first chunk of messages (10).

Probably SMS team can pick that bug from here if Daniel doesn't mind :)

Comment 26

[Julien Wajsberg [:julienw]]

Oleg, can you take care to file a separate issue for adding integration tests and cover at least this use case?

Comment 27

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Oleg, can you take care to file a separate issue for adding integration
> tests and cover at least this use case?

Sure, filed a bug 1043903 for that.

Comment 28

[Oleg Zasypkin [:azasypkin]]

I'll take it as it's dogfooding blocker.

Comment 29

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Julien,

I followed Daniel's advice in comment 14 and removed redundant container. I've played with it in different cases and haven't noticed any other obvious regressions.

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #25)
> Reopening this bug since we tracked scroll position for
> "#messages-container" previously to lazily load messages by chunks.
[...]
> Probably SMS team can pick that bug from here if Daniel doesn't mind :)

Ah, oops. Thanks!

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8462564 [details] [review]
GitHub pull request URL

r=me

Comment 33

[Julien Wajsberg [:julienw]]

master: 77bda216fb4111d9ccfe5abbd26c97a0fd606f2c

Comment 34

[Rachel Pribble [:rpribble]]

VERIFIED FIXED on v2.1.

Environmental Variables:
Device: Flame 2.1 KK (319 MB)
BuildID: 20141011000201 (full flash)
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

The message thread scrolls as expected.