Closed Bug 1096513 Opened 9 years ago Closed 9 years ago

[Edit Mode] Homescreen shows through when over scrolling in edit mode.

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla38
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: smiko, Assigned: cwiiis)

References

(
URL
)

Details

(Keywords: polish, Whiteboard: [2.2-Daily-Testing] [systemsfe])

Attachments

(7 files, 1 obsolete file)

overscroll.txt
11.17 KB, text/plain
Details
Layer dump
5.77 KB, text/plain
Details
Disallow underscroll
7.51 KB, patch
Details | Diff | Splinter Review
Disallow underscroll by transforming overscroll v2
10.35 KB, patch
botond
: review+
Details | Diff | Splinter Review
Follow-up to fix an incorrect assertion
1.02 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
Patch + follow-up rolled up
13.17 KB, patch
cwiiis
: review+
bajaj
: approval-mozilla-b2g37+
Details | Diff | Splinter Review
verify_video.MP4
1.61 MB, video/mp4
Details
Attached file overscroll.txt — 
Description: Over scrolling in edit mode allows the homescreen to be displayed. 

Repro Steps:
1: Update a Flame to 20141110040206.
2: Long tap an app icon to enter edit mode.
3: Pull the screen down or up all the way.

Actual: The over scroll bounce displays the homescreen.

Expected: The over scroll bounce remains on edit mode.

Device: Flame 2.2 (319mb/shallow flash)
BuildID: 20141110040206
Gaia: 5f8206bab97cdd7b547cc2c8953cadb2a80a7e11
Gecko: d380166816dd
Gonk: Could not pull gonk.  Did you shallow Flash?
Version: 36.0a1 (2.2) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Repro frequency: 5/5

See attached: logcat

Video clip: http://youtu.be/DgB9_oXMfQQ
This issue does NOT occur on Flame 2.1 (319mb/shallow flash) as the over scroll feature does not exist. 

Device: Flame 2.1
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

_______________________________________________________________________________________________________________

This issue DOES repro on Friday's Flame 2.2 (319mb/shallow flash)

Actual result: The over scroll bounce displays the homescreen.

Device: Flame 2.2
BuildID: 20141107073659
Gaia: 779f05fead3d009f6e7fe713ad0fea16b6f2fb31
Gecko: b62ccf3228ba
Version: 36.0a1 (2.2)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
URL: http://youtu.be/DgB9_oXMfQQ
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: --- → affected
Flags: needinfo?(pbylenga)
Whiteboard: [2.2-Daily-Testing] [systemsfe]
Not a regression, need info on QA lead for Window Management for nomination decision.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Component: Gaia::Homescreen → Gaia::System::Window Mgmt
Flags: needinfo?(pbylenga) → needinfo?(gchang)
NI Dev to look at this issue.
Flags: needinfo?(gchang) → needinfo?(kgrandon)
Botond - any idea on what the best way to address this is?
Flags: needinfo?(kgrandon) → needinfo?(botond)
Attached file Layer dump — 
Here's a layer dump from when it happens. It's not clear to me why it's happening. My first guess is that the regular homescreen background is in layer 0xaafb6400 which is marked as fixed-position, so it doesn't compress with the rest of the stuff under 0xaaf24c00. There must be something else going on with clipping too though.
Oh, I think I know what's going on. The ColorLayer at 0xaafb8800 which sits between the regular homescreen and the edit-mode homescreen is too short. It gets sized to the visible area rather than the displayport. It's also fixed-position which is why it stays put while scrolling.
blocking-b2g: --- → 2.2?
Component: Gaia::System::Window Mgmt → Panning and Zooming
Product: Firefox OS → Core
Version: unspecified → Trunk
Yeah, I suppose the new overscroll effect is breaking new ground in that it's compressing the rendered content at times, rather than just stretching it. This might break the contract between layout and the compositor in ways that didn't happen before.

I'll investigate.
Assignee: nobody → botond
Flags: needinfo?(botond)
In edit mode, the Homescreen app has a fixed-position <div> called "curtain" whose size is the whole viewport, and which has the grey background color. This is what gives the grey background color to the whole screen in edit mode.

Since the element that's scrolling on the page is the entire viewport (i.e. the <html> element), the layer for this <div> is a child of the scrollable layer. This means that any overscroll transform that we apply to the scrollable layer, is applied to this layer as well. In particular, in the compression phase of the spring oscillation, we compress this "curtain", exposing a thin strip of the underlying background.

This is a pretty fundamental consequence of applying a compression transform to a scrollable layer. Note that we previously considered not applying the stretch/compression to fixed-position layers, but this causes other problems, e.g. if content under the fixed-position layer is stretched, then a portion of that content that the app author doesn't intend to be visible, can become visible.

I ran this issue by Gordon, and while it's possible to fix this by adjusting the overscroll transform is such a way that it always stretches and never compresses, we fear the resulting effect may not look as good. Before we explore this avenue, I'd like some input from some Gaia devs on whether it's necessary for the Homescreen to be structued this way. If it would be possible to write things in a way such that the grey background is on the <body> or <html> element itself, that would provide a simple solution to this problem.
Flags: needinfo?(kgrandon)
The gaia specific problem is that we have the same component for edit mode in different apps, and not all apps necessarily use the viewport as the scrollable frame. This is definitely a solvable problem, but I'm hesitant to do so if this is something that web developers would expect to work, and I assume it is since they are using this today.

It seems like much less of an impact to developers if we do not apply the transform to fixed-position layers. Alternatively - is there some additional CSS hooks that we could add to opt-out of the transform effect for a certain element/layer?
Flags: needinfo?(kgrandon) → needinfo?(botond)
Related: if you use the medium reference workload and go into an SMS thread, scroll it to the bottom, and "bounce", you'll see the date (which is sticky-position to the top of the scrollable area) shift down on the compression. This looks kinda bad, and has the same root cause as this bug - that is, we apply compression to fixed/sticky  layers as well.

I'm not sure what the best fix here is; as Botond said in comment 8 not applying the compression to fixed/sticky layers has its own set of problems. However the background color change that Botond suggested in comment 8 would not fix this problem either, as it is more specific to the homescreen structure.
Another perhaps-crazy option: allow the "stretch" half of the overscroll effect on fixed/sticky layers, but not the "compress" half of the effect.
(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #9)
> The gaia specific problem is that we have the same component for edit mode
> in different apps, and not all apps necessarily use the viewport as the
> scrollable frame. This is definitely a solvable problem, but I'm hesitant to
> do so if this is something that web developers would expect to work, and I
> assume it is since they are using this today.

Do you expect scenarios like this to be common "in the wild"?

> It seems like much less of an impact to developers if we do not apply the
> transform to fixed-position layers. 

Chris Lord suggested the same thing. This is certainly one of the more straightforward solutions, and if we believe this is less prone to break things than the current behaviour, then I'm happy to give it a try.

> Alternatively - is there some additional
> CSS hooks that we could add to opt-out of the transform effect for a certain
> element/layer?

Given that - as far as I'm aware - the specs don't talk about overscrolling at all, I doubt it.
Flags: needinfo?(botond)
For the record, here are some of the other suggestions that came up during an IRC discussion about this:

  - Apply the stretch/compression to fixed layers that have scrolling content
    underneath them, but not to ones that don't (like the "curtain" layer).

      - Kats pointed out that this won't fix the scenario from comment 10,
        though we may be able to work around that by still adjusting the
        position of the fixed layer to account for the scrollable layer's
        stretch or compression.

  - The suggestion in comment 11 was considered. One issue with this is that
    if you have a background layer like the "curtain" which is patterned
    instead of a solid color, then you get an odd effect where during a
    stretch, scrollable content stretches the same as the background, while 
    during a compression, scrollable content moves relative to the background.

  - Special-casing fixed layers that cover the entire scrollable area. This 
    might work but is pretty hackish and also doesn't fix the STR from
    comment 10 (though again, we can potentially fix that by other means).
Thanks for the thorough investigation here!

(In reply to Botond Ballo [:botond] from comment #12)
> Do you expect scenarios like this to be common "in the wild"?

FWIW - yes, I think this is a fairly common pattern for positioning elements in a page, and something that would impact other developers. I could be wrong, but my preference would be to take the conservative route and assume that this is something developers would run into.

It looks like there are some good ideas in comment 13 to fix this. I think I personally prefer comment 11, I think that the mis-matched distortions would be more acceptable than the current state, but I'm happy to see this fixed with whatever you think is the best approach.
Whiteboard: [2.2-Daily-Testing] [systemsfe] → [2.2-Daily-Testing] [systemsfe] polish
Gord - Wanted to alert you to this issue. - Rob
Flags: needinfo?(gbrander)
"  - Apply the stretch/compression to fixed layers that have scrolling content
    underneath them, but not to ones that don't (like the "curtain" layer)."

I think we should do this, land it and see how it is. I expect that it'd be the least-bad solution in terms of unwanted side-effects, assuming we want to keep an overscroll effect (my personal preference is that it's nice and we should keep it)

Botond, where are we on this at the moment?
Flags: needinfo?(botond)
At the moment we don't have spare cycles to work on this. Feel free to pick it up. If it's a serious issue that you would like to see fixed right away we can disable overscroll entirely.
Flags: needinfo?(botond)
(In reply to Chris Lord [:cwiiis] from comment #16)
> "  - Apply the stretch/compression to fixed layers that have scrolling
> content
>     underneath them, but not to ones that don't (like the "curtain" layer)."
> 
> I think we should do this, land it and see how it is. I expect that it'd be
> the least-bad solution in terms of unwanted side-effects, assuming we want
> to keep an overscroll effect (my personal preference is that it's nice and
> we should keep it)
> 
> Botond, where are we on this at the moment?

Technically, I believe the latest consensus between Gordon and me was to try a variation of the spring physics that doesn't involve compression, only stretching; it seemed like all other workarounds would mess up some cases.

Planning-wise, the change in 2.2's release schedule (as I understand, it will now be based on Gecko 37) has messed things up a bit. In order to be able to meet some of the APZ goals for 2.2, we've had to de-prioritize others, and overscroll polish items like this one were among them. Unfortunately, this means that if the 2.2 overscroll effect is not good enough to ship as is, we may need to revert to the 2.1 effect, or something like that.
blocking-b2g: 2.2? → 2.2+
Thanks Rob!
Due to the change of plans with 2.2, our time horizon on this has changed somewhat (for the better). My understanding is that this item was already pushed to the end of the GFX sprint, with the idea being that if they have spare cycles at end, they can resolve it. My recommendation was to resolve if possible, or roll back to 2.1 behavior if not.
Flags: needinfo?(gbrander)
Any updates here?
What's our deadline on this? If there's some flexibility to fix this after January 12th, I wouldn't mind taking this on.
If it's still unfixed when you get back, feel free to take it.
Taking, let's see if I can knock this on the head this week.
Assignee: botond → chrislord.net
Status: NEW → ASSIGNED
Should we have a "plan B"? Or is "plan B" to roll it back to 2.1 behavior?

needsinfo: Chris for understanding of what the realistic outcome is for this. needsinfo: Francis for design opinion (we can discuss over Vidyo).
Flags: needinfo?(fdjabri)
Flags: needinfo?(chrislord.net)
I think at this point "plan A" is to fix it. "plan B" is to not fix it. I don't really want to roll back to 2.1 behavior if we can avoid it.
Agree with kats in comment #25. Not fixing this isn't that bad, but I'll do my best - commencing now.
Flags: needinfo?(chrislord.net)
Keywords: polish
Whiteboard: [2.2-Daily-Testing] [systemsfe] polish → [2.2-Daily-Testing] [systemsfe]
This patch effectively removes underscroll. Instead of allowing underscroll, it reverses the velocity direction, acting as if the spring hit a perfectly elastic, immovable object.

Not asking for review, because this looks and feels markedly worse than the current solution - I think we'll need tweaked values for this to look good.

I haven't tried to understand the equations well enough to know what those values are yet though - ideally, we want the spring to oscillate only 2 or 3 times, but for those oscillations to have a wide arc. So I guess we want the deceleration to start very slowly, but increase quickly, exponentially (to allow for 2 or 3 big oscillations, then to very quickly drop off).

Also, will we/I need to alter any tests if we remove underscroll?
Attachment #8549007 - Flags: feedback?(botond)
Comment on attachment 8549007 [details] [diff] [review]
Disallow underscroll

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

The patch looks solid as an implementation of the physics model you described ("reverses the velocity direction, acting as if the spring hit a perfectly elastic, immovable object").

However, when I was discussing this bug with Gordon, he had reservations about using this model, because the "bounce" part would look like in a sharp, discontinuous change, in contrast with the otherwise smooth motion of the spring. 

BenWa instead proposed (and Gordon was optimistic about) the following model:

   - Keep the physics model where the spring oscillates
     in either direction relative to a "resting state".

   - However, rather than having the resting state of
     the spring be the edge of the layer's composition
     bounds, as it is now (so that a displacement on
     one side stretches the layer, while one on the
     other side compresses it), have the resting state
     position start away from the edge of the layer,
     and move it towards the edge of the layer, timing
     it so that it reaches the edge by the end of the
     animation. This way, displacements on either side
     of the resting state would result in stretching
     the layer (there would be no compression).

Does that make sense? Do you think it would be feasible to give this a try? I think the tricky bit would be figuring out the math; I'm happy to try and talk through that if it helps.

(Looking over the previous comments in this bug, I just realized that I hadn't summarized this discussion and the proposed model up until now. My apologies, I thought I had!)

> Also, will we/I need to alter any tests if we remove underscroll?

I don't expect so.
Attachment #8549007 - Flags: feedback?(botond)
(In reply to Botond Ballo [:botond] from comment #28)
> Comment on attachment 8549007 [details] [diff] [review]
> Disallow underscroll
> 
> Review of attachment 8549007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks solid as an implementation of the physics model you
> described ("reverses the velocity direction, acting as if the spring hit a
> perfectly elastic, immovable object").
> 
> However, when I was discussing this bug with Gordon, he had reservations
> about using this model, because the "bounce" part would look like in a
> sharp, discontinuous change, in contrast with the otherwise smooth motion of
> the spring. 
> 
> BenWa instead proposed (and Gordon was optimistic about) the following model:
> 
>    - Keep the physics model where the spring oscillates
>      in either direction relative to a "resting state".
> 
>    - However, rather than having the resting state of
>      the spring be the edge of the layer's composition
>      bounds, as it is now (so that a displacement on
>      one side stretches the layer, while one on the
>      other side compresses it), have the resting state
>      position start away from the edge of the layer,
>      and move it towards the edge of the layer, timing
>      it so that it reaches the edge by the end of the
>      animation. This way, displacements on either side
>      of the resting state would result in stretching
>      the layer (there would be no compression).
> 
> Does that make sense? Do you think it would be feasible to give this a try?
> I think the tricky bit would be figuring out the math; I'm happy to try and
> talk through that if it helps.

Yup, sounds perfectly feasible and probably a nicer looking effect - will implement this tomorrow :) (most of my time writing that patch was just re-familiarising myself with the code, so not really any time lost).
This does as suggested. I spent too many hours trying to work out if the current behaviour could be expressed as a geometric progression (I don't think it can), so I settled for this coarse approximation. Ends up it looks fine anyway.

This works by for each peak of an oscillation, it treats the negation of that peak as the largest value it could oscillate to on the next oscillation and transforms the overscroll in GetOverscroll based on that. This effectively adjusts the oscillation centre of the spring as it animates, but only at each peak (instead of smoothly, as I would have liked to have done).

I'm sure you'll let me know if I'm missing anything obvious :)
Attachment #8549980 - Flags: review?(botond)
Blocks: 1042103
Comment on attachment 8549980 [details] [diff] [review]
Disallow underscroll by transforming overscroll

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

Thanks! This generally looks great.

My one concern is that there is a scenario where an overscroll is started with zero velocity [1]. It happens in the (relatively rare) scenario described in bug 1040226 comment 3.

In this case, the first velocity sign change, as detected by 'oldVelocity * mVelocity < 0', won't happen until you get to the peak "on the other side", by which time we'll be in negative overscroll. I think we can handle this by changing the condition to 'oldVelocity * mVelocity <= 0' instead (and perhaps fuzzing the calculation of a new velocity to make sure _it's_ never zero but +/- EPISLON).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=edc1cbc1d7e4#2353

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +716,5 @@
>    }
>  private:
>    AsyncPanZoomController& mApzc;
> +  bool mXAnimating;
> +  bool mYAnimating;

I think we can avoid storing these variables by having an early-return in SampleOverscrollAnimation() if 'mVelocity == 0 && mOverscroll == 0'.

::: gfx/layers/apz/src/Axis.cpp
@@ +192,4 @@
>  }
>  
>  ParentLayerCoord Axis::GetOverscroll() const {
> +  return (mOverscroll - mOverscrollOffset) / mOverscrollScale;

It's a good sanity check to MOZ_ASSERT({{result}} >= 0) here.

@@ +232,5 @@
> +    mFirstOverscrollAnimationSample = mOverscroll;
> +  }
> +  bool velocitySignChange = (oldVelocity * mVelocity) < 0;
> +  if (velocitySignChange) {
> +    bool oddOscillation = ((float)mOverscroll * (float)mFirstOverscrollAnimationSample) < 0;

nit: I think it's clearer to use '.value' to unwrap a Coord.

::: gfx/layers/apz/src/Axis.h
@@ +227,5 @@
> +  // These three variables are used to modify overscroll during overscroll
> +  // animation to keep it from underflowing.
> +  ParentLayerCoord mFirstOverscrollAnimationSample;
> +  ParentLayerCoord mOverscrollOffset;
> +  float mOverscrollScale;

I think it would enhance people's ability to understand this patch if each variable's purpose was explained in a comment. In particular, it's worth mentioning that mOverscrollScale only takes on two values (1 and 2), and what the purpose of each is.
(In reply to Botond Ballo [:botond] from comment #32)
> Comment on attachment 8549980 [details] [diff] [review]
> Disallow underscroll by transforming overscroll
> 
> Review of attachment 8549980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! This generally looks great.
> 
> My one concern is that there is a scenario where an overscroll is started
> with zero velocity [1]. It happens in the (relatively rare) scenario
> described in bug 1040226 comment 3.
> 
> In this case, the first velocity sign change, as detected by 'oldVelocity *
> mVelocity < 0', won't happen until you get to the peak "on the other side",
> by which time we'll be in negative overscroll. I think we can handle this by
> changing the condition to 'oldVelocity * mVelocity <= 0' instead (and
> perhaps fuzzing the calculation of a new velocity to make sure _it's_ never
> zero but +/- EPISLON).
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> AsyncPanZoomController.cpp?rev=edc1cbc1d7e4#2353

Whoops, think I handled this in an earlier revision of the patch and accidentally removed... Will fix.

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +716,5 @@
> >    }
> >  private:
> >    AsyncPanZoomController& mApzc;
> > +  bool mXAnimating;
> > +  bool mYAnimating;
> 
> I think we can avoid storing these variables by having an early-return in
> SampleOverscrollAnimation() if 'mVelocity == 0 && mOverscroll == 0'.

Yes, sounds reasonable - this was unrelated really, just made debugging easier.

> ::: gfx/layers/apz/src/Axis.cpp
> @@ +192,4 @@
> >  }
> >  
> >  ParentLayerCoord Axis::GetOverscroll() const {
> > +  return (mOverscroll - mOverscrollOffset) / mOverscrollScale;
> 
> It's a good sanity check to MOZ_ASSERT({{result}} >= 0) here.

The result should be the same sign as mOverscrollOffset I think, but it may well be negative (depending on what direction we're overscrolled).

> @@ +232,5 @@
> > +    mFirstOverscrollAnimationSample = mOverscroll;
> > +  }
> > +  bool velocitySignChange = (oldVelocity * mVelocity) < 0;
> > +  if (velocitySignChange) {
> > +    bool oddOscillation = ((float)mOverscroll * (float)mFirstOverscrollAnimationSample) < 0;
> 
> nit: I think it's clearer to use '.value' to unwrap a Coord.

Sure.

> ::: gfx/layers/apz/src/Axis.h
> @@ +227,5 @@
> > +  // These three variables are used to modify overscroll during overscroll
> > +  // animation to keep it from underflowing.
> > +  ParentLayerCoord mFirstOverscrollAnimationSample;
> > +  ParentLayerCoord mOverscrollOffset;
> > +  float mOverscrollScale;
> 
> I think it would enhance people's ability to understand this patch if each
> variable's purpose was explained in a comment. In particular, it's worth
> mentioning that mOverscrollScale only takes on two values (1 and 2), and
> what the purpose of each is.

Yes, fair enough - just me being lazy really :)


If I get some time this weekend I'll submit another patch, otherwise Monday morning.
(In reply to Chris Lord [:cwiiis] from comment #33)
> > ::: gfx/layers/apz/src/Axis.cpp
> > @@ +192,4 @@
> > >  }
> > >  
> > >  ParentLayerCoord Axis::GetOverscroll() const {
> > > +  return (mOverscroll - mOverscrollOffset) / mOverscrollScale;
> > 
> > It's a good sanity check to MOZ_ASSERT({{result}} >= 0) here.
> 
> The result should be the same sign as mOverscrollOffset I think, but it may
> well be negative (depending on what direction we're overscrolled).

Whoops, you're right!

What I'd like to assert is that GetOverscroll() never goes in the opposite direction of where the overscroll started. Since we record the first sample in mFirstOverscrollAnimationSample, I think

  MOZ_ASSERT(({{result}} * mFirstOverscrollAnimationSample) >= 0); 

should do it?
Hopefully this addresses all comments.
Attachment #8549980 - Attachment is obsolete: true
Attachment #8549980 - Flags: review?(botond)
Attachment #8551281 - Flags: review?(botond)
Comment on attachment 8551281 [details] [diff] [review]
Disallow underscroll by transforming overscroll v2

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

Looks great, thanks!

::: gfx/layers/apz/src/Axis.h
@@ +223,3 @@
>    bool mAxisLocked;     // Whether movement on this axis is locked.
>    AsyncPanZoomController* mAsyncPanZoomController;
>    ParentLayerCoord mOverscroll;  // See GetOverscroll().

Now that GetOverscroll() no longer simply returns mOverscroll, it would be superb to update the comment here too, mentioning that mOverscroll is a displacement from the resting state of a spring as it oscillates (with the resting state moving as the animation progresses).

@@ +230,5 @@
> +  // GetOverscroll() never changes sign during animation. This is necessary,
> +  // as mOverscroll itself oscillates around zero during animation.
> +  // If we're not sampling overscroll animation, mOverscrollScale will be 1.0
> +  // and mOverscrollOffset will be zero.
> +  // If we are animating, mOverscrollScale will be 2.0 and mOverscrollOffset

This is super-nitpicky, but technically there's a phase of the animation (prior to the first peak) when mOverscrollScale is still 1.0.
Attachment #8551281 - Flags: review?(botond) → review+
Comments amended and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/981a1a2dd464
Flags: needinfo?(fdjabri)
https://hg.mozilla.org/mozilla-central/rev/981a1a2dd464
https://hg.mozilla.org/mozilla-central/rev/9e46d10862f7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(In reply to Botond Ballo [:botond] from comment #34)
> What I'd like to assert is that GetOverscroll() never goes in the opposite
> direction of where the overscroll started. Since we record the first sample
> in mFirstOverscrollAnimationSample, I think
> 
>   MOZ_ASSERT(({{result}} * mFirstOverscrollAnimationSample) >= 0); 
> 
> should do it?

The patch that landed contained the assertion

   MOZ_ASSERT((result * mOverscrollOffset) >= 0); 

which is incorrect and was firing in my debug build. This patch corrects it to be:

   MOZ_ASSERT((result * mFirstOverscrollAnimationSample) >= 0);
Attachment #8552544 - Flags: review?(chrislord.net)
Comment on attachment 8552544 [details] [diff] [review]
Follow-up to fix an incorrect assertion

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

Sorry about that, that's what I intended but I didn't test it :/
Attachment #8552544 - Flags: review?(chrislord.net) → review+
Please request b2g37 approval on this when you get a chance.
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
status-firefox38: --- → fixed
Flags: needinfo?(chrislord.net)
I was going to leave it a full week, but this is close enough I guess :) My own dog-fooding has shown up no problems.

This is the original patch plus the two follow-up commits rolled into one patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: In some cases after over-scrolling or flinging towards an edge, you may see page content that wasn't intended to be seen.
Testing completed: Locally on dog-food device and on master for almost a week.
Risk to taking this patch (and alternatives if risky): Low risk, I think.
String or UUID changes made by this patch: None
Flags: needinfo?(chrislord.net)
Attachment #8553657 - Flags: review+
Attachment #8553657 - Flags: approval-mozilla-b2g37?
Comment on attachment 8553657 [details] [diff] [review]
Patch + follow-up rolled up

Thanks for the roll-up patch here!

Flagging qa for verificaiton.
Attachment #8553657 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Keywords: verifyme
Attached video verify_video.MP4 — 
This issue has been verified pass on Flame 2.2/3.0
STR:
1. Long tap an app icon at homescreen to enter edit mode.
2. Scroll the screen down or up all the way.
**The over scroll bounce remains on edit mode.
See attachment:verify_video.MP4
Rate:0/5

Flame 2.2 build:
Gaia-Rev        cd42b034fd2825c3675ace3a67f5775eb61c2d60
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d824c65a6a2b
Build-ID        20150128002506
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150128.035910
FW-Date         Wed Jan 28 03:59:20 EST 2015
Bootloader      L1TC000118D0

Flame 3.0 build:
Gaia-Rev        ba613ae583a706131c45e885f65d428d4a541a81
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/9b6b80222e66
Build-ID        20150128101733
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150128.134719
FW-Date         Wed Jan 28 13:47:30 EST 2015
Bootloader      L1TC000118D0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+], [MGSEI-Triage+]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
You need to log in before you can comment on or make changes to this bug.