Content jitters when URL bar hidden

RESOLVED FIXED

Status

()

Core
Graphics: Layers
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: julienw, Assigned: kentuckyfriedtakahe)

Tracking

Trunk
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [status: needs uplift][madrid])

Attachments

(9 attachments, 9 obsolete attachments)

3.38 MB, video/mp4
Details
8.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.20 KB, patch
Details | Diff | Splinter Review
7.18 KB, patch
Details | Diff | Splinter Review
32.60 KB, image/gif
Details
1.18 MB, application/zip
Details
584.76 KB, application/pdf
Details
3.72 MB, video/quicktime
Details
7.84 KB, patch
drs
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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.
Flags: needinfo?(ben)

Comment 2

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

Comment 3

6 years ago
*kentucky
(Reporter)

Comment 4

6 years ago
This happens on other websites as well (eg the bing result page for example)
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.
(Reporter)

Comment 6

6 years ago
I'm on b2g18 :)
blocking-b2g: shira? → ---
tracking-b2g18: ? → +
Keywords: perf
(Reporter)

Comment 7

6 years ago
That's not "perf" at all. The scroll is smooth, but it jitters when the address bar disappears/displays.
Keywords: perf
I'm not seeing what you're seeing. Can you post a video?
(Reporter)

Updated

6 years ago
Flags: needinfo?(felash)
(Reporter)

Comment 9

6 years ago
Created attachment 709003 [details]
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(ben)
(Reporter)

Comment 11

6 years ago
(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 ?
Pan looks terrible but fling looks smooth. That may indicate a problem with AZPC.
(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.
Depends on: 839328
Assignee: nobody → ajones
Status: NEW → ASSIGNED
No longer depends on: 839328
Created attachment 711667 [details] [diff] [review]
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.
(Reporter)

Updated

6 years ago
blocking-b2g: --- → leo?
Duplicate of this bug: 837476
Attachment #711667 - Flags: review?(jones.chris.g)
Blocks: 836891
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.
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 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?
Attachment #711667 - Flags: review?(jones.chris.g)
blocking-b2g: leo? → leo+
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.
Created attachment 719114 [details] [diff] [review]
Pass screen relative co-ordinates to APZC
Attachment #719114 - Attachment description: Bug 833795 - Pass screen relative co-ordinates to APZC; r=cjones → Pass screen relative co-ordinates to APZC
Attachment #719114 - Flags: review?(jones.chris.g)
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 :(.
Attachment #719114 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
Attachment #711667 - Attachment is obsolete: true
Whiteboard: [caf-v1.0.1]
https://hg.mozilla.org/mozilla-central/rev/e0be1d5500f2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e5b09bb5efb9
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
blocking-b2g: leo+ → tef?

Updated

6 years ago
Depends on: 846816
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.

Updated

6 years ago
Whiteboard: [caf-v1.0.1] → [caf-v1.0.1][do not land to any branch without bug 833795]

Updated

6 years ago
Whiteboard: [caf-v1.0.1][do not land to any branch without bug 833795] → [caf-v1.0.1][do not land to any branch without bug 846816]
backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a9a5606617
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
status-b2g18: fixed → affected
status-firefox22: fixed → affected
Flags: in-testsuite?
blocking-b2g: tef? → tef+
status-b2g18-v1.0.0: wontfix → affected
Whiteboard: [caf-v1.0.1][do not land to any branch without bug 846816] → [do not land to any branch without bug 846816]
How are things going here?
Created attachment 721847 [details] [diff] [review]
Pass screen relative co-ordinates to APZC v2
Attachment #719114 - Attachment is obsolete: true
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.
Attachment #721847 - Attachment description: Pass screen relative co-ordinates to APZC; → Pass screen relative co-ordinates to APZC v2
Attachment #721847 - Flags: review?(cjones.bugs)
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.
Attachment #721847 - Flags: review?(cjones.bugs) → review?(bugzilla)
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.
Attachment #721847 - Flags: review?(bugzilla)
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.
(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.
Created attachment 724746 [details] [diff] [review]
Use screen relative co-ordinates for gestures;
Attachment #721847 - Attachment is obsolete: true
Attachment #724746 - Flags: review?(bugzilla)

Comment 36

5 years ago
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/ajones@mozilla.com-4b66d682be0c

Updated

5 years ago
status-b2g18-v1.0.0: affected → wontfix
status-b2g18-v1.0.1: wontfix → affected
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.
Attachment #724746 - Flags: review?(bugzilla) → review+
Duplicate of this bug: 852500
Duplicate of this bug: 853232
https://hg.mozilla.org/mozilla-central/rev/8a6cacf047a1
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox22: affected → fixed
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?
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Whiteboard: [do not land to any branch without bug 846816]
(Reporter)

Comment 44

5 years ago
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.
(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.
(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.
Maybe *this* time we'll get some tests for this...
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.
Created attachment 733731 [details] [diff] [review]
Add mScreenPoint to SingleTouchData;
Attachment #724746 - Attachment is obsolete: true
(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.)
I'm actually backing out the original patch as soon as it finishes compiling.
Comment on attachment 733731 [details] [diff] [review]
Add mScreenPoint to SingleTouchData;

A different approach to fixing this problem.
Attachment #733731 - Flags: review?(bugzilla)
Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc159983a018
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 54

5 years ago
It needs to be backed out from b2g-v1.0.1 and b2g18 branches too then.
Keywords: checkin-needed
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
status-b2g18: fixed → affected
status-b2g18-v1.0.1: fixed → affected
status-firefox23: --- → affected
Keywords: checkin-needed
Whiteboard: [leave open]
Target Milestone: mozilla22 → ---

Comment 56

5 years ago
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
Created attachment 735355 [details] [diff] [review]
Add mScreenPoint to SingleTouchData;
Attachment #733731 - Attachment is obsolete: true
Attachment #733731 - Flags: review?(bugzilla)

Comment 60

5 years ago
Should the patch be also backed out from Aurora 22?
(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.
Created attachment 736083 [details] [diff] [review]
Rename mScreenPoint to mClientContainerPoint;
Attachment #735355 - Attachment is obsolete: true
Created attachment 736084 [details] [diff] [review]
Rename mScreenPoint to mClientContainerPoint;
Attachment #736083 - Attachment is obsolete: true
Created attachment 736120 [details] [diff] [review]
Fix hiding the URL bar one and for all;
Attachment #736084 - Flags: review?(bugs)
Attachment #736120 - Flags: review?(bugs)
Attachment #736120 - Flags: review?(bugzilla)
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.
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 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.
Attachment #736084 - Flags: review?(bugs) → review+
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.
Attachment #736120 - Flags: review?(bugs) → review-
Anthony's in hospital for a few days.
Thanks Roc. Is there someone else who can take this bug over?
Assignee: ajones → roc
Whiteboard: [leave open] → [leave open][status: maybe needs new owner]

Updated

5 years ago
Whiteboard: [leave open][status: maybe needs new owner] → [leave open][status: maybe needs new owner][madrid]
Created attachment 738876 [details] [diff] [review]
Marionette test for regression
Over to kats per discussion with ajones and blassey.
Assignee: roc → bugmail.mozilla
Whiteboard: [leave open][status: maybe needs new owner][madrid] → [leave open][status: needs new patch][madrid]

Updated

5 years ago
Target Milestone: --- → Madrid (19apr)
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.
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.
Whiteboard: [leave open][status: needs new patch][madrid] → [leave open][status: possible gaia workaround, kats ramping up][madrid]
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
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
(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.
Created attachment 741058 [details] [diff] [review]
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.
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

5 years ago
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?
(Reporter)

Comment 81

5 years ago
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

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

Comment 83

5 years ago
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.
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 :)
Summary: the scroll jitters on liberation.fr → Content jitters when URL bar hidden

Updated

5 years ago
Target Milestone: 1.0.1 Madrid (19apr) → ---
I just confirmed this on Sotaro's buri device.
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.
Flags: needinfo?(bfrancis)
Whiteboard: [leave open][status: possible gaia workaround, kats ramping up][madrid] → [leave open][status: may need gaia workaround][madrid]
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(bfrancis)
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?
Flags: needinfo?(rmacdonald)
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.
(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.)
Flags: needinfo?(rmacdonald)
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?
(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.
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.
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
Assignee: bugmail.mozilla → ajones
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.
Duplicate of this bug: 868038
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?
Flags: needinfo?(fdjabri)
(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?
(Reporter)

Comment 99

5 years ago
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 ?
(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?
Flags: needinfo?(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.
Flags: needinfo?(akeybl)
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.
Flags: needinfo?(fdjabri)
(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".
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).
Created attachment 746603 [details]
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.
(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?
> (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

5 years ago
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.
(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

5 years ago
(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

5 years ago
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.
Created attachment 747222 [details]
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.
Whiteboard: [leave open][status: may need gaia workaround][madrid] → [status: may have working patch \o/, ajones will pick up on May 13th][leave open][madrid]
Created attachment 747236 [details]
Overscroll options PDF

Adding a PDF version of attachment 747222 [details]

Comment 114

5 years ago
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

5 years ago
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.
(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.
We'll take bug 853457 to mitigate the impact of this bug, but we're past the point of blocking here any longer.
blocking-b2g: tef+ → -
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?
(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.
blocking-b2g: - → tef?

Comment 120

5 years ago
Created attachment 748213 [details]
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.
(Reporter)

Comment 122

5 years ago
(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.
(Reporter)

Comment 123

5 years ago
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.
Created attachment 749673 [details] [diff] [review]
Sample offset on touch start;
Attachment #749673 - Flags: review?(bugzilla)
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.
Attachment #749673 - Flags: review?(bugzilla) → review+
tef+ since we were waiting to see if we could find a fix by mid-week, please land asap to all open branches.
blocking-b2g: tef? → tef+
Blocks: 775452

Updated

5 years ago
Whiteboard: [status: may have working patch \o/, ajones will pick up on May 13th][leave open][madrid] → [status: needs landing][leave open][madrid]
Created attachment 750282 [details] [diff] [review]
Sample offset on touch start;
Attachment #749673 - Attachment is obsolete: true
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.

Updated

5 years ago
Whiteboard: [status: needs landing][leave open][madrid] → [status: needs more local testing, and landing ETA 5/16][leave open][madrid]
Attachment #750282 - Flags: review?(bugzilla)
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.
Attachment #750282 - Flags: review?(bugzilla) → review+

Updated

5 years ago
Keywords: checkin-needed
Please do not check in yet. This needs a small change to a comment before that.
Keywords: checkin-needed
Already pushed. Need me to backout or can we just push a follow-up?
https://hg.mozilla.org/projects/birch/rev/459bf64f2569
It's ok, just let it go. It was just ordering of words in a comment. :)
https://hg.mozilla.org/mozilla-central/rev/459bf64f2569
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [status: needs more local testing, and landing ETA 5/16][leave open][madrid] → [status: needs uplift][madrid]
status-firefox22: fixed → wontfix
status-firefox23: affected → wontfix
status-firefox24: --- → fixed
This is a huge improvement btw, thank you Anthony.
Attachment #736120 - Attachment is obsolete: true
Attachment #736120 - Flags: review?(bugzilla)
You need to log in before you can comment on or make changes to this bug.