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

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: azasypkin, Assigned: kats)

Tracking

({regression})

unspecified
mozilla38
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.0 unaffected, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

()

Attachments

(3 attachments, 2 obsolete attachments)

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.
QA Contact: ychung
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Kartikaya, can you take a look at this please? It's possible that the work done on bug 1107280 might have caused this.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(bugmail.mozilla)
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?
Blocks: 928833
Flags: needinfo?(bugmail.mozilla)
See Also: → 1128093
Note that I had a scolling issue in bug 1126777 and I don't reproduce either with a current build.
Hmm, I still see it on build 20150130010210 + bug 1128093 that maybe related. Do you see these issues as well?
Posted file Issue (video)
Video of the issue, build id is 20150201010217
Flags: needinfo?(felash)
triage: based on the sympton it should be blocking. If comment 3 is the case, Julien please help to set this to corresponding component.
blocking-b2g: 2.2? → 2.2+
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.
Assignee: nobody → bugmail.mozilla
Component: Gaia::SMS → Panning and Zooming
Product: Firefox OS → Core
Same for me, I could reproduce (and bug 1128093 as well). But not in v2.2, could you reproduce in v2.2 too, Kats?
Flags: needinfo?(felash)
I haven't tried on 2.2. I'm able to reproduce on master which is good enough for me :)
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.
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.
Posted patch Patch (obsolete) — Splinter Review
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 :)
Attachment #8558575 - Flags: review?(tnikkel)
Attachment #8558575 - Flags: review?(roc)
See Also: 1128093
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.
Attachment #8558575 - Flags: review?(roc) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Posted patch Patch v2 (obsolete) — Splinter Review
Ok, that makes sense, thanks for the explanation! Patch updated accordingly.
Attachment #8558575 - Attachment is obsolete: true
Attachment #8558575 - Flags: review?(tnikkel)
Attachment #8558752 - Flags: review?(roc)
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);
Attachment #8558752 - Flags: review?(roc) → review-
How about
nsIScrollableFrame* scrollableFrame = nsLayoutUTils::GetScrollableFrameFor(aFrame);
if (scrollableFrame) {
  borderBox = scrollableFrame->GetScrolledRect();
}...
?

Or would that not have the right offset?
Posted patch Patch v3Splinter Review
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.
Attachment #8558752 - Attachment is obsolete: true
Attachment #8559353 - Flags: review?(tnikkel)
Attachment #8559353 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/3fbcc923f55d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
Attachment #8559353 - Flags: approval-mozilla-b2g37?
Attachment #8559353 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.