1127773 - [Messages] It's impossible to scroll content inside message input

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Oleg Zasypkin [:azasypkin]]

STR:

* Open Messages app and navigate to New Message (Composer) panel;
* Type some letters into message input;
* Tap on Enter several times so that text typed on step#2 is scrolled up enough and isn't visible (~20 times on Flame);
* Try to scroll up to see your text;

Expected result: user can scroll up to see entire message content;

Actual result: user can't scroll up.

Comment 1

[Yeojin Chung [:YeojinC]]

Mozilla-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame 2.2
BuildID: 20150102135212
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 9689b122bc76
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame 2.2
BuildID: 20150102140811
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 8564f04a6f14
Version: 37.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Last Working Gaia First Broken Gecko: Issue DOES reproduce 
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 8564f04a6f14

First Broken Gaia Last Working Gecko: Issue does NOT reproduce
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 9689b122bc76

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9689b122bc76&tochange=8564f04a6f14

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Kartikaya, can you take a look at this please? It's possible that the work done on bug 1107280 might have caused this.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

If the regression range is right then it's bug 928833. However I can't reproduce this on the latest b2g37 flame-kk-eng build from the pvtbuilds folder, using either a 319MB configuration or full memory configuration on the flame.

Can you provide a video of the issue?

Comment 4

[Julien Wajsberg [:julienw]]

Note that I had a scolling issue in bug 1126777 and I don't reproduce either with a current build.

Comment 5

[Oleg Zasypkin [:azasypkin]]

Hmm, I still see it on build 20150130010210 + bug 1128093 that maybe related. Do you see these issues as well?

Comment 6

[Oleg Zasypkin [:azasypkin]]

Attachment: Issue (video)

Video of the issue, build id is 20150201010217

Comment 7

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

triage: based on the sympton it should be blocking. If comment 3 is the case, Julien please help to set this to corresponding component.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Thanks for the video! I'm able to reproduce this now. I think I just wasn't pressing enter enough times before. I'll look into this.

Comment 9

[Julien Wajsberg [:julienw]]

Same for me, I could reproduce (and bug 1128093 as well). But not in v2.2, could you reproduce in v2.2 too, Kats?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I haven't tried on 2.2. I'm able to reproduce on master which is good enough for me :)

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Dump of frametree, display list, layer tree

Here's a dump of the messages app when this happens. The scrollframe @b2022128 is the thing that's not getting scrolled properly, I think because the blockframe @b2018e28 is sized too short. This blockframe doesn't correspond to a div in the content tree, but it seems to be generated in layout to hold all the text inside. However the height of this div is the same as the scrollframe rather than expanding to encompass the height of its contents. I'm not sure if this is expected or not.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Here's a simplified test case. The mScrolledFrame frame that corresponds to the frame has the same size as the mScrollFrame, even though the scrollable overflow rect encompasses the contents. When we build event regions for this kind of setup, the event regions for the scrollable layer includes the visible rect while scrolled to the top, plus any actual content below. The blank space on the right once you're scrolled down is not included. There's definitely a gecko bug here but I don't know if the expected behaviour is to be using the full scrollable overflow rect or not to use anything at all from the mScrolledFrame for the event regions. If the latter then there's an additional gaia change that will be needed.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Does this make any sense? See analysis in comment 11. mstange said on IRC that the size seems correct, and that the scrollable overflow rect holds the size of the scrolledFrame that we care about here. I'm not sure if the scrollable overflow rect is the same as the size in the general case or what. All I know is that it fixes the bug :)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8558575 [details] [diff] [review]
Patch

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

I don't think we should do this in general. The scrollable overflow rect includes stuff which is not a hit target for this frame.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Here's a simplified test case. The mScrolledFrame frame that corresponds to
> the frame has the same size as the mScrollFrame, even though the scrollable
> overflow rect encompasses the contents.

Right, that's by design.

> When we build event regions for this
> kind of setup, the event regions for the scrollable layer includes the
> visible rect while scrolled to the top, plus any actual content below. The
> blank space on the right once you're scrolled down is not included. There's
> definitely a gecko bug here but I don't know if the expected behaviour is to
> be using the full scrollable overflow rect or not to use anything at all
> from the mScrolledFrame for the event regions. If the latter then there's an
> additional gaia change that will be needed.

With normal event handling, an event that's inside the scrollport but outside the border-box of the scrolled frame and not in the border-box of any descendant content ends up being handled by the scrollframe itself, and since that has the same element as the scrolled frame, everything works fine.

I think here we need to treat the scrollable overflow area of the scrolled frame (but not other kinds of frames) as an event target. So we need a version of your patch that only affects scrolled frames. You can call nsLayoutUtils::GetScrollableFrameFor and check if the result is non-null.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Ok, that makes sense, thanks for the explanation! Patch updated accordingly.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8558752 [details] [diff] [review]
Patch v2

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

::: layout/base/nsDisplayList.cpp
@@ +3111,5 @@
> +    // area corresponding to the overflow rect as well. Otherwise the parts of
> +    // the overflow that are not occupied by descendants get skipped and the
> +    // APZ code sends touch events to the content underneath instead.
> +    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1127773#c15.
> +    borderBox.SizeTo(aFrame->GetScrollableOverflowRect().Size());

GetScrollableOverflowRect() may have a (negative) offset, and you need to take that into account. So something like
  nsRect borderBox;
  if (nsLayoutUtils::GetScrollableFrameFor(aFrame)) {
    borderBox = aFrame->GetScrollableOverflowRect();
  } else {
    borderBox = nsRect(nsPoint(0, 0), aFrame->GetSize());
  }
  borderBox += aBuilder->ToReferenceFrame(aFrame);

Comment 18

[Markus Stange [:mstange]]

How about
nsIScrollableFrame* scrollableFrame = nsLayoutUTils::GetScrollableFrameFor(aFrame);
if (scrollableFrame) {
  borderBox = scrollableFrame->GetScrolledRect();
}...
?

Or would that not have the right offset?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v3

Rewritten using roc's code from comment 17. Flagging tn for review since roc shouldn't review his own code :)

I also tried what Markus suggested and that seems to work as well although I don't know which of the two approaches is more correct.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fbcc923f55d

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3fbcc923f55d

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8559353 [details] [diff] [review]
Patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 928833
User impact if declined: in some cases it's impossible to scroll things past the first "pageful", such as in the STR for this bug
Testing completed: locally
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3ddacd731889

Comment 24

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: The message input box can easily be scrolled.

Environmental Variables:
Device: Flame 3.0 KK (319MB) (Full Flash)
BuildID: 20150305010212
Gaia: eff3321ab4e65da3f906688ebb55ddf1e93d9452
Gecko: 56492f7244a9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2 KK (319MB) (Full Flash)
BuildID: 20150305002528
Gaia: 89af288bad6751248ff84504fa898206fee127fe
Gecko: 6d8d294aa8f3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0