Bugzilla

Michael Henretty [:mikehenrty][:mhenretty]

Comment 4

•

10 years ago

Ni :doliver here given this needs urgent attention as it breaks smoketests.

Flags: needinfo?(doliver)

James Burke [:jrburke]

Comment 5

•

10 years ago

I flashed a 2.1 device, and I see the bug, but I am at a loss to see something that email has done specifically to cause this. It is either a change in gecko or maybe something in app window system management? Or maybe in a building block?

Here is what I have checked so far:

The core email app on 2.1 has not had any changes to it in over a week:

https://github.com/mozilla-b2g/gaia/commits/v2.1/apps/email

I first thought that maybe the email cards event interception went awry, which would result in scroll or clicks not being allowed. However, Cards._eatingEventsUntilNextCard is correctly set as false, and taps in the card still work. So for example, tapping on the signature button to edit it, works, and transitions to a new card.

Interestingly, after opening the "Sent Using Firefox OS" signature edit card, then going back to account settings, the account settings can then be scrolled.

I checked the scrollHeight of the scrollregion-below-header element, the scrollHeight of 905 is larger than the getBoundingClientRect height of 489, so it implies it thinks there is content that can be scrolled.

Explicitly adding an overflow-y via WebIDE does not affect the issue.

Looking at the changes in /shared:
https://github.com/mozilla-b2g/gaia/commits/v2.1/shared/

This change sounded vaguely scroll related:
https://github.com/mozilla-b2g/gaia/commit/c91588fbddfe4a15418b2cf51e0792092eaed71c

I tried reverting that locally, as well as this system app change:
https://github.com/mozilla-b2g/gaia/commit/9f1b84c3655128aec5b50523878b7892f69430a0

But it did not fix the issue. However, perhaps code related/around those changes, maybe in the platform too, contribute to the issue?

I am out of time for it tonight, but so far I do not expect an email change caused the issue, perhaps just an interaction with how it does the card work and a recent gecko or system change. A regression range would help.

Flags: needinfo?(doliver)

Updated

•

10 years ago

Whiteboard: [2.1-Daily-Testing] [systemsfe] → [2.1-Daily-Testing]

Pi Wei Cheng [:piwei] (inactive)

Comment 6

•

10 years ago

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141020174936
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 33f348ff0417
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141020182200
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 2d4f4862589d
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=33f348ff0417&tochange=2d4f4862589d

Caused by Bug 1076783.

Note that as comment 2 mentioned, this bug is much LESS severe in trunk. On 2.2 this issue only occurs on the initial swipe on the said page, the issue would no longer occur on the 2nd swipe. Whereas in 2.1 the page doesn't respond to all swipes. I'm not sure what makes the discrepancy between these two branches.

QA Whiteboard: [QAnalyst-Triage?]

Flags: needinfo?(jmitchell)

Keywords: regressionwindow-wanted

James Burke [:jrburke]

Comment 7

•

10 years ago

Moving to Core/Layout to match component on bug 1076783.

Component: Gaia::E-Mail → Layout

Product: Firefox OS → Core

Version: unspecified → Trunk

Martijn Wargers (dead)

Updated

•

10 years ago

Blocks: 1076783

Yeojin Chung [:YeojinC]

Updated

•

10 years ago

No longer blocks: 1076783

Joshua Mitchell (Inactive)

Comment 8

•

10 years ago

If this is a smoketest blocker please ensure that the keywords indicate that.

Blocks: 1076783

Comment 9

•

10 years ago

Caused by Bug 1076783. Can you take a look Timothy

QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]

Flags: needinfo?(jmitchell) → needinfo?(tnikkel)

QA Contact: pcheng

Comment 10

•

10 years ago

Attached file Layer tree dumps from master — Details

Layer dumps from B2G on master. In the first layer dump, the layer 0xae57a400 is the scrollinfo container layer that represents the scrollable area in question. It is (almost entirely) obscured by the PaintedLayerComposite just after it, 0xae57a800. The compbounds height on the scrollinfo layer is 734, but the height of the visible region of the PaintedLayer is 710, so there's a 24-pixel gap at the bottom. If I initiate a scroll in that 24 pixel gap it works fine. If I try to scroll from higher up on the page it doesn't work the first time.

However, the failed attempt at scrolling somehow seems to activate the subframe, so that the PaintedLayerComposite now gets a frame metrics. This allows it to scroll normally on the second scroll attempt.

I'm not sure why the layer tree ends up exactly this way, but this is kind of what I was worried about with bug 1076783 and what I was doing sanity tests for. I wasn't able to construct a case where this happened but clearly it does.

Comment 11

•

10 years ago

I'm assuming that on 2.1 the failed attempt at scrolling doesn't activate the subframe, for whatever reason, and so the subsequent scroll attempts fare no better than the first.

Comment 12

•

10 years ago

Attached file Layer dump with multi-layer-apz disabled (obsolete) — Details

This is the layer dump with layout.async-containerless-scrolling.enabled set to false.

Comment 13

•

10 years ago

Attached file Layer dump with multi-layer-apz disabled (v2) — Details

Same as before, but now also includes the layer dump after doing the scroll. There is an extra APZ in this one.

Attachment #8510554 - Attachment is obsolete: true

Comment 14

•

10 years ago

Attached file Display-list dump with multi-layer-apz enabled (master) (obsolete) — Details

This is the last bit of output after I clicked on the email account and it painted the email settings screen. I full log was a >1meg so I just kept the last few layer/displaylist dump iterations.

Comment 15

•

10 years ago

Attached file Display-list dump with multi-layer-apz disabled (master) (obsolete) — Details

Ditto

Comment 16

•

10 years ago

Ok, I don't know what's going on here. Now when I load it with multi-layer-apz disabled I still see the scrollinfo layer. I'll attach the full dumps of everything that gets output between my clicking on the email account and the final render of the settings for that email account.

Comment 17

•

10 years ago

Attached file Display-list dump with multi-layer-apz enabled (master) — Details

Attachment #8510591 - Attachment is obsolete: true

Comment 18

•

10 years ago

Attached file Display-list dump with multi-layer-apz disabled (master) — Details

Attachment #8510592 - Attachment is obsolete: true

Comment 19

•

10 years ago

A brief history of the relevant layer subtree...

(A) In the olden days (2.0) we had:

ContainerLayerComposite
  ThebesLayerComposite
  ContainerLayerComposite    APZC (4,2,7)    cb=Rect(0,75,480,743) (scrollinfo)
  ThebesLayerComposite

In this case the scrollinfo layer was obscured by another layer, but this was before bug 973096 landed so it didn't matter because we didn't check for obscured-ness.

(B) On a central build from 2014-07-29, just after bug 973096 landed, I see a layer tree that looks like this: (dimensions are a little different because I used a hamachi for this one)

ContainerLayerComposite
  ContainerLayerComposite    APZC (3,2,7)    cb=Rect(0,50,320,406) (scrollinfo)
  ThebesLayerComposite
  ThebesLayerComposite

That is, the scrollinfo layer magically moved to the top, and it is no longer obscured, so hit-testing worked fine.

(C) On master with multi-layer-apz DISABLED, the layer tree looks like this:

ContainerLayerComposite
  ContainerLayerComposite  APZC (5,2,8)    cb=Rect(0,75,480,734) (scrollinfo)
  PaintedLayerComposite
  PaintedLayerComposite

Same as before, so again hit-testing works fine since the scrollinfo layer is not obscured.

(D) On master with multi-layer-apz ENABLED, the layer tree looks like this:

ContainerLayerComposite
  PaintedLayerComposite
  ContainerLayerComposite  APZC (5,2,7)    cb=Rect(0,75,480,734)   (scrollinfo)
  PaintedLayerComposite

and this fails because we check for obscured-ness and the PaintedLayerComposite on top of the scrollinfo layer obscures it.

(E) On master, prior to landing bug 1076783 I assume the scrollinfo layer was still on top of everything, and so again it worked fine.

---
It's an open question as to what changed between A and B that caused the scrollinfo layer to move up.

It's also an open question which is the more "correct" layer tree structure. Thinking about what the APZ code does at the moment, it expects the scrollinfo layer to be not necessarily at the "top" or the "bottom", but in the correct position in z-order so that it sits just on top of the layer with the content. Perhaps the real problem in bug 1076783 was that the root process scrollinfo layer was in the wrong position. That is, the "fullscreen" layer tree from attachment 8507380 [details], looked like this:

[start]
ContainerLayerComposite
  ContainerLayerComposite
    ContainerLayerComposite   APZC (1,3,3) (scrollinfo)
    RefLayerComposite
      ContainerLayerComposite APZC (5,2,3) (child process)
        ImageLayerComposite
        ColorLayerComposite
        PaintedLayerComposite
        PaintedLayerComposite
    ContainerLayerComposite
      ColorLayerComposite
      PaintedLayerComposite
    PaintedLayerComposite
  ColorLayerComposite
  PaintedLayerComposite
[end]

The scrollinfo layer is generated because of the scrollgrab attribute set on an element in the parent process, but it's not clear to me why that layer is positioned where it is. In particular, I would expect a scrollgrabbing layer to be an ancestor of the thing it's grabbing, and that's not the case here.

---
Putting all that aside for the moment, tn suggested a solution for now which involves modifying the APZ hit test code so that it treats scrollinfo layers (with no displayport) as always unobscured. So based on the layer trees above, I think it would fix this bug but quite possibly regress bug 1076783, because the (1,3,3) scrollinfo layer above would not be obscured by the reflayer and its contents. I'll give that a shot though, it should be pretty easy to hack up.

Comment 20

•

10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> So based on the layer trees
> above, I think it would fix this bug but quite possibly regress bug 1076783,
> because the (1,3,3) scrollinfo layer above would not be obscured by the
> reflayer and its contents. I'll give that a shot though, it should be pretty
> easy to hack up.

Actually it turns out that does work, because the (1,3,3) APZ ends up as a sibling of the (5,2,3) APZ and the (5,2,3) one is "on top". So I guess this might be a viable solution if we don't find anything better.

Comment 21

•

10 years ago

Attached patch The workaround (WIP) — Details — Splinter Review

Assignee

Comment 22

•

10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> It's an open question as to what changed between A and B that caused the
> scrollinfo layer to move up.

Bug 1041510. It was a regression fix for bug 1022612 that came shortly after.

Assignee

Comment 23

•

10 years ago

Attached patch put scroll info layers belows positioned descendants that aren't scrolled by the associated scroll frame — Details — Splinter Review

The reason that bug 1076783 happened was because the scroll info layer was on top of fixed position content because when placing the scroll info layer we just checked if there was anything on the positioned descendants list and then placed it on top of those. This is a bad idea when those positioned descendants aren't scrolled by our scroll frame.

I think we should also ignore all scroll info layers for the purposes of hit testing if there is a non-scroll info layer for the same scroll id. Because if we have the actual scrolled content we should hit that, not the scroll info layer, which just blindly covers the whole scroll frame area.

Flags: needinfo?(tnikkel)

Comment 25

•

10 years ago

So your patch to reposition the scrollinfo layer works great, it fixes both bugs (this one and the youtube one) and your description of it makes sense to me conceptually. As discussed I think we should get it landed. If we need to I can also write a patch to ignore the scroll info layers for hit testing if there is a non-scroll info layer for the same scroll id, but it should really only matter in the case where the scrollinfo layer is in the root process (such as it was in the youtube case).

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

Assignee

Comment 26

•

10 years ago

Comment on attachment 8510982 [details] [diff] [review]
put scroll info layers belows positioned descendants that aren't scrolled by the associated scroll frame

Turns out we need scroll info layers on top because they don't get hit otherwise. The original problem was a scroll info layer being put on top of fixed pos content, which doesn't make sense since our scroll frame does not scroll it. In this patch we only put the scroll info layer on top of positioned descendants that we scroll. There are still cases that will not get the ideal behaviour, but given that the bug 1076783 was the only problem we've found since multi-layer-apz landed with having scroll info layers on top (and that was caused by/complicated by the fact that the scroll info layer was in the parent process), it should be pretty rare.

Attachment #8510982 - Flags: review?(roc)

Updated

•

10 years ago

Attachment #8510982 - Flags: review?(roc) → review+

Assignee

Comment 29

•

10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/853cd6236161

Updated

•

10 years ago

blocking-b2g: 2.1? → 2.1+

Carsten Book [:Tomcat]

Comment 30

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/853cd6236161

Assignee: nobody → tnikkel

Status: NEW → RESOLVED

Closed: 10 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla36

Ryan VanderMeulen [:RyanVM]

Updated

•

10 years ago

Blocks: 1089620

Comment 31

•

10 years ago

Please request b2g34 approval on this when you get a chance (though it sounds like we may want to wait on bug 1089620).

status-b2g-v2.2: affected → fixed

status-firefox34: --- → wontfix

status-firefox35: --- → wontfix

status-firefox36: --- → fixed

Flags: needinfo?(tnikkel)

Comment 32

•

10 years ago

I think the state is that uplifting this should fix bug 1089620 (which so far has only been seen on 2.1) so we should go ahead and request uplift here.

Assignee

Comment 33

•

10 years ago

Comment on attachment 8510982 [details] [diff] [review]
put scroll info layers belows positioned descendants that aren't scrolled by the associated scroll frame

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 #): bug 1076783
User impact if declined: can't scroll in some situations
Testing completed: on m-c, tested locally by kats
Risk to taking this patch (and alternatives if risky): this patch moves scroll info layers up. before bug 1076783 they used to be even higher. but that caused bug 1076783. so this patch puts them near the top, but slightly lower (below out of flow stuff that isn't scrolled by the scroll frame). Since we've had this behaviour for quite a while and only found bug 1076783 this should be the best compromise.
String or UUID changes made by this patch: none

Flags: needinfo?(tnikkel)

Attachment #8510982 - Flags: approval-mozilla-b2g34?

Updated

•

10 years ago

Keywords: verifyme

Comment 34

•

10 years ago

Comment on attachment 8510982 [details] [diff] [review]
put scroll info layers belows positioned descendants that aren't scrolled by the associated scroll frame

:njpark, can you please help or assign someone to do some exploratory testing here around scrolling. Thanks!

Attachment #8510982 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+