833795 - Content jitters when URL bar hidden

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

STR:
* open Browser
* go to http://mobile.liberation.fr
* wait until it's loaded
* scroll down the page

Expected:
* the scroll is smooth

Actual:
* the scroll jitters when the address bar disappear

It's as if when the address bar disappear the page goes up (because of layout stuff), and then goes back again so that it's at the same position as before, and so we see that it jumps up and down.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I don't actually know if this is more a Gaia::Browser bug or a Layers bug. Maybe the way we make the address bar display and/or disappear is not correct, so I'm asking Ben for feedback on this too.

Comment 2

[Nick Cameron [:nrc]]

kentucyfriedtakahe was looking at something similar, he might be able to dup this bug.

Comment 3

[Nick Cameron [:nrc]]

*kentucky

Comment 4

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

This happens on other websites as well (eg the bing result page for example)

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I noticed it jittering badly on m-c but not on b2g18. The problem seemed to exist with or without the address bar. I didn't have or file a bug so I don't think we have a dup.

Comment 6

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I'm on b2g18 :)

Comment 7

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

That's not "perf" at all. The scroll is smooth, but it jitters when the address bar disappears/displays.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm not seeing what you're seeing. Can you post a video?

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Attachment: video of the jitter

here I believe you'll see easily what I mean

this is quite small but it's noticeable and it's one of the unpleasant things that gives an impression of unfinishedness.

Comment 10

[Ben Francis [:benfrancis]]

This happens if you keep your finger on the screen while scrolling instead of "flicking" upwards.

The reason for this is that when the URL bar disappears the content moves upwards to take its place and the continued touch events from your finger interact with this movement to produce the effect you see.

I'm not sure how we'd fix this within the constraints of the current interaction design.

Comment 11

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Ben Francis [:benfrancis] from comment #10)
> The reason for this is that when the URL bar disappears the content moves
> upwards to take its place and the continued touch events from your finger
> interact with this movement to produce the effect you see.
> 
> I'm not sure how we'd fix this within the constraints of the current
> interaction design.

don't we know if a touch is occuring (ie: touchstart without touchend, maybe you already have internally this information) and if yes, don't move the content upwards ?

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Pan looks terrible but fling looks smooth. That may indicate a problem with AZPC.

Comment 13

[Ben Francis [:benfrancis]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> don't we know if a touch is occuring (ie: touchstart without touchend, maybe
> you already have internally this information) and if yes, don't move the
> content upwards ?

Those touch events don't bubble up to the browser app AFAIK, they're handled internally by the async pan & zoom code in Gecko.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Workaround for layer relative co-ordinate system.

The co-ordinates in the touch events are relative to the layer position rather than the physical screen. This means that when the layer moves the touch event shows a corresponding move in the opposite direction. The patch proves that this is the case by working around the issue.

Comment 16

[David Flanagan [:djf]]

I don't know anything about gfx/layers/ipc/Axis.cpp, but that patch seems specific to the Browser apps (hardcoding 49 pixels) and not a general fix.

If this patch does not pass review, there is a Gaia side patch in bug 837476. It was awaiting review when Anthony closed that bug.

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm assuming this isn't an issue outside of the browser because there is no URL bar. It seems like changing Browser.js is allowing the problem to leak even further away from root cause.

The patch basically ignores a large >=30px jump in the *reverse* direction. The proper way to fix it is to feed screen relative co-ordinates into the APZC.

MultiTouchInput.mTouches[0].mScreenPoint is not screen relative (in AZPC) and changing it may break other things.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 711667 [details] [diff] [review]
Workaround for layer relative co-ordinate system.

The coordinates we deliver are supposed to be screen-relative, IIRC.  Can we fix that?

Comment 19

[David Flanagan [:djf]]

I discovered in my work on bug 837476 that the coordinates in the event depend on the meta viewport tag of the site being viewed as well and on whether the user has pinch-zoomed on the site. The way I conceptualize it is that the event is reporting CSS pixels for the website, but we want device pixels because that is what the address bar height is measured in.

A possibly related bug (or if not related something that should be reported separately) is that if the user pinches out to zoom way in on the site, scrolling seems to slow way down.  I'm guessing that is some kind of float to integer rounding error.

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Pass screen relative co-ordinates to APZC

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 719114 [details] [diff] [review]
Pass screen relative co-ordinates to APZC

Ugh, sorry for missing this earlier today.  Not sure how that happened :(.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0be1d5500f2

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0be1d5500f2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e5b09bb5efb9

Comment 25

[Jason Smith [:jsmith]]

Note to triagers - do *not* take this patch up to v1.01. This totally just broke facebook. We should back this out or get a bustage fix immediately.

Comment 26

[[:fabrice] Fabrice Desré]

backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a9a5606617

Comment 27

[Ryan VanderMeulen [:RyanVM]]

And b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c11d91e1b575

(I assume we'll get a test for the issue in bug 846816 before this re-lands as well)

Comment 28

[Andrew Overholt [:overholt]]

How are things going here?

Comment 29

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Pass screen relative co-ordinates to APZC v2

Comment 30

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 721847 [details] [diff] [review]
Pass screen relative co-ordinates to APZC v2

I'm not entirely happy with moving the out-event co-ordinates but not the in-event co-ordinates. I'd welcome any suggestions on how this could be done more cleanly.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'll try to get to this review request as soon as I can, but if you can find another reviewer in the meantime that'd be fine by me.

Comment 32

[Doug Sherk (:drs) (inactive)]

Comment on attachment 721847 [details] [diff] [review]
Pass screen relative co-ordinates to APZC v2

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

The actual coordinate conversion looks fine to me.

::: dom/ipc/TabParent.cpp
@@ +666,5 @@
>    }
>  
> +  // The return event needs to be mapped into the frame relative co-ordinate
> +  // space before it gets passed to the AZPC. The AZPC applies the scale after
> +  // that.

I don't think this code necessarily interacts with APZC, so stating this assuming that it does is probably misleading.

@@ +673,5 @@
> +  MaybeForwardEventToRenderFrame(event, &e);
> +
> +  return (e.message == NS_TOUCH_MOVE) ?
> +      PBrowserParent::SendRealTouchMoveEvent(e) :
> +      PBrowserParent::SendRealTouchEvent(e);

Why are we not calling TabParent::SendRealTouchEvent() anymore? This isn't a big deal, but if all we're doing is mapping coordinate spaces, I don't see anything in TabParent::SendRealTouchEvent() that would interfere with the new functionality. Maybe I'm missing something, but it seems like a refactor makes more sense if we should skip everything in TabParent::SendRealTouchEvent() up until the MaybeForwardEventToRenderFrame() call.

Comment 33

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

So the problem that t(In reply to Doug Sherk (:drs) (:dRdR) from comment #32)
> I don't think this code necessarily interacts with APZC, so stating this
> assuming that it does is probably misleading.

This code could interact with something else. Right now it doesn't. I agree that this is overly specific.

> Why are we not calling TabParent::SendRealTouchEvent() anymore? This isn't a
> big deal, but if all we're doing is mapping coordinate spaces, I don't see
> anything in TabParent::SendRealTouchEvent() that would interfere with the
> new functionality. Maybe I'm missing something, but it seems like a refactor
> makes more sense if we should skip everything in
> TabParent::SendRealTouchEvent() up until the
> MaybeForwardEventToRenderFrame() call.

The point is not to skip everything at the beginning of SendRealTouchEvent rather that the code is not reachable through TryCapture.

>> bool TabParent::SendRealTouchEvent(nsTouchEvent& event)
>> {
>>   if (mIsDestroyed) {
>>     return false;
>>   }

Might need this guard clause.

>>   if (event.message == NS_TOUCH_START) {
>>     MOZ_ASSERT((!sEventCapturer && mEventCaptureDepth == 0) ||
>>                (sEventCapturer == this && mEventCaptureDepth > 0));
>>     // We want to capture all remaining touch events in this series
>>     // for fast-path dispatch.
>>     sEventCapturer = this;
>>     ++mEventCaptureDepth;
>>   }

If we're calling from TryCapture we will never hit this code because if this guard:
if (event.message == NS_TOUCH_START || isTouchPointUp) 

>> 
>>   nsTouchEvent e(event);
This is copying the event so we need this bit.

>>   // PresShell::HandleEventInternal adds touches on touch end/cancel.
>>   // This confuses remote content into thinking that the added touches
>>   // are part of the touchend/cancel, when actually they're not.
>>   if (event.message == NS_TOUCH_END || event.message == NS_TOUCH_CANCEL) {
>>     for (int i = e.touches.Length() - 1; i >= 0; i--) {
>>       if (!e.touches[i]->mChanged) {
>>         e.touches.RemoveElementAt(i);
>>       }
>>     }
>>   }

We never hit this code because isTouchPointUp in the above guard is:
bool isTouchPointUp = (event.message == NS_TOUCH_END ||
                       event.message == NS_TOUCH_CANCEL);

So in order to inline TabBrowser::SendRealTouchEvent we just need to collect together the mIsDestroyed guard (missing from the patch, my mistake), the copying of the event then the following code:

>>   MaybeForwardEventToRenderFrame(event, &e);
>>   return (e.message == NS_TOUCH_MOVE) ?
>>     PBrowserParent::SendRealTouchMoveEvent(e) :
>>     PBrowserParent::SendRealTouchEvent(e);
>> }

So we take the inlined code above but the change we want to make is to transform 'e' but not 'event'. So the thing that we need to do differently is to copy 'event' to 'e' and then transform 'e' so that 'event' is screen relative and 'e' is frame relative.

'event' needs to be screen relative to prevent the gestures getting muddled.
'e' needs to be frame relative so that the scroll and zoom can be applied before it is sent to PBrowserParent.

Comment 34

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #33)
> So we take the inlined code above but the change we want to make is to
> transform 'e' but not 'event'. So the thing that we need to do differently
> is to copy 'event' to 'e' and then transform 'e' so that 'event' is screen
> relative and 'e' is frame relative.
> 
> 'event' needs to be screen relative to prevent the gestures getting muddled.
> 'e' needs to be frame relative so that the scroll and zoom can be applied
> before it is sent to PBrowserParent.

In that case, why not name them as such? Or at least "e"->"frameRelativeEvent" or something more meaningful like that. I think it could also benefit from a comment summarizing what you wrote here. "event" and "e" aren't as bad names inside TabParent::SendRealTouchEvent() because there's no coordinate transforms going on there, so they're closer to being the same thing.

Otherwise based on what you said I'm happy with it. Maybe we should file a followup bug to deal with the difference of coordinate spaces.

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Use screen relative co-ordinates for gestures;

Comment 36

[Mozilla RelEng Bot]

Try run for 4b66d682be0c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4b66d682be0c
Results (out of 354 total builds):
    success: 339
    warnings: 14
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4b66d682be0c

Comment 37

[Doug Sherk (:drs) (inactive)]

Comment on attachment 724746 [details] [diff] [review]
Use screen relative co-ordinates for gestures;

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

Nice, two nits.

::: dom/ipc/TabParent.cpp
@@ +677,5 @@
> +
> +    // Remove the frame offset and compensate for zoom.
> +    nsEventStateManager::MapEventCoordinatesForChildProcess(frameLoader,
> +                                                            &event);
> +    rfp->ApplyZoomCompensation(&event);

I'd prefer to call this "ApplyZoomCompensationToEvent."

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +100,4 @@
>     */
> +  nsEventStatus ReceiveMainThreadInputEvent(const nsInputEvent& aEvent);
> +
> +  /** Transform from frame relative co-ordinates to DOM relative co-ordinates.

Spacing issue here.

Comment 39

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6cacf047a1

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a6cacf047a1

Comment 42

[Ben Francis [:benfrancis]]

\o/

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3048c1c1ee92
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a4d7f8764567

So no tests then? What could possibly go wrong?

Comment 44

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Would it be possible to have a test for bug 846816 ? This seems like an important thing after all, Ryan is right in comment 27 and bug 846816 comment 7.

Thanks.

Comment 45

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> So no tests then? What could possibly go wrong?

Good point. I'll get onto that right away.

Comment 46

[Diego Wilson [:diego]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/3048c1c1ee92
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a4d7f8764567
> 
> So no tests then? What could possibly go wrong?

Apparently plenty :( This patch causes bug 856083, links in the browser can't be selected.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Maybe *this* time we'll get some tests for this...

Comment 48

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The fundamental problem is that the browser frame can move so to you need screen relative co-ordinates to process gestures. The code wasn't written with that in mind so it becomes a tracking down the flow of events through the code. Fixing it properly is has always been higher risk than the extremely nasty but lower risk first patch. I have been working on a regression test for 846816 but that wouldn't have caught bug 856083.

Comment 49

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add mScreenPoint to SingleTouchData;

Comment 50

[Michael Vines [:m1] [:evilmachines]]

(FWIW, I /think/ the proper protocol here is to create a new bug to fix the regression rather than piling new patches into this bug, which has already landed.)

Comment 51

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm actually backing out the original patch as soon as it finishes compiling.

Comment 52

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 733731 [details] [diff] [review]
Add mScreenPoint to SingleTouchData;

A different approach to fixing this problem.

Comment 53

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc159983a018

Comment 54

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

It needs to be backed out from b2g-v1.0.1 and b2g18 branches too then.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

The backout was backed out and re-landed in bug 856083.
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d0583cc8cb

Backed out on the branches.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/38a237247841
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e66f76c0e4f2

Comment 56

[Verdi [:verdi] Best to slack me]

I just updated my phone and I'm seeing this exact issue.
Unagi phone
OS version: 1.1.0.0-prerelease
Hardware revision: nice
Platform version: 18.0
Build Identifier: 20130403070204
Update channel: beta

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84d0583cc8cb

Comment 58

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=01567809c9a4

Comment 59

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add mScreenPoint to SingleTouchData;

Comment 60

[Scoobidiver (away)]

Should the patch be also backed out from Aurora 22?

Comment 61

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Scoobidiver from comment #60)
> Should the patch be also backed out from Aurora 22?

It won't affect anything other than b2g but one could argue that it is safer to back it out.

Comment 62

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Rename mScreenPoint to mClientContainerPoint;

Comment 63

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Rename mScreenPoint to mClientContainerPoint;

Comment 64

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix hiding the URL bar one and for all;

Comment 65

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Previously we were mapping:
  nsDOMTouchEvent::mRefPoint -> SingleTouchData::mScreenPoint

The field mScreenPoint contained a value which referred to a position relative to the client area. This was a little confusing given that nsDOMTouchEvent::mScreenPoint was always set to (0,0).

With these patches we map:
  nsDOMTouchEvent::mScreenPoint -> SingleTouchData::mScreenPoint
  nsDOMTouchEvent::mRefPoint -> SingleTouchData::mClientContainerPoint

Although the meanings are still not compeltely consistent with other code, it doesn't change the value contained in nsDOMTouchEvent::mRefPoint and therefore isn't far less likely to break code elsewhere.

Comment 66

[Doug Sherk (:drs) (inactive)]

I'm really not happy with having touch events have two coordinate spaces of the same point stored on them. The approach in all other code has been to convert to whatever coordinate space is needed, on the fly. Doing it the way proposed in this patch is really bad for code maintainability and clarity. What's to prevent data from going stale? What happens if you have a nested frame? It also wasn't immediately clear to me, as someone who has worked a lot in this code, what the added/changed code was doing.

Another issue here is that MultiTouchData and the other classes in InputData.h are intended to be basically mirrored copies of nsTouchEvent and its associated classes, but for off-main-thread usage. These changes make them diverge quite a bit.

I'd like to see more investigation into propagating this data from the compositor instead. I also want to see what smaug says about this.

Not going to r- until we all chat a bit.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 736084 [details] [diff] [review]
Rename mScreenPoint to mClientContainerPoint;

We need some comment in http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/InputData.cpp?rev=f134e2c4bb2a&mark=59-59,102-102#58
to indicate why ->refPoint there isn't actually refPoint but something else
which TabParent/ESM have modified.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 736120 [details] [diff] [review]
Fix hiding the URL bar one and for all;

So I don't understand why MultiTouchInput::MultiTouchInput for touch events use
screenpoint and refpoint, but MultiTouchInput::MultiTouchInput with mouse event
would work just with refPoint.
Based on http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp?rev=366e6a39d71a#1536
only (m)refPoints are translated.

Comment 69

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

Anthony's in hospital for a few days.

Comment 70

[Dietrich Ayala (:dietrich)]

Thanks Roc. Is there someone else who can take this bug over?

Comment 71

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Marionette test for regression

Comment 72

[Johnny Stenback (:jst)]

Over to kats per discussion with ajones and blassey.

Comment 73

[Ben Francis [:benfrancis]]

Next week I'm going to experiment with implementing bug 860812 to change the way this feature works. There's a possibility that could allow us to close this bug, but I'm not sure if it will work. If anyone is able to work on this gecko patch then please continue to do so, but I will try to work around it in Gaia if I can.

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

So I don't see any major problem, except that we need to make sure right coordinates are passed
to MultiTouchInput::MultiTouchInput.

I might agree with comment 66 that it is a bit ugly to have two coordinates, but at least
the patch makes code more readable than it is now since the member variables actually tell
what the coordinates are about.

Comment 75

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

So I'm trying to wrap my head around what's going on, and the first question I have is, why does the gonk widget code populate the DOMTouch instances with screen-relative coordinates?

This happens at [1]. The constructor that takes the argument assigns it to mRefPoint [2], which AFAICT is supposed to be relative to the widget. This is documented (for nsEvent::refPoint) at [3], and Fennec obeys this by doing the conversion from screen-relative to widget-relative at [4].

Or am I completely misunderstanding this code?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#153
[2] http://mxr.mozilla.org/mozilla-central/source/content/events/src/Touch.h#61
[3] http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#665
[4] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp#732

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Note, this is all super-hackish. TabParent ends up calling ESM 
which makes refPoint to be translated to new coordinate space
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp?rev=e57ac5581703#1535

Comment 77

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Ben Francis [:benfrancis] from comment #73)
> ... but I will try to work around it in Gaia if I can.

I still think the first (hacky) patch is preferable to letting the problem pollute gaia.

Comment 78

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

Attachment: WIP - Ensure APZC always gets events in screen coordinates

I looked at the code for a while (sorry it took so long, I got bogged down with build issues). I think that a proper fix will probably require more than code "adjustment" - I think the attached WIP does one of the adjustments but there is another one needed somewhere else (still looking to see where exactly).

In the code as it is now, the event that is propagated from the widget -> APZC gets transformed by MapEventCoordinatesForChildProcess before it gets to APZC. TabParent also creates a copy of the event after it has been mapped to the child process and passes that as the "aOutEvent" to APZC::ReceiveInputEvent, so that APZC can further adjust it based on the async pan/zoom transform. That copy then goes along to PBrowserParent. The problem AIUI is that the APZC gets event coordinates relative to the child process, which have a discontinuity when the toolbar hides.

With my patch, the MapEventCoordinatesForChildProcess is moved in the flow so that it only applies to the copy of the event that is the "aOutEvent". This way the events that the APZC receives are always relative to the screen and so there is no discontinuity from the toolbar hiding. It seems to me that this is a more correct behaviour in terms of what APZC is expecting, although this patch by itself just makes things worse :(

I'll keep investigating and see if I can find other spots that adjustments need to be made to compensate for this. Please feel free to tell me how wrong I am.

Comment 79

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Having all the co-ordinates to APZC in user space may cause click targets to be broken. Check for regressing bug 856083 and bug 846816.

Comment 80

[Chris Lee [:clee]]

I just tried to repro this on the latest Buri device with a 4/19 build at http://mobile.libration.fr and the jittering isn't very obvious.  

I have not reviewed all 70+ comments, but are there other sites that highlight this issue?

Comment 81

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I see this a lot when I visit my own webapps-install-testing website: http://everlong.org/mozilla/

try to scroll and click on "hosted app without app cache". This is painful and I do this several times a day ;-)

Comment 82

[Chris Lee [:clee]]

I just tried to repro your steps and I don't see a major issue on the Buri device.  What hardware are you using?

Comment 83

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I'm using unagi.

But I think there is also a big difference whether you keep the finger on the screen while you scroll or if you lift the finter and let the page scroll with inertia, at the moment where the jittering happens (ie: when the address bar disappears). So please try both ways as I don't remember which one is the most obvious and I don't have my phone here.

Comment 84

[Ben Francis [:benfrancis]]

You have to keep your finger on the screen while scrolling to reproduce the bug. It took me a while to figure that out because I always "fling" when scrolling and for a long time I couldn't tell what all the fuss was about :)

Comment 85

[Andrew Overholt [:overholt]]

I just confirmed this on Sotaro's buri device.

Comment 86

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

The more I look at this the more I'm thinking this is a Gaia-level bug. The "jitter" is actually a result of (a) the content iframe element moving up by 50 pixels and getting painted there, and (b) the APZC recompositing content back down 50 pixels when it receives the next touch event which is relative to the iframe rather the screen coordinates.

My patch from comment 78 fixes (b), but leaves behind the 50-pixel jump that is caused by (a), because this is a Gaia-side change. I believe that fixing (a) in Gaia should completely fix the jitter without needing my patch from comment 78.

The quickest way to fix it, IMO, would be to modify the code at [1] so that it scrolls the content down by 50 pixels [2], adjusted for the resolution the content is being painted at (because we want to adjust by 50 device pixels, not 50 CSS pixels).

However I couldn't find a way to adjust the scroll offset of the content from that point in the code. The currentTab.dom element there has a null contentWindow (I assume because it is a remote-content iframe) and I'm not familiar with the correct way to hook up this call. All I want to do is the equivalent of currentTab.dom.contentWindow.scrollBy(0, -(50/resolution)) where resolution can be calculated by doing currentTab.dom.scrollWidth / evt.detail.width. Can somebody who knows the Gaia code either do this or tell me how to?

Also note that the first half of the "jitter" does occur even during flings - that is, if you start at the top of the page with the address bar showing and fling down by > 50 pixels, the content will move up by 50 pixels when the address bar disappears. It's just harder to notice because the content is moving and there's no reference frame without your finger on the screen. The second half of the jitter doesn't occur because in a fling you're not delivering more touch events to the APZC so it doesn't recomposite and do (b).

[1] https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/browser/js/browser.js;h=c28ac9b6fa8c63d29a4d6217fae4088ec3ac462c;hb=HEAD#l566
[2] The "50 pixels" is really 5rem in the CSS. To avoid re-hard-coding this we should check the difference in the clientHeight before and the classList.add('expanded') call.

Comment 87

[Ben Francis [:benfrancis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #86)
> However I couldn't find a way to adjust the scroll offset of the content
> from that point in the code. The currentTab.dom element there has a null
> contentWindow (I assume because it is a remote-content iframe) and I'm not
> familiar with the correct way to hook up this call. All I want to do is the
> equivalent of currentTab.dom.contentWindow.scrollBy(0, -(50/resolution))
> where resolution can be calculated by doing currentTab.dom.scrollWidth /
> evt.detail.width. Can somebody who knows the Gaia code either do this or
> tell me how to?

You can't access the contentWindow of a cross-origin iframe, that's the reason much of the Browser API exists. To implement what you describe in Gaia I think we'd need to add a new scrollBy method to the Browser API. I don't know enough about how this works in Gecko to be sure, but I'd be concerned that the workaround you describe could actually make the situation worse rather than better because the scrollBy method would just cause another mozbrowserscroll event to be fired after the "jitter" has already happened.

I'm still trying to come up with a workaround in Gaia as part of bug 860812 but I'm not having much success, and if you look at the comments on that the implementation in Fennec has similar problematic edge cases.

Comment 88

[Ben Francis [:benfrancis]]

Over in bug 860812 is a description of how this feature is being implemented in Fennec, where they have had similar challenges and valuable lessons have been learnt. I think there's an opportunity to take the same approach in Gaia in the future if we want to but after some experimentation and discussions with Chris Lord I've come to the conclusion this would require even bigger changes to Gecko/the browser API to do this in a web app. This probably isn't feasible in the 1.0.1/1.1.0 timeframe.

I've talked to Kartikaya who is going to try to work on a prototype of a fix as described above which involves adding a scrollBy method to the Browser API (a new feature), but I think it would be wise to consider backup solutions at this stage. This bug has now been around for 7 months, refuses to go away, and has now been called out as a certification blocker. There are also other issues with the current implementation such as having to scroll all the way to the top of the page in order to switch tabs.

One alternative might be to change the UI design, make the top bar fixed and focus our space-saving attention on the bottom bar instead. Short of re-designing the whole UI to have no bottom bar, Chris has suggested that we could transplant in the hideable bottom bar used by the homescreen wrapper as that seems to work quite well.

Do UX have any thoughts on an alternative to scrolling the address bar off the screen which we could quickly put in place?

Comment 89

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

I put together a prototype of the scrollTo approach, and now I'm hitting a bug in APZC that makes it not work (mostly). Turns out that APZC will happily clobber the scroll position set by scrollTo calls if it's in the middle of doing something. This happens because it sends a bunch of paint requests while panning/flinging is happening, and assumes that all the paint notifications it receives are a result of these. If a scrollTo-generated paint occurs in the middle of this sequence, it throws off the code in NotifyLayersUpdated, since the unexpected paint gets treated as an "expected paint", and then the last paint request APZC triggers from the panning operations gets treated as the "scrollTo paint" and clobbers the scroll position. I'll see if I can come up with a way to handle this.

Comment 90

[Rob MacDonald [:robmac]]

(In reply to Ben Francis [:benfrancis] from comment #88) 
> Do UX have any thoughts on an alternative to scrolling the address bar off
> the screen which we could quickly put in place?

I talked to Francis and Casey about this. Assuming the jittering cannot be fixed for TEF, then another option would be to have the address bar scroll off the screen at the same speed and time as the content within the view.

One additional complication is that we would like to see the address bar retained at the top of the view while the page is loading. When the loading stops, the bar can disappear instead of transitioning away. (This allows the user to stop page loads or refresh the view even after they've scrolled down.)

Comment 91

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm not convinced that it makes sense to fix this is Gaia. Could we try my latest patch for TEF and work on a proper solution on m-c?

Comment 92

[Ben Francis [:benfrancis]]

(In reply to Rob MacDonald [:robmac] from comment #90)
> I talked to Francis and Casey about this. Assuming the jittering cannot be
> fixed for TEF, then another option would be to have the address bar scroll
> off the screen at the same speed and time as the content within the view.

Thanks for the input, but unfortunately I don't think this helps. This was the original design and is what Fennec is trying to do, but is even harder to get right than the current solution. The issue is that the content being interacted with and the toolbar are two completely separate web pages in two separate domains in two separate frames in two separate system processes, so making them appear to move as one is non-trivial!

> One additional complication is that we would like to see the address bar
> retained at the top of the view while the page is loading.

This was already implemented in bug 853457 on master, but has not been uplifted to any release branches.

Comment 93

[Rob MacDonald [:robmac]]

I discussed the idea of having a fixed address bar with Francis Djabri, who is leading ux efforts on browser going forward. However, my understanding from Ben is that the feedback on the fixed address bar previously was quite negative. So, although this has the potential to address this bug, we felt the cure is worse than the original problem.

I'll add Francis to the discussion for comment.

Comment 94

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

To follow up with what I was trying in comment 89: I haven't been able to get it working reliably because of various behavioural issues in AsyncPanZoomController.

For those who don't care about the details ignore this paragraph. I added the scrollTo plumbing that I was talking about [1] but the scrollTo position would keep getting clobbered by APZC. When I dug into this I found that the code to track paints in APZC (specifically the mWaitingForContentToPaint branch in AsyncPanZoomController::NotifyLayersUpdated) is pretty broken because it assumes that content will never call ScrollTo while the user is panning. I tried to improve this code with the patch in [2] by recording the area of the requested paint and matching it up with the paint that actually happened. This helped a little bit but is still too brittle to be usable, since if it fails you can get stuck in a situation where paints aren't requested at all. While doing this I also found one cause of paint mismatches, which I filed bug 867582 for (that seems like a nice fix that should go in regardless).

At this point it looks like continuing down this road will lead to a very risky fix, if one can be reached at all - I suspect there will be other lurking bugs that are exposed instead. I don't know if Anthony is back now and/or has time to look at this again but I'm going on PTO tomorrow so I won't be able to look at this more in the near future. Handing this back to Anthony for now; please reassign as needed.

Also, re Anthony's latest patch from comment 64 - I tried it out and it fixes half of the jitter, same as the patch I posted in comment 78. The page still jumps up by 50 pixels when the toolbar disappears. This might also be an acceptable workaround since it is a lot less noticeable than the full jitter.

[1] https://github.com/staktrace/mozilla-central/commit/6da06743398472c34bb252b78b4d966bfad4f340
[2] https://github.com/staktrace/mozilla-central/commit/5ff53811044b7fc9296c788d6e22308cd23033a8

Comment 95

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm intending to return to work one-handed on the 13th. I've got a good idea of how I can make a more acceptable patch based on feedback.

I've considered that the content moving up when the URL bar disappears to be out of scope for this bug on the basis that it is moving in the same direction as your finger and I personally think it makes sense the way it is. If someone can file a separate bug then we can consider it separately for TEF. It requires a very different approach to fix and as per your discussion above may be trickier to fix.

Comment 97

[Ben Francis [:benfrancis]]

Note that in bug 868038 Chris Lord again suggests that we have a fixed top bar and a collapsible bottom bar, using the same bottom bar as the homescreen wrapper.

Francis, what do you think of this suggestion if a platform fix is not sufficient?

Comment 98

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Ben Francis [:benfrancis] from comment #97)
> Francis, what do you think of this suggestion if a platform fix is not
> sufficient?

Is the platform fix sufficient or not?

Comment 99

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Note that the already fixed Bug 853457 could be an acceptable work around for tef+, maybe we should tef+ bug 853457 and un-tef+ this bug (keeping it leo+ or tracking+).

Ben, your thoughts ?

Comment 100

[Daniel Coloma:dcoloma]

(In reply to Julien Wajsberg [:julienw] from comment #99)
> Note that the already fixed Bug 853457 could be an acceptable work around
> for tef+, maybe we should tef+ bug 853457 and un-tef+ this bug (keeping it
> leo+ or tracking+).
> 
> Ben, your thoughts ?

I think Julie's suggestion is the right one for 1.0.1 timeframes. Alex, Lukas, should we follow that path?

Comment 101

[Alex Keybl [:akeybl]]

(In reply to Daniel Coloma:dcoloma from comment #100)
> I think Julie's suggestion is the right one for 1.0.1 timeframes. Alex,
> Lukas, should we follow that path?

I'm all for mitigating the impact at this point - I'm asking UX to review 853457 urgently for one more set of eyes.

Comment 102

[Francis Djabri [:djabber]]

The fix for bug #853457 solves the issue of allowing the user to halt the download if she has scrolled down the page, but I don't think it mitigates the UX impact of this bug. The content still jitters badly when the content has finished loading if the user is scrolled down the page, so the user will experience the jitter one way or another.

I'm not against deferring the fix to this bug for a later release but I certainly would want to see this bug fixed. 

Incidentally, I don't consider keeping the URL bar fixed at the top and collapsing the bottom bar, as suggested in comment 98, to be a fix for this bug. This seems to be a case of the fix being worse than the original bug.

Comment 103

[Ben Francis [:benfrancis]]

(In reply to Julien Wajsberg [:julienw] from comment #99)
> Note that the already fixed Bug 853457 could be an acceptable work around
> for tef+, maybe we should tef+ bug 853457 and un-tef+ this bug (keeping it
> leo+ or tracking+).
> 
> Ben, your thoughts ?

As Francis says, that bug represents a separate issue (keeping the address bar displayed as the page is loading), it has little impact on this bug. The address bar will still "jitter" when it is scrolled off the screen.

So if UX doesn't like the idea of always keeping the top bar displayed (in favour of collapsing the bottom bar), and they haven't been able to come up with an alternative solution, then the best solution we have so far is Anthony's platform patch which kats points out only "fixes half of the jitter".

Comment 104

[Chris Lord [:cwiiis]]

Perhaps it's a matter of opinion, but I really think noticeable jitter on *every* page browsed to is something we shouldn't ship with. This is something that will look bad on even the most cursory look at the phone, and people will be doing video reviews where this bug will be obvious.

On a UX level, I fail to see the point of even having a toolbar that hides if a permanently visible lower toolbar was added because of it (at least on iOS, the lower toolbar houses basically all the functionality except 'navigate to url' - though I still think we do it better on Firefox for Android).

Comment 105

[Francis Djabri [:djabber]]

Attachment: Example of floating browser buttons

If this bug can't be fixed, one potential workaround that minimizes the amount of chrome and still remains usable would be to: 

- keep the top bar fixed to the top of the screen  
- replace the bottom toolbar with floating buttons 

Please see the attachment for an example of how this could look. 

The current toolbar in any case kills a lot of space and generally only shows back and favorite buttons. I'd suggest we pursue the floating buttons option, even if we can fix this bug. One other advantage with this is solution that it makes it more consistent with the Firefox on Android design. 

So this would be my suggestion for resolving this, but I need to finalize this proposal with the visual design team and also the Firefox on Android UX team. I'll try to get this closed off ASAP.

Comment 106

[Ben Francis [:benfrancis]]

(In reply to Ben Francis [:benfrancis] from comment #103)
> the best solution we have so far is
> Anthony's platform patch which kats points out only "fixes half of the
> jitter".

After re-reading all 103 comments on this bug I've come to the conclusion that Anthony's patch is by far the best solution.

We can either wait for him to return on the 13th, or change the UX design.

(In reply to Francis Djabri [:djabber] from comment #105)
> If this bug can't be fixed, one potential workaround that minimizes the
> amount of chrome and still remains usable would be to: 
> 
> - keep the top bar fixed to the top of the screen  
> - replace the bottom toolbar with floating buttons 

This design was proposed about a year ago, but the problem with it is that if the buttons are always shown then they can obscure content and in some cases make it impossible to click some buttons/hyperlinks at the bottom of a page. How would we mitigate this?

> One other advantage with this is
> solution that it makes it more consistent with the Firefox on Android
> design. 

What makes you say this?

Comment 107

[Francis Djabri [:djabber]]

> (In reply to Francis Djabri [:djabber] from comment #105)
> > If this bug can't be fixed, one potential workaround that minimizes the
> > amount of chrome and still remains usable would be to: 
> > 
> > - keep the top bar fixed to the top of the screen  
> > - replace the bottom toolbar with floating buttons 
> 
> This design was proposed about a year ago, but the problem with it is that
> if the buttons are always shown then they can obscure content and in some
> cases make it impossible to click some buttons/hyperlinks at the bottom of a
> page. How would we mitigate this?

Ben, as discussed, there are a few options we can explore here. e.g., 

- allowing the web page to overscroll above the floating buttons (this would be my suggestion if possible)
- dismissing the button when the bottom of the page is reached and re-invoking them if the user scrolls back up the page (potential discoverability issues)
- collapsing the buttons down to a small visual indicator and allowing them to be re-invoked if the user swipes up from the bottom of the page

I'll mock these options up tomorrow and send them out for review

Comment 108

[Larissa Co [:lco]]

Hello all, I'm back :) In the interest of getting this bug closed for v1.0.1, I'm going to be a little pushy so that we can make a call, so bear with me :)

A disclaimer which I know we all realize: We are not going to get the ideal browser UX with the timelines we have. We have to compromise somewhere for v1 and put in place a solid design plan which actually solves the UX problem in the next iteration. More on this later.

The UX needs captured in this bug are these:
1. the browser needs to feel performant to look polished. No jank, nice smooth scrolling
2. the user has to be able to look at content in the browser with minimal UI in the way

In my opinion, #1 trumps #2, if we are looking at a perfectly black-and-white world. If the browser doesn't look performant every time the user scrolls down a web page, it makes the entire phone experience look bad and speaks about engineering quality. The user's feature phone is likely not going to have this jank. If the UI is kind of annoying and in the way of seeing more content onscreen, well, honestly, most users have dealt with more annoying UI hacks in the past. This one isn't going to be the product-killer if it's there for one release.

So for me, #1 is non-negotiable for the first version. Which is why I think Anthony's patch (though an awesome effort, which I truly appreciate) isn't going to be enough. If we can have a reasonable temporary solution for #2 as well, great! I think Francis' floating bottom bar icons is quite usable, even if it might not be the final design we all want. I would suggest making the icons slightly transparent, and perhaps making them feel docked to the bottom of the screen so that the interface feels a little more cohesive; We want the user to know that these icons are part of the browser and not the page. I also agree with Francis that an overscroll past the page content can solve some of the overlapping content and hit-target issues.

I do want to emphasize that I think the floating icons are a temporary solution, especially if more features get added to that bottom bar. Perhaps in the future, we should reconsider eliminating the bottom bar altogether and go with a version of Fennec's top toolbar (with back button) design. There's a lot of history behind this decision which I won't go into on this bug, but it might be the best "proper" solution if we can't get the top bar scroll fixed, even in subsequent releases. 

In conclusion, let's review Francis' mocks tomorrow (it would be nice to just have one meeting and get it done with rather than discussing on bugzilla), and decide whether any of them are good interim solutions. If we can't find something we can be happy with, I suggest that we focus on the ideal next iteration and keep the unfortunate bottom bar in for now.

Comment 109

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Larissa Co [:lco] from comment #108)
> So for me, #1 is non-negotiable for the first version. Which is why I think
> Anthony's patch (though an awesome effort, which I truly appreciate) isn't
> going to be enough.

Have you looked at the result with my patch applied?

It meets #1 and #2. You could argue that it feels slippery when the content moves ahead of your finger. I certainly wouldn't describe the slipperyness as jitter.  

A quick fix for this is and has always been available. I'll be working on an acceptable long term fix for it again next week.

Comment 110

[Larissa Co [:lco]]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #109)
> (In reply to Larissa Co [:lco] from comment #108)
> > So for me, #1 is non-negotiable for the first version. Which is why I think
> > Anthony's patch (though an awesome effort, which I truly appreciate) isn't
> > going to be enough.
> 
> Have you looked at the result with my patch applied?

> 
> It meets #1 and #2. You could argue that it feels slippery when the content
> moves ahead of your finger. I certainly wouldn't describe the slipperyness
> as jitter.  

No, I will take a look at it today and tell you what I think. I've been on vacation :)

> 
> A quick fix for this is and has always been available. I'll be working on an
> acceptable long term fix for it again next week.

Comment 111

[Larissa Co [:lco]]

Here's a video of Anthony's patch (too big to add as an attachment, but I'll work on this tomorrow)
http://cl.ly/1o2430112P0R

First of all, I'm really sorry for how blurry the video is. I had a really bad tech day today (I tried to capture this video with six different devices, all which failed in various ways) and this is the best I could do. I'll create a new one tomorrow so that everyone can see it more clearly.

Fabrice and I looked at the patch today, and it's not bad at all. I think this is *much* better than the jitter, and acceptable UX. There is a tiny bit of "slide" if you look hard enough, but it feels fluid and not awkward in my opinion.

I did find a bug with the patch, however. You'll see later on in the video that if you tap to open the tab menu and then tap back to go back to the page, it introduces a really weird double scroll effect when you slide the page up. It doesn't happen when you click on a tab icon, only when you click the upper-left corner to go back to the current page. 

The other UX issue with this patch is that you have to scroll all the way up to reveal the top bar, which can get annoying on a long page if the user wants to switch tabs. In the intended design, the top bar should reappear (without scrolling the page all the way up) if the user scrolls up. Is this feasible with the current implementation?

But besides these issues, I think the top bar hide animation that Anthony's created is fine.

Comment 112

[Francis Djabri [:djabber]]

Attachment: Overscroll options

Thanks Larissa for posting the video. I agree that the scrolling looks much smoother now and gets my thumbs up as well. 

For what it's worth, I mocked up some ideas for what we could do if we need to stick with a fixed top bar and have attached them here. I think in any case it might be worth pursuing this lighter style for the toolbar buttons as opposed to the heavy grey bar that we currently have. 

Re: re-invoking the top bar quickly, the solution you mention sounds ideal, but failing that, one idea would be to show a 'return to top' floating button in the content area when the user scrolls down the page. This sounds like a separate bug however.

Comment 113

[Francis Djabri [:djabber]]

Attachment: Overscroll options PDF

Adding a PDF version of attachment 747222 [details]

Comment 114

[Daniel Nunes]

Hey, I'm new here, just a diehard FF Fan I guess :P To me, solving the problem seems clear, and I didn't see it mentioned here. It involves changing the way the address bar physically disappears from the screen, but its nature cannot have this problem.

I identify two main points that make this problem possible: 1. That the page itself moves when the bar goes away. 2. The bar is over the page.

Here's my two solutions to solve it based on those points then...

1. Make the URL bar scroll with the page so it's never covering the page. This means part of the URL bar could be covered and part visible from scrolling the page.

2. Make the URL bar disappear without affecting the position of the page. Basically it will only disappear once part of the page behind it wouldn't be visible anyways.

I have no idea whether these solutions would be feasible though.

Comment 115

[Daniel Nunes]

I'd like to point out that using both solutions I provided sounds pretty good when used together: one for hiding the URL bar and one for revealing it. Have it simply go away when you scroll down enough and have it tacked to the top of the page when you scroll to the top to reveal it. This behavior would persist until the entire URL bar is revealed.

Comment 116

[Ben Francis [:benfrancis]]

(In reply to Larissa Co [:lco] from comment #111)
> Here's a video of Anthony's patch

Which patch is this? I can't get either of the patches attached to this bug to apply. Maybe I'm doing something wrong.

Comment 117

[Alex Keybl [:akeybl]]

We'll take bug 853457 to mitigate the impact of this bug, but we're past the point of blocking here any longer.

Comment 118

[Ben Francis [:benfrancis]]

Alex, bug 853457 does not mitigate this bug in any way.

Many people have expressed the opinion that we shouldn't ship with this as it is, and I'm inclined to agree.

What has changed that we are now "past the point of blocking"?

Can it not wait for Anthony to return on Monday?

Comment 119

[Alex Keybl [:akeybl]]

(In reply to Ben Francis [:benfrancis] from comment #118)
> Alex, bug 853457 does not mitigate this bug in any way.

OK - triage misunderstood in that case.

> Many people have expressed the opinion that we shouldn't ship with this as
> it is, and I'm inclined to agree.
> 
> What has changed that we are now "past the point of blocking"?

Some of our partners will only take this change with a single test cycle.

> Can it not wait for Anthony to return on Monday?

I want to hear that this is going to land next week or not at all. Leaving on tef? for now.

Comment 120

[Larissa Co [:lco]]

Attachment: video of Anthony's patch

Same video, not blurry. Scrolling is decently smooth.

Bfrancis-- not sure which patch this was, but I'll ask Fabrice to respond to the bug with what he put on the phone he gave me.

Comment 121

[[:fabrice] Fabrice Desré]

I applied these 2 patches:

https://bugzilla.mozilla.org/attachment.cgi?id=736120

https://bugzilla.mozilla.org/attachment.cgi?id=736084

Comment 122

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Ben Francis [:benfrancis] from comment #116)
> (In reply to Larissa Co [:lco] from comment #111)
> > Here's a video of Anthony's patch
> 
> Which patch is this? I can't get either of the patches attached to this bug
> to apply. Maybe I'm doing something wrong.

They apply correctly on central, with minimal conflicts that are easy to resolve.

attachment 736120 [details] [diff] [review] needs a Touch.h file which doesn't exist on the b2g18 branch afaik.

Comment 123

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I tested Anthony's patch and I'm happy to say it works fine for me. I still find not-ideal behaviors when I scroll up and down around the position where the bar hides/displays, to make it hiding and displaying while it's animating. I see this especially on http://everlong.org/mozilla, sometimes hiding the address bar when it was just displayed, without a user action.

But IMHO this is good enough for 1.0 and even maybe for 1.1. Moreover this is probably more a gaia bug.

Comment 124

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Sample offset on touch start;

Comment 125

[Doug Sherk (:drs) (inactive)]

Comment on attachment 749673 [details] [diff] [review]
Sample offset on touch start;

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

This is an acceptable fix for now but I still really think we should revisit this. This is actually a clever way of doing it, but my problem with it is that it doesn't account for things other than the touch start event. For example, what would happen if it were a mouse event instead (though this is relatively easy to deal with)? Another problem I could foresee is dealing with async-panned nested frames. For this reason, if we land this, I think it should block bug 775452 for reference.

Please file followup bugs and add comments in the code.

::: dom/ipc/TabParent.h
@@ +304,5 @@
>      // anymore.
>      bool mIsDestroyed;
>      // Whether we have already sent a FileDescriptor for the app package.
>      bool mAppPackageFileDescriptorSent;
> +    // The offset for the child process which is sampled at touch start

I think in here would be the best spot.

Comment 126

[Alex Keybl [:akeybl]]

tef+ since we were waiting to see if we could find a fix by mid-week, please land asap to all open branches.

Comment 127

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Sample offset on touch start;

Comment 128

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=a91325480888

I found some problems with the last patch when testing for regressions. This one is hopefully good to go.

Comment 129

[Doug Sherk (:drs) (inactive)]

Comment on attachment 750282 [details] [diff] [review]
Sample offset on touch start;

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

::: dom/ipc/TabParent.h
@@ +296,5 @@
>      // transform in place, |aOutEvent| is the transformed |aEvent| to
>      // dispatch to content.
>      void MaybeForwardEventToRenderFrame(const nsInputEvent& aEvent,
>                                          nsInputEvent* aOutEvent);
> +    // The offset for the child process which is sampled at touch start. This

I'd prefer a comment of the form "TODO/bug 872911: The offset for the child process..." so that it's more obvious that this is talking about a followup bug at a glance.

Comment 130

[Doug Sherk (:drs) (inactive)]

Please do not check in yet. This needs a small change to a comment before that.

Comment 131

[Ryan VanderMeulen [:RyanVM]]

Already pushed. Need me to backout or can we just push a follow-up?
https://hg.mozilla.org/projects/birch/rev/459bf64f2569

Comment 132

[Doug Sherk (:drs) (inactive)]

It's ok, just let it go. It was just ordering of words in a comment. :)

Comment 133

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/459bf64f2569

Comment 134

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/46d590927f40
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/29af59290179

Comment 135

[Ben Francis [:benfrancis]]

This is a huge improvement btw, thank you Anthony.