I'm not sure if this is a Gaia or Gecko bug yet but it happens with APZ turned on and does not happens otherwise. Steps to reproduce: - If you have a Gaia without any data (calls, messages, contacts, ..), go into the gaia directory and do |make reference-workload-medium| - Open the sms app - Wait for the thread to be loaded - Click on the thread with the name 'Catherine M.Keller' - Try to pan to the bottom of the view Expected result: - The user is able to scroll to the end of the messages list and see the content of messages Actual result: - The user is able to scroll to the end of the messages list but the end of the last message is overidden by the text input at the end of the list. I think to remember that this list use some margin or padding mechanism to offer a bit more content to scroll at the end. But maybe this is an old information and this has changed now.

It seems like a Gecko issue. The thread list is correctly rendered the first time and as soon as I touch hit with my finger it repaints scrolled to the bottom and then I can't scrolled to the bottom anymore.

The first time it works because the sms code calls https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L637 It I removed this line then we can never scroll to the bottom of the list :/

Milan, do you know who can look at that? It prevents me to dogfood the phone IRL since I can't read my girlfriend's text messages! :)

Interestingly if I hit the home button to push the app to the background and open the app open the message is positioned correctly.

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #4) > Interestingly if I hit the home button to push the app to the background and > open the app open the message is positioned correctly. I don't see any new calls to UpdateSubFrame. Maybe this is a display port mis-positioning.

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #4) > Interestingly if I hit the home button to push the app to the background and > open the app open the message is positioned correctly. Not for me. Otherwise I see exactly what you described.

Here's an initial diagnosis based on testing the program with and without APZ, and looking at the scrolling logic being invoked in APZC: - The element being scrolled extends all the way to the bottom of the page, including behind the text input. Without anything else being in play, this would lead to the bug we're observing both with and without APZ. - In what seems like an attempt to work around the above, some margin or padding is added at the bottom of this scrollable element, to make it so that when you scroll to the bottom, the last message is visible (and it's the margin/padding that's behind the text input). This is consistent with what Vivien said in comment #0. - This margin or padding is being ignored when computing the scrollable rect of the scrollable element for the purpose of APZ scrolling, and as a result the workaround does not work. Vivien, do you know why this margin/padding is being used instead of just sizing the scrollable element so it stops before the text input?

(In reply to Botond Ballo [:botond] from comment #7) > Here's an initial diagnosis based on testing the program with and without > APZ, and looking at the scrolling logic being invoked in APZC: > > - The element being scrolled extends all the way to the bottom of the > page, including > behind the text input. Without anything else being in play, this would > lead to the > bug we're observing both with and without APZ. > - In what seems like an attempt to work around the above, some margin or > padding is > added at the bottom of this scrollable element, to make it so that when > you scroll > to the bottom, the last message is visible (and it's the margin/padding > that's > behind the text input). This is consistent with what Vivien said in > comment #0. > - This margin or padding is being ignored when computing the scrollable > rect of the > scrollable element for the purpose of APZ scrolling, and as a result the > workaround > does not work. > > Vivien, do you know why this margin/padding is being used instead of just > sizing the scrollable element so it stops before the text input? It was used as a workaround before freezing the 1.0.1 version IIRC and there was not other strong reason behind it than pressure. If removing some padding / margin will fix it in Gaia, let's do that for 1.3 and we can fix this bug for 1.4 since it will likely affects web content. How does it sounds ?

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #8) > (In reply to Botond Ballo [:botond] from comment #7) > > Here's an initial diagnosis based on testing the program with and without > > APZ, and looking at the scrolling logic being invoked in APZC: > > > > - The element being scrolled extends all the way to the bottom of the > > page, including > > behind the text input. Without anything else being in play, this would > > lead to the > > bug we're observing both with and without APZ. > > - In what seems like an attempt to work around the above, some margin or > > padding is > > added at the bottom of this scrollable element, to make it so that when > > you scroll > > to the bottom, the last message is visible (and it's the margin/padding > > that's > > behind the text input). This is consistent with what Vivien said in > > comment #0. > > - This margin or padding is being ignored when computing the scrollable > > rect of the > > scrollable element for the purpose of APZ scrolling, and as a result the > > workaround > > does not work. > > > > Vivien, do you know why this margin/padding is being used instead of just > > sizing the scrollable element so it stops before the text input? > > It was used as a workaround before freezing the 1.0.1 version IIRC and there > was not other strong reason behind it than pressure. If removing some > padding / margin will fix it in Gaia, let's do that for 1.3 and we can fix > this bug for 1.4 since it will likely affects web content. > > How does it sounds ? Sounds good.

The problematic "padding" is the 'border-bottom-width' being added to the 'messages-container' <article> element in JS. The attached patch removes the border and decreases the height of the element instead by the corresponding amount. I don't know if it's proper HTML/CSS style to do it like this, but it should give an idea of what needs to be done.

Here is an alternative approach, suggested by Jeff Muizelaar and Mike Conley, which involves using flexbox to achieve the desired layout instead of making adjustments to CSS properties from JS. Vivien, please feel free to pick whichever approach you think is best.

Comment on attachment 8344146 [details] [diff] [review] bug942863-workaround-approach2.patch I see some regressions with this one where the position of the header is wrong, and even wronger when the size of the field is updated because of growing the size of the input field. I prefer this one in theory but I feel like this is more regression prone. SMS needs to be fixed in many ways to avoid reflowing post 1.3.

Comment on attachment 8343962 [details] [diff] [review] bug942863-workaround.patch Let's ask a sms peer.

The plan is definitely to move to flexbox here. The current way of working here is just workarounds piling up for 6 months, giving extra synchronous reflows way too often. The main issue here is that it's difficult, because there are a lot of edge cases. I'll review both approaches monday, but I think moving to flexbox will actually be difficult to do without regressions so it's probably safer to do the other approach in 1.3.

Comment on attachment 8343962 [details] [diff] [review] bug942863-workaround.patch Review of attachment 8343962 [details] [diff] [review]: ----------------------------------------------------------------- Here is a first comment. This could fix bug 917181 in the process. ::: apps/sms/js/thread_ui.js @@ +914,5 @@ > // padding-bottom is ignored with overflow:auto;") > this.input.style.height = newHeight + 'px'; > + this.composeForm.style.height = newHeight + verticalMargin + 'px'; > + this.container.style.height = > + 'calc(100% - 5rem - ' + (newHeight + verticalMargin) + 'px)'; A first thought: I don't like magic values here. It's probably possible to get the '5rem' value from the header style instead. You can get it at init time (like INPUT_MARGIN) because this won't change, so that we don't need to get it at each change.

(In reply to Julien Wajsberg [:julienw] from comment #14) > The plan is definitely to move to flexbox here. The current way of working > here is just workarounds piling up for 6 months, giving extra synchronous > reflows way too often. The main issue here is that it's difficult, because > there are a lot of edge cases. > That's so true :) > I'll review both approaches monday, but I think moving to flexbox will > actually be difficult to do without regressions so it's probably safer to do > the other approach in 1.3. I agree. Thanks for the quick feedback.

I've seen a regression on master on the same area while I was reviewing the first approach, I need to look at this before looking at your patch, sorry.

Bug 947977 is the regression I found, so now I can review this right now.

See bug 875362 comment 10, I remember we discussed about using height, but I don't remember why we didn't choose this solution. Mike, would you remember why? In the mean time I'm fixing the nit and I'll ask r.

see also github PR at https://github.com/mozilla-b2g/gaia/pull/14508

Taking a look right now!

This shows a bigger border in the top of the composer.

Julien! Please remove the border-bottom styles here https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L356 and we will fix the wrong border shown here https://bug942863.bugzilla.mozilla.org/attachment.cgi?id=8345816. Please fix this and I think we are done! Im gonna try to test this further while you're fixing this.

Mmm that's funny, I don't see it in Firefox, only in the device. Debugging.

Borja, the same border happens on master, so this patch is not the cause. I've updated the PR https://github.com/mozilla-b2g/gaia/pull/14508 with the changes you've asked.

Comment on attachment 8344734 [details] [diff] [review] first approach, patch v2 Wait Travis for merging!

I think I found the reason we used `borderBottomWidth` and not `height`--please do not merge this until verifying! Typing up an explanation now...

One requirement of the UI was that, while composing in an existing message thread, the Compose area should be allowed to grow until *just before* it completely covered the underlying thread view. I've verified the following behavior on `master` 1. Open an existing thread 2. Enter the letter "a" 3. Enter ~20 carriage returns into the Compose field 4. Enter the letter "z" 5. Dismiss the keyboard by tapping on some other part of the UI 6. Ensure the (now collapsed) Compose field is scrolled to the very bottom (the "z" character should be visible) 7. Re-open the keyboard by tapping on the empty space next to the "z" character in the Compose field 8. Scroll to the top of the (now expanded) Compose field and view the "a" character On `master`, the following conditions hold true throughout the above interaction: 1. It is always possible to view the Thread UI 2. It is always possible to bring the "a" character into view With this patch applied, both of these conditions are violated after step 7

Sorry for the spam, Julien--I just want to be sure you read comment 28 before continuing with this patch.

(In reply to Mike Pennisi [:jugglinmike] from comment #29) > Sorry for the spam, Julien--I just want to be sure you read comment 28 > before continuing with this patch. Thanks for the spam. I'm seeing some issues too. I will have a look to see if those can be resolved without using the border hack.

Julien is sick, so I'm making a patch inspired by all the stuffs that has happened here. Can you guys tell me if it sounds fine and if it does not regress anything?

Comment on attachment 8346528 [details] [diff] [review] bug942863.try.patch Review of attachment 8346528 [details] [diff] [review]: ----------------------------------------------------------------- I'm back! ;) This adds asynchronous reflow, but I see we're removing one, so... maybe it's identical than before. I have a look now to see if I can come with something better. ::: apps/sms/js/thread_ui.js @@ +965,5 @@ > > + // Update the thread list container height, based on the remaining available > + // space. > + var availableHeight = window.innerHeight - this.HEADER_HEIGHT; > + var composeHeight = this.composeForm.getBoundingClientRect().height; This "innerHeight" + "getBoundingClientRect" will give an additional synchronous reflow :(

(In reply to Julien Wajsberg [:julienw] from comment #32) > Comment on attachment 8346528 [details] [diff] [review] > bug942863.try.patch > > Review of attachment 8346528 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm back! ;) > > This adds asynchronous reflow, but I see we're removing one, so... maybe > it's identical than before. > I have a look now to see if I can come with something better. > > ::: apps/sms/js/thread_ui.js > @@ +965,5 @@ > > > > + // Update the thread list container height, based on the remaining available > > + // space. > > + var availableHeight = window.innerHeight - this.HEADER_HEIGHT; > > + var composeHeight = this.composeForm.getBoundingClientRect().height; > > This "innerHeight" + "getBoundingClientRect" will give an additional > synchronous reflow :( I know. One way to avoid calling getBoundingClientRect() could be to get the height of the input field, and clamped it based on the maxheight that has been defined. That should avoid this reflow.

I can verify that this patch avoids the regression I reported above, and it doesn't seem to introduce any new regressions. I'll leave it to Julien to judge the effectiveness of the specific approach.

Pushed to the existing PR https://github.com/mozilla-b2g/gaia/pull/14508 Rebased to the latest gaia master. No apparent regressions. I tried: * having a big composer * with/without subject * with/without subheader * new message view with lots of recipients * showing/hiding keyboard * enabling/disabling edit mode looks good

Comment on attachment 8346596 [details] [diff] [review] patch v3 Review of attachment 8346596 [details] [diff] [review]: ----------------------------------------------------------------- I really like your code style ;)

Julien, I add to modify a little bit the patch in order to works correctly with APZC. Seems like there is a race with the resize handler, so I wrap it into a nextTick call.

Hmm. Maybe the fact that I need to do that is an other Gecko bug.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #38) > Hmm. Maybe the fact that I need to do that is an other Gecko bug. Seems like we are calling SetCSSViewport too early, and it results into firing a resize with the wrong width/height values (it should be called after http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#105 since this is what is used by http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4441 in order to return the size of the window. Without doing this, the resize event fire with the old width/height values.

Comment on attachment 8346914 [details] [diff] [review] bug942863.resize.patch It nicely break the call screen. So removing r?

This one does not break the call screen.

Hi all! Retesting again the patch with APZ enabled there are a lot of issues doing the following: - "make reference-workload-medium" - Enable APZ - Open "Catherine M.Keller" thread (Sometimes when opening I've found [1]) - Type some text and a lot of carriage returns for getting the scroll - Change the focus tapping on the list of messages - Return the focus to the composer EXPECTED: Resizing is done properly CURRENTLY: [2] & [3] appears, so the height is not updated properly anymore. Vivien, Could you check if this issue is fixed with your patch to "resize.window"? Thanks! [1]https://bug942863.bugzilla.mozilla.org/attachment.cgi?id=8347159 [2]https://bug942863.bugzilla.mozilla.org/attachment.cgi?id=8347160 [3]https://bug942863.bugzilla.mozilla.org/attachment.cgi?id=8347161

Comment on attachment 8346596 [details] [diff] [review] patch v3 Review of attachment 8346596 [details] [diff] [review]: ----------------------------------------------------------------- A lot of issues when APZ mode is enabled... :( Could you check again with Vivien patch? Probably we would need to land Vivien's code in a separate bug and then focus on this one, Wdyt? I'll clean the review flag, please ask me to review this again when APZ mode will be ready! Thanks

Hey Borja, so yeah those are the issues that are fixed by the attached Gecko patch. Can you review Julien's patch with apzc turned off and see if you can find regressions ?

Comment on attachment 8346596 [details] [diff] [review] patch v3 Review of attachment 8346596 [details] [diff] [review]: ----------------------------------------------------------------- I'm removing f- from Borja since those are issues that are fixed by the gecko dependency.

Borja, if you also really want to try with APZ turned on then you can try the Gaia patch at https://bug942863.bugzilla.mozilla.org/attachment.cgi?id=8346874 that normally work around the gecko issue.

Hi Vivien! This bug is only related with APZ, so it does not make sense to test it without APZ (because this is working properly in Gaia master currently). I would suggest to land your patch in Gecko as a separate bug, create the dependency, and when that bug will be landed, I'll review & merge this one. Wdyt?

I admit I haven't tried this with APZ turned on at all. Will do it (with Vivien's Gecko patch as well) and will report if I find anything strange.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #39) > Seems like we are calling SetCSSViewport too early, and it results into > firing a resize with the wrong width/height values (it should be called > after > http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/ > APZCCallbackHelper.cpp#105 since this is what is used by > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow. > cpp#4441 in order to return the size of the window. > > Without doing this, the resize event fire with the old width/height values. Yeah, I ran into this problem on Fennec also, tracked by bug 940889. I almost have a proper fix for that one (just need to test a little more) and if that works then I can apply the same solution to TabChild.cpp. That being said I'm having a hard time following what's going on in this bug.

Comment on attachment 8346952 [details] [diff] [review] resize.window.patch Review of attachment 8346952 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is quite right. Based on my investigation/work on bug 940889, I think a better solution is to ensure that we always call setScrollPositionClampingScrollPortSize when we call setCSSViewport. Currently the helpers in APZCCallbackHelper call the setScrollPositionClampingScrollPortSize which is why your patch probably seems to work, at least in some cases. I'll be working on bug 940889 today and can probably write the patch for B2G as well.

Borja, could you review the patch with azpc disabled? So that we know that it's working in non-azpc environments. Another possibility is to make azpc working with the current code ;)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54) > Comment on attachment 8346952 [details] [diff] [review] > resize.window.patch > > Review of attachment 8346952 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think this is quite right. Based on my investigation/work on bug > 940889, I think a better solution is to ensure that we always call > setScrollPositionClampingScrollPortSize when we call setCSSViewport. > Currently the helpers in APZCCallbackHelper call the > setScrollPositionClampingScrollPortSize which is why your patch probably > seems to work, at least in some cases. I'll be working on bug 940889 today > and can probably write the patch for B2G as well. Yeah I moved it intentionally under ProcessUpdateFrame for this purpose. If you have a patch for it that's perfect. Let's block on bug 940889.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #56) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54) > > Comment on attachment 8346952 [details] [diff] [review] > > resize.window.patch > > > > Review of attachment 8346952 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I don't think this is quite right. Based on my investigation/work on bug > > 940889, I think a better solution is to ensure that we always call > > setScrollPositionClampingScrollPortSize when we call setCSSViewport. > > Currently the helpers in APZCCallbackHelper call the > > setScrollPositionClampingScrollPortSize which is why your patch probably > > seems to work, at least in some cases. I'll be working on bug 940889 today > > and can probably write the patch for B2G as well. > > Yeah I moved it intentionally under ProcessUpdateFrame for this purpose. If > you have a patch for it that's perfect. Let's block on bug 940889. Kats, while you are fixing it. I think this is wrong to call SetCSSViewport in the before first paint handler too. That will cause a resize event on the window, when it may just be resize a bit after. As a result this can cause some extra resive events in the window, and give wrong informations to apps, it will also trigger some media queries. My feeling is that SetCSSViewport should be called only once during startup.

Spoke quickly with kats on IRC and he will fix the gecko issue in this bug.

Comment on attachment 8346596 [details] [diff] [review] patch v3 Review of attachment 8346596 [details] [diff] [review]: ----------------------------------------------------------------- Code looks great! However, as this bug is related with APZ, I would like to ensure that this is working as expected in that scenario. Julien, could you wait until getting Gecko fixed for re-test this and ensure that everything works as expected? Thanks!

So... this depends on 940889, and both need to be done for 1.3?

No, bug 940889 is Fennec-only. It problem is the same but the code paths are different and the patches will be independent of each other. I am working on the B2G equivalent of my patches there and will check to see if it fixes the problem in comments 46.

To simplify tracking I filed bug 950180 for the resize event issue on B2G. I have a patch on that bug that fixes the resize event issue and improves the situation in the SMS app, but doesn't fix it entirely. There might be some additional things that need to be done.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #57) > Kats, while you are fixing it. I think this is wrong to call SetCSSViewport > in the before first paint handler too. That will cause a resize event on the > window, when it may just be resize a bit after. As a result this can cause > some extra resive events in the window, and give wrong informations to apps, > it will also trigger some media queries. > > My feeling is that SetCSSViewport should be called only once during startup. So it's not actually the call to SetCSSViewport itself that triggers the resize event. SetCSSViewport sets some values in layout and only when a reflow is triggered is the resize event flushed out to content. So as long as we don't trigger a reflow it's fine. That being said, yeah we might want to make some changes so that we only call SetCSSViewport once during startup. However I would like to do that in a separate bug as I suspect it won't be as easy as it sounds. We've had similar code in Fennec for a long time now and it was definitely added for a reason; we'd have to track down that reason and make sure it doesn't regress.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62) > To simplify tracking I filed bug 950180 for the resize event issue on B2G. I > have a patch on that bug that fixes the resize event issue and improves the > situation in the SMS app, but doesn't fix it entirely. There might be some > additional things that need to be done. The issues I was seeing go away once I apply the patch on bug 949404. So, to summarize: with the patch from bug 949404 + the patch from bug 950180 + the patch on this bug, the SMS app behaves well with APZC turned on or off.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #64) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62) > > To simplify tracking I filed bug 950180 for the resize event issue on B2G. I > > have a patch on that bug that fixes the resize event issue and improves the > > situation in the SMS app, but doesn't fix it entirely. There might be some > > additional things that need to be done. > > The issues I was seeing go away once I apply the patch on bug 949404. > > So, to summarize: with the patch from bug 949404 + the patch from bug 950180 > + the patch on this bug, the SMS app behaves well with APZC turned on or off. I'm glad that we only need 3 patches to workaround the original issue ;)

Building a gecko with those patches right now because I really want to land this before working on bug 947977.

I'm gonna land this because this specific issue looks good. However there are rendering issues when you attach an image in the composer, and especially if you make the composer scrollable by adding then a lot of content you'll see it's jerky when AZPC is enabled. The image is added as an iframe.

master: b44a75862ebcb651381eabc9c3a3b203d76d63bf

Uplifted b44a75862ebcb651381eabc9c3a3b203d76d63bf to: v1.3: 5f73dbdc1961173f50e6e73ef9e98e7591012b4a