Closed Bug 970093 (overlaid-keyboard) Opened 11 years ago Closed 8 years ago

Implement overlaid keyboard to not require reflow when showing and hiding the keyboard

Categories

(Core :: Panning and Zooming, defect)

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

Tracking

()

Status:
RESOLVED INCOMPLETE
Project Flags:
tracking-b2g backlog

People

(Reporter: vingtetun, Unassigned)

References

Details

(Keywords: perf, Whiteboard: gfx-noted)

Attachments

(2 files, 5 obsolete files)

Implement overlaid keyboard including manifest attribute to enable it. (Gaia patch)
(deleted), patch
Details | Diff | Splinter Review
Implement overlaid keyboard including manifest attribute to enable it.
(deleted), patch
kats
: review-
Details | Diff | Splinter Review 
When the keyboard is shown/hidden the current app window is resized.
This situation creates a lot of reflows inside the current app and it ends up not beeing a pleasant user experience. Futhermore there is some case where the app author may not want the app window to be resized (fullscreen game for example).

I suggest that we introduce a new property in the manifest that let people opt-out of automatic resizing when the keyboard is shown.

This bug will serve as a tracking for all the dependencies I have in mind that prevent to offer this simple attribute in the manifest.
No longer depends on: 970094
OS: Linux → All
Hardware: x86_64 → All
Could we try to not tie that to the manifest? I guess this can also be useful for a game that is just bookmarked and uses the fullscreen api for instance.
(In reply to Fabrice Desré [:fabrice] from comment #1)
> Could we try to not tie that to the manifest? I guess this can also be
> useful for a game that is just bookmarked and uses the fullscreen api for
> instance.

I have no strong opinion about where the configurable part should live. 
I just now that it needs to live somewhere and the manifest is my first idea. Ideally it would be nice to assume that not apps wants to be resized and use that by default but this seems a bit agressive and I'm pretty sure it will break some apps. Maybe we can assume that fullscreen apps does not want to be resized as well ?
I haven't thought a lot about that, but this looks a lot like a window management issue to me. Are we doing a single reflow when hiding/showing the keyboard? Why would that be so slow?
(In reply to Fabrice Desré [:fabrice] from comment #3)
> I haven't thought a lot about that, but this looks a lot like a window
> management issue to me. Are we doing a single reflow when hiding/showing the
> keyboard? Why would that be so slow?

As said on IRC this is mosly because of the in-app reflows.
In general I'm not sure this is a good idea. You cite the example of a fullscreen game that may not want to be resized by a keyboard, but there's also a counter-example of apps that may want some part of the app always visible to the user, and putting up a keyboard on top of the app without notifying it will break that.

Being able to interact with all parts of the app when there is a keyboard on top of it will be a pain regardless, and personally I would rather work on making in-app reflows faster or finding some other way to deal with them.

Note that even in Android apps the app gets resized when the keyboard comes up. I assume they put some thought into that decision, but even if they didn't, it seems likely that app authors have that expectation now and can deal with it appropriately.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> In general I'm not sure this is a good idea. You cite the example of a
> fullscreen game that may not want to be resized by a keyboard, but there's
> also a counter-example of apps that may want some part of the app always
> visible to the user, and putting up a keyboard on top of the app without
> notifying it will break that.

Having any kind of events for when the IME pops up will be useful. Relying on the resize event has always been a kind of hack.

> 
> Being able to interact with all parts of the app when there is a keyboard on
> top of it will be a pain regardless, and personally I would rather work on
> making in-app reflows faster or finding some other way to deal with them.
>

I have some very ugly patches locally. I will try to attach them somewhere so you can test them and see that interacting with the app is far from beeing painful. Actually, it's as easy than with the resize, minus the glitches.

Making in-app reflows faster is easier to say than to do. In a more platform-oriented point of view that is supposed to work for third party authors too I don't think that saying to app author - "if you want your app to not be laggy with a core interaction, then spend a lot of times optimizing for it" is a good message. 

 
> Note that even in Android apps the app gets resized when the keyboard comes
> up. I assume they put some thought into that decision, but even if they
> didn't, it seems likely that app authors have that expectation now and can
> deal with it appropriately.

Actually the default behavior on Android (at least in the previous versions, I don't know if this has changed), is to have a kind of floating keyboard. App authors can opt-in to a keyboard that resize the window, but that's not the default.
Attached patch floating.keyboard.POC.patch (obsolete) (deleted) — — Splinter Review 
Here the dirty Gecko patch I used for seeing how it looks.
Attached patch gaia.keyboard.overscroll.poc.patch (obsolete) (deleted) — — Splinter Review 
The gaia part.
Here are my thoughts on this. I haven't tried this patch because I think I understand it, but I sat around and thought about it for a bit. When I read it, I was happy that there were changes to APZC to support overscrolling, because I was going to suggest it if it wasn't there.

For starters, I think this can give us some really big perf gains. We can only make reflowing so fast, and we can only reduce the amount of reflows we do by so much. Even if we perfect this process in every app and every situation (which, let's face it, isn't going to happen), we're going to still have to reflow the entire app window once whenever the keyboard is shown or hidden. On low-end hardware, this is always going to be slow. I don't think we should even primarily argue about this from our perspective (despite it still being useful to us), but from the perspective of third-party app developers.

This patch and the general idea behind it doesn't force us into doing this everywhere. I agree that we shouldn't use this as a replacement for optimizing number of reflows and reflow speed. But in some cases, we will never get it as fast as it can be without something more drastic like this.

In terms of implementation, I think it might not be granular enough to stick this in the manifest, among other problems. There might be cases where an app wants its window to be resized in some situations, but not in others. For this reason, I think it might be better to stick an attribute on each <input> element where we don't want the keyboard to shrink the app window. Something like <input overlay-keyboard>. I could foresee problems with this; for example, the keyboard is not aware of the DOM of this process, so we would have add some IPC goop to handle this, and this would almost certainly be slower than putting it in the manifest. This would also require spec work, which putting it on the manifest doesn't (AFAIK).

Vivien and I talked on IRC about some of the implications of this. In particular, we weren't sure about how |position: fixed| elements would be positioned in the situation where the popping up of the keyboard did not resize the app window. After thinking about this a bit, I think what makes the most sense is to position them relative to the "simulated" viewport, i.e. the one that is equivalent to the "real" viewport minus the area occupied by the keyboard. I thought of a really simple use case for this: the Twitter app has an absolutely positioned "share your status" bar fixed to the bottom of the viewport. If we left it on the bottom of the app window, instead of above the keyboard, it would become invisible when you tap on it. Yes, this could be handled by the app when it receives the keyboard shown/hidden events, but it seems to me like unexpected behavior if I'm an app developer.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> Note that even in Android apps the app gets resized when the keyboard comes
> up. I assume they put some thought into that decision, but even if they
> didn't, it seems likely that app authors have that expectation now and can
> deal with it appropriately.

This is actually not true, even within the Android system and settings apps. For example, their wifi login details view doesn't resize when the keyboard comes up, so sometimes I can't type my password without knocking the keyboard down, then tapping on the password field.

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #6)
> Making in-app reflows faster is easier to say than to do. In a more
> platform-oriented point of view that is supposed to work for third party
> authors too I don't think that saying to app author - "if you want your app
> to not be laggy with a core interaction, then spend a lot of times
> optimizing for it" is a good message.

In general I agree with this. App developers would have to opt-in to this behavior. Finding, analyzing, and fixing reflow issues is difficult even for us.

(In reply to Fabrice Desré [:fabrice] from comment #3)
> I haven't thought a lot about that, but this looks a lot like a window
> management issue to me. Are we doing a single reflow when hiding/showing the
> keyboard? Why would that be so slow?

We're rarely actually doing only 1 reflow on resize. Some apps have got it down to that, but many haven't, and I don't think any apps have it down to only 1 reflow in every situation and in every view.
If we can use overscroll to make sure that the user can always reach the bottom content of the page with the keyboard popped up over I would be very happy to make not resizing the default behavior (have to check how thatll work on games tho).

It also allows us to make the keyboard more shiny by having it blend in with the content more through partial opacity (e.g. bottom top is now fixed color, can make it opaque).
(In reply to Jan Jongboom [:janjongboom] from comment #10)
> If we can use overscroll to make sure that the user can always reach the
> bottom content of the page with the keyboard popped up over I would be very
> happy to make not resizing the default behavior (have to check how thatll
> work on games tho).

This patch does that, based on my understanding of it.
(In reply to Doug Sherk (:drs) from comment #11)
> (In reply to Jan Jongboom [:janjongboom] from comment #10)
> > If we can use overscroll to make sure that the user can always reach the
> > bottom content of the page with the keyboard popped up over I would be very
> > happy to make not resizing the default behavior (have to check how thatll
> > work on games tho).
> 
> This patch does that, based on my understanding of it.

It does.
roc, we'd like to get your thoughts on this. Would also be nice for one of the current panning/zooming guys (botond/kats) to weigh in some more.
Flags: needinfo?(roc)
The discussion above has convinced me that this is not a bad idea. I still think it would be good to go through a bit more formal design to ensure that all use cases and edge cases are covered and we know what the expected behaviour is always.
This sounds reasonable to me.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> This sounds reasonable to me.

roc, do you have any thoughts on my proposed design or anything else?
Botond and I talked about this a bit in person. He was thinking about how we could implement the displayport-only resizing. In particular, he pointed out that we don't understand how fixed-position elements are handled in the layers code.

My understanding is that they are layerized and a transform is applied to them which undoes the parent's scrolling transform, thus keeping them stationary. If this is always true, this shouldn't be difficult to handle, and we can simulate reflowing on the compositor for these elements (though content will think they're at the original spot they were reflowed in). However, we're not sure if fixed-position elements are always layerized. For example, if a page can't scroll in any direction and has a fixed-position element, maybe that element is just painted onto the main content layer. If so, my proposal was that we could bail out of the overlaid keyboard path, and force reflowing.
ni? Chris since Botond says that he knows more about fixed-position elements/layers (though he's on PTO for 2 weeks).
Flags: needinfo?(chrislord.net)
We just talked again and Botond mentioned that he doesn't think we have to resize the displayport, or really tell the content thread that anything changed at all (besides the keyboard open/close events). My concern here was that we'd be using more memory storing a bigger buffer than is necessary, but resizing it would also cause checkerboarding underneath the keyboard when it's hiding. We agreed that resizing it isn't necessary for a first iteration, and we should wait until we hear back from someone who knows more about fixed-position layers.
(In reply to Doug Sherk (:drs) from comment #19)
> We just talked again and Botond mentioned that he doesn't think we have to
> resize the displayport, or really tell the content thread that anything
> changed at all (besides the keyboard open/close events). My concern here was
> that we'd be using more memory storing a bigger buffer than is necessary,
> but resizing it would also cause checkerboarding underneath the keyboard
> when it's hiding. We agreed that resizing it isn't necessary for a first
> iteration, and we should wait until we hear back from someone who knows more
> about fixed-position layers.

Can we handle the fixed position case as an orthogonal issue ? I feel like the foundation of the work can be done here, and the fixed positionning issue address as a separate bug.

For example if there is a mechanism to opt-in to this overlaid keyboard, we may start to use it into some apps, but the app authors that used fixed element positionning will not use it until the fixed positioning issue is solved.

Does it makes sense ?
What about this approach?
1) Land a simple version to see the benefits. Use an attribute in the manifest of certified apps to opt-in this behaviour. 
2) Fix the first bugs that appear with the simple version.
3) Refine the solution so we can treat more cases (fixed positioning, media queries, etc) and have more certified apps opting in. Maybe that step involves giving a slow path to some cases.
4) Remove the flag and have all web content benefit.

1 to 3 is only available for certified apps so that we don't pollute the manifest spec.
I'm not in agreement yet that we're at the point where we should start building this for the purpose of landing it, but I'll start anyways. I think a little bit of thought put into this now can save us a lot of time in the future.

(In reply to Anthony Ricaud (:rik) from comment #21)
> 4) Remove the flag and have all web content benefit.

This can never happen because this is fundamentally breaking layout by telling it one thing and doing another.
Comment on attachment 8376193 [details] [diff] [review]
floating.keyboard.POC.patch

Hey,

do you have an opinion about the bridge that is implemented in this patch in order to let Gaia control the x/y possible overscroll ?

Ideally it would be nice if Gaia has nothing to do, but since the keyboard app is part of the Gaia system app layout, the platform has not idea about it.
Attachment #8376193 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8376193 [details] [diff] [review]
floating.keyboard.POC.patch

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

Based on what you're doing in this patch, I think a better solution would involve just artificially shrinking the mCompositionBounds that is set in the RecordFrameMetrics function in nsDisplayList.cpp, for the root scrollable layer.

Basically, my idea is that what you're trying to do is remove reflows. From the APZ side of things, you want everything to behave exactly the same way as it does now - this means you want the exact same FrameMetrics being passed in as we get now. If the keyboard becomes an overlay (by changing things in Gaia) then the FrameMetrics will be different, and you want to do some artificial changes to it so that it goes back to what it used to be. I believe the only things that will change in the FrameMetrics are the composition bounds (and may be the root composition size), and so if you provide the same old values to APZ even with the floating keyboard it will behave as it did before, and you will bypass the reflow in layout. Does that sound reasonable?
Attachment #8376193 - Flags: feedback?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> Based on what you're doing in this patch, I think a better solution would
> involve just artificially shrinking the mCompositionBounds that is set in
> the RecordFrameMetrics function in nsDisplayList.cpp, for the root
> scrollable layer.
> 
> Basically, my idea is that what you're trying to do is remove reflows. From
> the APZ side of things, you want everything to behave exactly the same way
> as it does now - this means you want the exact same FrameMetrics being
> passed in as we get now. If the keyboard becomes an overlay (by changing
> things in Gaia) then the FrameMetrics will be different, and you want to do
> some artificial changes to it so that it goes back to what it used to be. I
> believe the only things that will change in the FrameMetrics are the
> composition bounds (and may be the root composition size), and so if you
> provide the same old values to APZ even with the floating keyboard it will
> behave as it did before, and you will bypass the reflow in layout. Does that
> sound reasonable?

This makes sense to me, but I wonder if there's a way to do this without needing to have Gaia set this via this bridge.
It might be something we could do as a CSS property or attribute (or new value for CSS overflow). The property would basically say "this element should be scrollable such that any part that's hidden by stuff on top can be brought into view".

This will definitely require input from layout folks, because I'm not sure how this might work, and how it will interact with things like scrollTop/scrollLeft properties, functions like scrollIntoView(), and so on. This is the part that I think needs more thinking about. Presumably you'll need to use these to keep input fields visible once the keyboard pops over it.
CC'ing gordon from UX to keep him in the loop here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> It might be something we could do as a CSS property or attribute (or new
> value for CSS overflow). The property would basically say "this element
> should be scrollable such that any part that's hidden by stuff on top can be
> brought into view".
>

This is tricky as sometimes we want to overlay something on top of the content (for example a temporary notification at the bottom), but we don't want to alter content scrolling behavior in this case (at least I think we don't want to).
Okay, here's another version of this. I need to get the coordinate conversions correct, but this overall makes sense to me.
Assignee: nobody → drs+bugzilla
Attachment #8376193 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8413473 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8413473 [details] [diff] [review]
Implement overlaid keyboard including manifest attribute to enable it.

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

General comments:

- Does the call to setOverlayClip happen before or after the keyboard is actually made visible? The code in APZCTM::UpdatePanZoomControllerTree will only take effect on the next repaint *after* setOverlayClip is called, so if they repaint from the keyboard coming up already happened you may not get another repaint for a while.

- It looks like from the gaia patch that you call setOverlayClip on a specific tabparent. I think then that you should be passing in the layers id from that TabParent/RenderFrameParent to the APZCTreeManager and only apply the overlay clip to APZC instances which have that layers id. Otherwise you'll end up applying it to the keyboard app as well which might have undesirable effects..

::: dom/ipc/TabParent.cpp
@@ +2007,5 @@
>  NS_IMETHODIMP
> +TabParent::SetOverlayClip(int32_t aWidth, int32_t aHeight)
> +{
> +  if (RenderFrameParent* rfp = GetRenderFrame()) {
> +    rfp->SetOverlayClip(aWidth, aHeight);

Turn this into a CSSSize right here rather than passing untyped ints around and only turning it into a CSSSize in APZCTreeManager.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +218,5 @@
> +        ParentLayerSize computedOverlayClip;
> +        ParentLayerIntSize computedOverlayClipInt;
> +        if (mOverlayClip.width > 0 || mOverlayClip.height > 0) {
> +          computedOverlayClip = ParentLayerSize(-metrics.mCompositionBounds.x,
> +                                                -metrics.mCompositionBounds.y);

This code doesn't make sense to me. It might make sense if you only applied it to the root APZ with the correct layers id. (See my general comment about this). But I forgot what we decided in our IRC discussion.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +279,5 @@
>     */
>    bool CanBePanned(AsyncPanZoomController* aApzc);
>  
> +  /**
> +   * Clips every frame in this tree by the width and height provided, from the

s/frame/layer/

@@ +379,5 @@
>    Vector< nsRefPtr<AsyncPanZoomController> > mOverscrollHandoffChain;
>    /* For logging the APZC tree for debugging (enabled by the apz.printtree
>     * pref). */
>    gfx::TreeLog mApzcTreeLog;
> +  /* The overlay clipping rect used for clipping every frame in this tree. */

s/frame/layer/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1783,5 @@
>      mFrameMetrics.mHasScrollgrab = aLayerMetrics.mHasScrollgrab;
>  
> +    // Compensate for any overlay clipping, such as the overlaid keyboard being
> +    // on top of this frame.
> +    mFrameMetrics.mCompositionBounds -= aOverlayClip;

You also need to do this in the other branch, where (aIsFirstPaint || isDefault) is true.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1049,5 @@
>    ScrollbarStyles styles = GetScrollbarStylesFromFrame();
>    bool isFocused = IsFocused(mOuter->GetContent());
> +  bool isVScrollable = ((scrollRange.height > 0)
> +                    && (styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN))
> +                    || (styles.mVertical == NS_STYLE_OVERFLOW_SCROLL);

I'm not so sure about this, I think it will result in a lot of things getting layerized when we don't want them to. We might need to add a more specific check.
Attachment #8413473 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Doug Sherk (:drs) from comment #29)
> Created attachment 8413473 [details] [diff] [review]
> Implement overlaid keyboard including manifest attribute to enable it.
> 
> Okay, here's another version of this. I need to get the coordinate
> conversions correct, but this overall makes sense to me.

I just tried the patch. I don't think you want to be able to overscroll the root scroll frame if it is not scrollable by default. Instead you want to overscroll inner scrollables, such as list.

Otherwise you ends up in a situation where you can move out of the screen the element of the app that are not intended to be scrollable at all. Such as the title bar or the homescreen content.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Comment on attachment 8413473 [details] [diff] [review]
> Implement overlaid keyboard including manifest attribute to enable it.
> 
> Review of attachment 8413473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - Does the call to setOverlayClip happen before or after the keyboard is
> actually made visible? The code in APZCTM::UpdatePanZoomControllerTree will
> only take effect on the next repaint *after* setOverlayClip is called, so if
> they repaint from the keyboard coming up already happened you may not get
> another repaint for a while.
> 

The call to setOverlayClip happens after the keyboard has been shown. (see https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L574 where callback is a method that will fire a keyboardchange event, that will fire the system-resize event, that will end up into calling setOverlayClip).

That said nothing visible to the user is expected at this point when the keyboard opens right ?

For hiding the call to setOverlayClip will happens before the keyboard is hidden.

> - It looks like from the gaia patch that you call setOverlayClip on a
> specific tabparent. I think then that you should be passing in the layers id
> from that TabParent/RenderFrameParent to the APZCTreeManager and only apply
> the overlay clip to APZC instances which have that layers id. Otherwise
> you'll end up applying it to the keyboard app as well which might have
> undesirable effects..
> 

Not sure to understand how could you end up applying the setOverlayClip to the keyboard ? I may be missing something here.

> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +1049,5 @@
> >    ScrollbarStyles styles = GetScrollbarStylesFromFrame();
> >    bool isFocused = IsFocused(mOuter->GetContent());
> > +  bool isVScrollable = ((scrollRange.height > 0)
> > +                    && (styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN))
> > +                    || (styles.mVertical == NS_STYLE_OVERFLOW_SCROLL);
> 
> I'm not so sure about this, I think it will result in a lot of things
> getting layerized when we don't want them to. We might need to add a more
> specific check.

In the initial patch I did that for one case in the settings app. We can apply that only if the manifest has an overflowClip right ?
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #33)
> The call to setOverlayClip happens after the keyboard has been shown. (see
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> keyboard_manager.js#L574 where callback is a method that will fire a
> keyboardchange event, that will fire the system-resize event, that will end
> up into calling setOverlayClip).
> 
> That said nothing visible to the user is expected at this point when the
> keyboard opens right ?

I don't know, I assumed the code that shows the keyboard made changes in layout synchronously, so the repaint would happen right then.

> Not sure to understand how could you end up applying the setOverlayClip to
> the keyboard ? I may be missing something here.

The keyboard itself is going to have a corresponding layer in the layer tree, and if you clip the composition bounds of every layer, it will clip the composition bounds of the keyboard layer too.

> > 
> > I'm not so sure about this, I think it will result in a lot of things
> > getting layerized when we don't want them to. We might need to add a more
> > specific check.
> 
> In the initial patch I did that for one case in the settings app. We can
> apply that only if the manifest has an overflowClip right ?

Actually thinking about this more, I'm not sure it's needed at all, as long as we only do the clip on the root scrollable layer for the app. That layer is always scrollable anyway and we shouldn't need to make anything else scrollable. What was the problem you were seeing that made you add that code at all?
Thanks for the really thorough feedback.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> This code doesn't make sense to me. It might make sense if you only applied
> it to the root APZ with the correct layers id. (See my general comment about
> this). But I forgot what we decided in our IRC discussion.

In the UI test app, I found that I was able to scroll an extra 50px or so below content. I also noticed that this corresponds with the offset of this frame. There's a header above it with a back button and "UI tests" title. I'm a bit out of my element when it comes to subframe scrolling, but Botond has been helping me out to understand it better. I'll see what happens when I apply this to the root only with the correct scaling.

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #32)
> I just tried the patch. I don't think you want to be able to overscroll the
> root scroll frame if it is not scrollable by default. Instead you want to
> overscroll inner scrollables, such as list.

Sorry, I don't follow this.

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #33)
> Not sure to understand how could you end up applying the setOverlayClip to
> the keyboard ? I may be missing something here.

I thought each app gets its own APZCTreeManager? If that's the case, there's no way this can accidentally be applied to the keyboard.

> In the initial patch I did that for one case in the settings app. We can
> apply that only if the manifest has an overflowClip right ?

Yeah, good point. From my reading it looks like that would work.
(In reply to Doug Sherk (:drs) from comment #35)
> I thought each app gets its own APZCTreeManager? If that's the case, there's
> no way this can accidentally be applied to the keyboard.

No, there's only one global APZCTreeManager, in the root process.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
> No, there's only one global APZCTreeManager, in the root process.

Well, that changes things. :) I'll fix that up.
I've not followed all of these comments, but I like the idea - I'd be strongly in favour of this being an attribute on the element rather than in the manifest.

In terms of implementation, this could be done using content-document fixed position margins (see nsIDOMWindowUtils::setContentDocuemntFixedPositionMargins) and the scroll-position clamping scroll-port size for overscroll (nsIDOMWindowUtils::setScrollPositionClampingScrollPortSize) - the former also has a compositor-side equivalent for smooth transitions too (we use it in Android for the dynamic toolbar).

The short of my comment is that we have all of the machinery to do this in layout already, including nice handling of fixed position content.
Flags: needinfo?(chrislord.net)
(In reply to Chris Lord [:cwiiis] from comment #38)
> I've not followed all of these comments, but I like the idea - I'd be
> strongly in favour of this being an attribute on the element rather than in
> the manifest.

Cool. I mentioned this in comment 9, which you might want to read if you haven't.

> In terms of implementation, this could be done using content-document fixed
> position margins (see
> nsIDOMWindowUtils::setContentDocuemntFixedPositionMargins) and the
> scroll-position clamping scroll-port size for overscroll
> (nsIDOMWindowUtils::setScrollPositionClampingScrollPortSize) - the former
> also has a compositor-side equivalent for smooth transitions too (we use it
> in Android for the dynamic toolbar).
> 
> The short of my comment is that we have all of the machinery to do this in
> layout already, including nice handling of fixed position content.

Interesting. This seems a lot better than my current approach. I'll work on doing it this way instead.
Attached patch Unusable layout-based patch. (obsolete) (deleted) — — Splinter Review 

    
    

    
  
(In reply to Doug Sherk (:drs) from comment #40)
> Created attachment 8415931 [details] [diff] [review]
> Unusable layout-based patch.

I accidentally left out my comment for this:

Ok, here's a followup on this. I wrote an implementation that works this way, but it turns out that calling nsIDOMWindowUtils::setScrollPositionClampingScrollPortSize reflows the entire page, which basically defeats the purpose of this. I tried getting it working anyways and was having problems with it not working as expected. Still, knowing about nsIDOMWindowUtils::setContentDocumentFixedPositionMargins is useful. Chris tells me that it only reflows fixed-position elements, so we can reflow them using this and still be faster than reflowing the entire app window. I did have problems getting this working, though.

Chris and I talked about how to do the compositor-only implementation that we've been discussing here so far. He says that this is impossible to ever get completely correctly. For example, we'll never be able to get pages to scroll correctly to input elements at the bottom of a page. We'll also have a hard time with fixed position layers. These problems suck but they are surmountable for some apps.

    
    

    
  
Unrelatedly, I spent a bunch of time with BenWa today talking about this and other optimizations that we can do. For starters, his belief is that the heuristics required here can get rapidly out of control and that the costs for development may outweight the benefits, especially since there are other ways to eke out better perf that don't involve doing things hackishly.

For example, he suggested applying will-change to some things, to look for sync reflows, investigating a potential breakout of the OMTA path for the keyboard that we think we saw, and not resizing the app window until the keyboard animation has actually been kicked off. He also thinks we should look at more profiles before deciding whether or not to go ahead with this bug, so I'm going to gather a bit more data and go over it with him.

    
    

    
  
(In reply to Doug Sherk (:drs) from comment #42)
> and not resizing the app window until the
> keyboard animation has actually been kicked off.

Filed bug 1004769.
See Also:  → https://bugzilla.mozilla.org/show_bug.cgi?id=1004769

    
    

    
  
Browser definitely benefit from a 'floating' keyboard, I'm currently experiencing all types of pain when site UIs gets squashed to unusable proportions that designers/developers never thought they'd have to cope with.

    
    

    
  
(In reply to Doug Sherk (:drs) from comment #42)
> Unrelatedly, I spent a bunch of time with BenWa today talking about this and
> other optimizations that we can do. For starters, his belief is that the
> heuristics required here can get rapidly out of control and that the costs
> for development may outweight the benefits, especially since there are other
> ways to eke out better perf that don't involve doing things hackishly.
> 
> For example, he suggested applying will-change to some things, to look for
> sync reflows, investigating a potential breakout of the OMTA path for the
> keyboard that we think we saw, and not resizing the app window until the
> keyboard animation has actually been kicked off. He also thinks we should
> look at more profiles before deciding whether or not to go ahead with this
> bug, so I'm going to gather a bit more data and go over it with him.

Any progress on this ?
Flags: needinfo?(drs+bugzilla)

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #46)
> Any progress on this ?

Yep! I got swamped with other work which pulled me off of this for a while. But I'm still working on it when I can.

I decided to really narrow down what we're doing here and create a reference app. [1] This reference app looks like a barebones version of the SMS app with a few layout changes such as flexbox/position-relative -> position-fixed. I did this so that I could show people what I'm doing when I'm talking with them about problems, and to have a clear objective for something that is useful in the real world. It also allows me to do more precise measurements to clear up unit conversion issues.

I ran into a hurdle with this conceptually, in that I don't think it's possible for it to cover as many use cases as we originally thought. For example, we can't handle most subframe scrolling cases, only ones where parent frames are essentially containers for the subframes without their own contents. I also ran into another problem: the compositor and APZC were out of sync on overlay clipping because the compositor makes the same assumptions as layout. Botond and I talked about this a week ago, and he thinks I'll have to interface with the compositor to get it working correctly. This scrolling issue is the last major hurdle that I'm currently aware of.

As with most other projects like this, the prototype was about 20% of the work. :) But I'm still optimistic about this.

[1] https://github.com/DouglasSherk/fxos-overlay-keyboard-reference-app
Flags: needinfo?(drs+bugzilla)

    
    

    
  
(In reply to Wilson Page [:wilsonpage] from comment #45)
> Browser definitely benefit from a 'floating' keyboard, I'm currently
> experiencing all types of pain when site UIs gets squashed to unusable
> proportions that designers/developers never thought they'd have to cope with.

I don't think we'll ever be able to turn this on for arbitrary web content, unfortunately. Apps will have to be specifically architected to work correctly with this.

    
  
Blocks: 995299

    
    

    
  
Eli, I don't think this is a good candidate for uplifting to 1.3T. Each app that opts into this will have to undergo some pretty serious changes in layout to support it, which would also have to be uplifted. We can revisit this once we have a better idea of what changes will be involved in the apps, but I'm not optimistic about it and I'm going to remove the blocking flag for now.
No longer blocks: 995299

    
    

    
      
  


  

    
    

    
  


  
Ok, here's a new version which works pretty well. Each app is still going to require significant changes to be able to support this, but it works pretty well with the reference app, which you can find here: http://people.mozilla.org/~dsherk/fxos-overlay-keyboard-reference-app/

Unfortunately, scrollbars aren't handled correctly and get obscured by the keyboard, but we can deal with that later.

The Gaia patch remains the same, for now.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31) 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1783,5 @@
> >      mFrameMetrics.mHasScrollgrab = aLayerMetrics.mHasScrollgrab;
> >  
> > +    // Compensate for any overlay clipping, such as the overlaid keyboard being
> > +    // on top of this frame.
> > +    mFrameMetrics.mCompositionBounds -= aOverlayClip;
> 
> You also need to do this in the other branch, where (aIsFirstPaint ||
> isDefault) is true.

I couldn't think of a case where this would matter, so I didn't do it. But if you can think of one or you think it's important for consistency, I will.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +1049,5 @@
> >    ScrollbarStyles styles = GetScrollbarStylesFromFrame();
> >    bool isFocused = IsFocused(mOuter->GetContent());
> > +  bool isVScrollable = ((scrollRange.height > 0)
> > +                    && (styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN))
> > +                    || (styles.mVertical == NS_STYLE_OVERFLOW_SCROLL);
> 
> I'm not so sure about this, I think it will result in a lot of things
> getting layerized when we don't want them to. We might need to add a more
> specific check.

I got rid of this. I wasn't sure why it was necessary. I might be painting more than I have to now, but this was really gross and I think we can use will-change instead.

        Attachment #8440415 -
        Attachment is obsolete: true

        Attachment #8440415 -
        Flags: review?(bugmail.mozilla)

        Attachment #8440417 -
        Flags: review?(bugmail.mozilla)
Alias: overlaid-keyboard

    
    

    
  
Comment on attachment 8440417 [details] [diff] [review]
Implement overlaid keyboard including manifest attribute to enable it.

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

Interesting approach overall, and pretty cleanly implemented I think. I have a bunch of comments below on the code. I also wanted to walk through an example to make sure the approach makes sense, particularly with respect to subframes. Consider a case like this:

<body style="height: 5000px">
 <div style="top: 200px; height: 200px">
  <div style="height: 1000px"></div>
 </div>
</body>

Assume a screen height of 500px, and the overlay clip height would be 250px.

So in this case the body is scrollable (because it has a 5000px scrollable rect with a 500px composition bounds). The first div is also scrollable (because it has a 1000px scrollable rect with a 200px composition bounds). The scrollTopMax values of the body and div are 4500px and 800px respectively.

Now let's apply the overlay clip. Based on my visualizing what would happen if the bottom 250px of the screen were covered, we should end up in a situation where the body has a 5000px scrollable rect and a 250px composition bounds, and the div's scrollable rect and composition bounds would remain unchanged. The scrollTopMax of the body would increase to 4750px so that you can still scroll all the way to the bottom. If you scrolled the div while leaving the body at scrollTop=0 then you wouldn't be able to see the bottom of the inner div. Is that right?

Based on this example I feel like the overlay clip should only ever really apply to the root apzc of a layers id, so I'm not sure that it makes sense to apply it on all the sub apzcs as well. Can you explain that part?

::: dom/interfaces/base/nsITabParent.idl
@@ +21,5 @@
>  
>    readonly attribute boolean useAsyncPanZoom;
>  
>    void setIsDocShellActive(in bool aIsActive);
> +  void setOverlayClip(in int32_t aWidth, in int32_t aHeight);

Some docs on this would be nice

::: gfx/2d/BaseRect.h
@@ +174,5 @@
>  
> +  // Subtract a rect from this one. We don't allow there to be a hole in the
> +  // resulting rect, so either the horizontal or vertical dimensions must be
> +  // the same between *this and aRect.
> +  Sub Subtract(const Sub& aRect)

Split this into a separate patch and flag Bas for review.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +236,5 @@
> +        while (rootApzc->GetParent() && !rootApzc->IsRootForLayersId()) {
> +          rootApzc = rootApzc->GetParent();
> +        }
> +        if (rootApzc && rootApzc->GetGuid().mLayersId == mOverlayClipLayersId) {
> +          overlayClip = mOverlayClip;

You don't need to walk up to the root apzc here. You can just do:

CSSSize overlayClip;
if (aLayersId == mOverlayClipLayersId) {
  overlayClip = mOverlayClip;
}

But see also my comment in AsyncPanZoomController.h

@@ +1350,5 @@
> +  Collect(mRootApzc, &allApzcs);
> +
> +  for (size_t i = 0; i < allApzcs.Length(); i++) {
> +    if (allApzcs[i]->GetGuid().mLayersId == aLayersId) {
> +      Collect(allApzcs[i], &subApzcs);

This seems inefficient and unnecessarily brittle (i.e. it relies on pre-order traversal in Collect). Just do subApzcs->AppendElement(allApzcs[i]) here and get rid of the break statement.

Alternatively, don't even bother with subApzcs, and just do allApzcs[i]->SetOverlayClip directly inside this if statement.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +297,5 @@
>  
> +  /**
> +   * Clips every layer in this tree by the width or height provided, from the
> +   * bottom right of the composition bounds. Only one or the other can be
> +   * non-zero at any time.

s/one or the other/the width or the height/

Also mention that setting an overlay clip clobbers any previous overlay clip (including one set for a different layers id).

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +709,5 @@
> +  // TODO: Put this in a better spot. We have no guarantees that a
> +  // NotifyLayersUpdated that we received is associated with a repaint that was
> +  // requested due to overlay clipping. So the only reliable place to unset this
> +  // is when we get an input event.
> +  mOverlayClipUpdated = false;

Can you not just put this at the bottom of SetOverlayClip, after the call to RequestContentRepaint? It looks like that's the only place it's ever used. You could even extract the part of RequestContentRepaint that's after the if condition into a separate helper method and call that directly, which would let you get rid of this mOverlayClipUpdated variable entirely.

@@ +2276,5 @@
> +    // either the root or a child of the root with overlay clipping applied.
> +    bool shouldCompensateScrollOffset =
> +      mOverlayClipUpdated && ShouldCompensateScrollOffsetOverlayClip(mOverlayClip);
> +    mFrameMetrics.SetRootCompositionSize(mFrameMetrics.GetRootCompositionSize() - mOverlayClip);
> +    mFrameMetrics.mCompositionBounds -= CalculateEffectiveOverlayClip(aOverlayClip);

This block should go away entirely if you change to calling SetOverlayClip from UpdatePanZoomControllerTree as I mentioned elsewhere. You'll probably need to bail out early in SetOverlayClip if the new clip is the same as the old clip.

@@ +2714,5 @@
> +
> +  if (aOverlayClip != CSSSize()) {
> +    ParentLayerSize computedOverlayClip = aOverlayClip
> +                                        * mFrameMetrics.mDevPixelsPerCSSPixel
> +                                        * mFrameMetrics.GetParentResolution();

I think this needs to be aOverlayClip * mFrameMetrics.GetZoomToParent().

@@ +2718,5 @@
> +                                        * mFrameMetrics.GetParentResolution();
> +    // The overlay clipping size is in terms of the composition bounds of this
> +    // layer, so we must round it to an integer.
> +    return ParentLayerIntSize(NS_lround(computedOverlayClip.width),
> +                              NS_lround(computedOverlayClip.height));

You should be able to use the RoundedToInt function defined at the bottom of gfx/2d/Point.h for this.

return RoundedToInt(computedOverlayClip);

@@ +2735,5 @@
> +  return aOverlayClip == CSSSize() ||
> +         ((aOverlayClip.width > 0 &&
> +           scrollableRect.width - compositionSize.width - scrollOffset.x <= aOverlayClip.width) ||
> +          (aOverlayClip.height > 0 &&
> +           scrollableRect.height - compositionSize.height - scrollOffset.y <= aOverlayClip.height));

This condition is very hard to read. Please break it down into smaller pieces assigned to local variables with meaningful names.

@@ +2749,5 @@
> +
> +  if (mOverlayClip.width > EPSILON) {
> +    scrollOffset.x = std::max(scrollableRect.width - compositionSize.width, 0.0f);
> +  } else if (mOverlayClip.height > EPSILON) {
> +    scrollOffset.y = std::max(scrollableRect.height - compositionSize.height, 0.0f);

I don't really understand why the scroll offset would need to change when the overlay clip is getting applied. Perhaps it will make sense when you explain the other question I had about the scrollable rect below.

@@ +2766,5 @@
> +void AsyncPanZoomController::SetOverlayClip(const CSSSize& aOverlayClip)
> +{
> +  // Don't allow both width and height to be set at the same time. We can only
> +  // clip along one axis.
> +  MOZ_ASSERT(aOverlayClip == CSSSize() || (!!aOverlayClip.width) ^ (!!aOverlayClip.height));

MOZ_ASSERT(!(aOverlayClip.width && aOverlayClip.height));

@@ +2777,5 @@
> +    // Unapply the old clipping.
> +    mFrameMetrics.mCompositionBounds += CalculateEffectiveOverlayClip(mOverlayClip);
> +    mFrameMetrics.SetRootCompositionSize(mFrameMetrics.GetRootCompositionSize() + mOverlayClip);
> +    if (mFrameMetrics.mIsRoot) {
> +      mFrameMetrics.mScrollableRect += mOverlayClip;

I don't understand why the scrollable rect size changes with the overlay clip. The scrollable area of the page is still the same as before, no?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +178,5 @@
>     * the initial metrics and the initial paint of the frame has just happened.
>     */
> +  void NotifyLayersUpdated(const FrameMetrics& aLayerMetrics,
> +                           bool aIsFirstPaint,
> +                           const CSSSize& aOverlayClip = CSSSize());

There should be no difference between
(a) calling NotifyLayersUpdated with an overlay clip C
(b) calling NotifyLayersUpdated with an empty overlap clip, followed immediately by a call to SetOverlayClip with C

Right? If so I would rather you always did the latter and don't change anything in NotifyLayersUpdated. (i.e. change the code in UpdatePanZoomControllerTree to call SetOverlayClip after calling NotifyLayersUpdated - that should make it cleaner).

@@ +323,5 @@
> +  /**
> +   * Clips this layer by the width and height provided, from the bottom right of
> +   * the composition bounds.
> +   */
> +  void SetOverlayClip(const CSSSize& aOverlayClip);

Move all of these new overlay-clip related things into a new section. See the existing "The functions and members in this section" comments in this file. Place your new section at the bottom of the class definition, just before the final close-brace.

@@ +576,5 @@
> +   * overlay clip that is to be applied. This should be called before applying
> +   * the clip. Returns true if the overlay clip is non-zero, and any of the
> +   * region that will be clipped is currently visible.
> +   */
> +  bool ShouldCompensateScrollOffsetOverlayClip(const CSSSize& aOverlayClip);

Rename this to ShouldCompensateScrollOffsetByOverlayClip

@@ +584,5 @@
> +   * scroll offset past the overlay clipping (since it's not aware of this), we
> +   * must manually compensate for it here. This basically sets the scroll
> +   * offset along the clipped axis to the end of the layer.
> +   */
> +  void CompensateScrollOffsetOverlayClip();

Rename this to CompensateScrollOffsetByOverlayClip

::: layout/ipc/RenderFrameParent.h
@@ +124,5 @@
>                               const ZoomConstraints& aConstraints);
>  
>    bool HitTest(const nsRect& aRect);
>  
> +  void SetOverlayClip(uint32_t aWidth, uint32_t aHeight);

Take a CSSIntSize instead of two ints. All the places you store this as CSSSize should really be CSSIntSize.

        Attachment #8440417 -
        Flags: review?(bugmail.mozilla) → review-

    
    

    
  
Please correct the component if I move this bug to the wrong place.
Component: Gaia::Keyboard → Layout
Product: Firefox OS → Core

    
    

    
  
Let's put it in panning and zooming for now since we are thinking of picking up this work again and most of the changes are conceptually on the compositor side of things.
Component: Layout → Panning and Zooming
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?

    
    

    
  
Doug said he wouldn't have time to continue working on this, so unassigning.
Assignee: drs+bugzilla → nobody
Status: ASSIGNED → NEW
feature-b2g: 2.2? → 2.2+

    
    

    
  
What's the bug representing corresponding required input mgmt change? Are you going to submit a Gaia patch here as well? Please create another bug (and feature-b2g: 2.2? it) so we could track it there.
Flags: needinfo?(milan)

    
    

    
  
This bug is tracking the Gecko side.  I believe the Gaia side is tracked (among others?) in bug 1025672.
Flags: needinfo?(milan)

    
    

    
  
Bruce, could you work with product from Milan to make sure dependency of our team is properly filed and flagged?
Flags: needinfo?(bhuang)
Depends on: 1091797
Summary: Propose an attribute in the manifest to not resize the window when the keyboard is shown/hidden → Implement overlaid keyboard to not require reflow when showing and hiding the keyboard

    
    

    
  
This got knocked out of 2.2 with the date changes and no current plan to use it on the Gaia side for 2.2
blocking-b2g: --- → backlog
feature-b2g: 2.2+ → ---
Flags: needinfo?(bhuang)

    
    

    
  
Note that we're working on this problem in Chrome Android as well, and have already implemented it for ChromeOS (and IE has a similar implementation).  References:

https://code.google.com/p/chromium/issues/detail?id=404315
https://code.google.com/p/chromium/issues/detail?id=410894
https://docs.google.com/document/d/1h0tKgGO5tk3Kma7kaYRUDy43xbChXMAFRMhZXwuWYXU/edit#heading=h.outn16su5mo6

To do this really well, I suspect we'll need some new web-exposed APIs (Eg. maybe position: device-fixed like IE), so hopefully we can all work together in CSSWG on how to do this right.  See bug 1123938 for more discussion here.
blocking-b2g: backlog → ---

          tracking-b2g:
          --- → backlog

    
    

    
  
I was going to pick up this bug and started with a profile to see how bad the problem is:


I made a video walk through of the a profile opening the keyboard. A new experiment I'm trying to make sure everyone is on the same page when using performance data to guide the discussion:

http://people.mozilla.org/~bgirard/keyboard_timeline.mp4

As you'll notice:
1) We do work that looks like it doesn't need to happen. We do several system-process reflows when conceptually we only need to reflow the child app.
2) We're executing (almost) everything serially. If we weren't we could hide a lot of the app' execution since we're bottlenecking on the system process.
3) Looks like a lot of the work in the system process can be avoided. Why are we reflowing the keyboard? We should be able to have the keyboard fully built and just slide it into view. But instead we reflow it twice.
4) Very little time is spent reflowing the app. Even if we tested this on an app that was slower than the messaging up the system process execution would still bottleneck. The app's reflow would have to be take in the order of ~400ms+ for it to be noticeable.
5) Many things that aren't clear like a) Why are we painting so many times, b) Why are we spending so much time running JS/DOM manipulation in the system process. c) Why are we checking settings across IPC and performing costly JSON work in a critical path.

Looking at this data, if we solve bug 970093 it's not going to give us better performance, except for the apps that are really poorly optimized. Also avoiding the reflow means that we have to make some serious hacks which wont work well for certain apps and will be a large effort to build & maintain.

I'm actually going to propose that we WONTFIX this bug until we have performance profiles, or other data, proving that this bug is worthwhile. This wont happen until we do a lot of improvements to the above issues.

    
    

    
  
Interesting. One question I have is whether you were using the in-process keyboard or the out-of-process keyboard. The in-process keyboard (which is used if for example you configure the flame to have 319mb of RAM) lives in the system process and might very well trigger reflows in the system process. The out-of-process keyboard (1GB flame configuration) probably doesn't need to reflow the system process.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62)
> Interesting. One question I have is whether you were using the in-process
> keyboard or the out-of-process keyboard.

The profile shows the keyboard execution in the system process. This would certainly prove that we're using OOP keyboard I think.

    
    

    
  
Let me be more clear. At 0:37 we see PresShell:DoReflow (app://keyboard.gaiamobile.org/index.html#en) executing under the b2g process. I believe that confirms that the keyboard reflow is happening in the system process and that the keyboard is not running in the app.

    
    

    
  
There may be a misunderstanding. In the out-of-process case the keyboard gets its own separate process (you can see it listed as "Keyboard" if you run |adb shell b2g-ps|. I don't think the keyboard ever runs in the app process. Based on what you said it sounds like you're using the in-process keyboard.

    
    

    
  
Ohh yes, you're correct.

    
    

    
  
This bug does not just concern performance.

From an app developer perspective, it is very inconvenient for the viewport to get squashed so short when the keyboard opens.

UI that has not been specifically designed for viewports this short becomes squashed into half the vertical space and can become unusable. Pieces of UI suddenly jump around and overlap, resulting in an overall disruptive experience.

The keyboard opens only once text-input field is focused. While an input is focused we can assume the user will be interested in two things: the keyboard and the field. Resizing the window, and rearranging the app's UI only disrupts this task.

As comment 60 indicates, both ChromeOS and IE have avoided this UX.

    
    

    
  
To be more specific this bug affected the Telegram Firefox OS app. When the keyboard opened the country input's select dropdown got squashed so small you couldn't select an option.

    
    

    
  
(In reply to Benoit Girard (:BenWa) from comment #64)
> Let me be more clear. At 0:37 we see PresShell:DoReflow
> (app://keyboard.gaiamobile.org/index.html#en) executing under the b2g
> process. I believe that confirms that the keyboard reflow is happening in
> the system process and that the keyboard is not running in the app.

Keyboard apps are oop'd if total memory report back is > 512M. Are you running 319M config?

    
    

    
  
(In reply to Benoit Girard (:BenWa) from comment #61)
> Looking at this data, if we solve bug 970093 it's not going to give us
> better performance, except for the apps that are really poorly optimized.

This kind of comments keeps making me uncomfortable.

Gaia app developers keeps spending their time on optimizing things to avoid platform slow paths or just to have a rendering that is not too ugly for arbitrary sizes (as the keyboard size is unknown).

It seems unreasonable to me to have developers spending their time on optimizing their apps for some keyboard resizing performances instead of working on working on their application logic.


> I'm actually going to propose that we WONTFIX this bug until we have
> performance profiles, or other data, proving that this bug is worthwhile.

Just trying the first patch I made on some apps was clearly showing visual improvements last time I tried (more than one year ago tbh), without the need for any profiling. (not saying profiling is not needed!).

I'm curious, don't you see any visual improvements with the 2 original patches of this bug ?

    
    

    
  
In the video profile, the "mozScrollAreaChanged" changed event that triggers a reflow (which is a bug on BrowserElementChildPreload.js) seems strange. I'm not sure why the keyboard is emitting such an event.

Was it the first time the keyboard was opened ?

    
    

    
  
Alright as pointed above there two keyboard configuration. I'll profile the other one and see where that stands.

So there's two things here that got conflated here: 1) Giving apps a different keyboard behavior 2) Providing a platform fast path.

My analysis above was if we should invest in building and maintain a complex platform fast path, like the path above that modify the compositor, and my conclusion, at least for the system keyboard, is that we should not. I take back that we should 'WONTFIX' this bug, if we want to do 1) that's fine, but we already have all the platform pieces to achieve 1) without doing 2).

What we started discussing in this bug, the component this bug is filled under, and the patches here, were special case fast paths. Given the data we have so far it's premature optimization to optimize non bottlenecks.

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #70)
> (In reply to Benoit Girard (:BenWa) from comment #61)
> > Looking at this data, if we solve bug 970093 it's not going to give us
> > better performance, except for the apps that are really poorly optimized.
> 
> This kind of comments keeps making me uncomfortable.
> 
> Gaia app developers keeps spending their time on optimizing things to avoid
> platform slow paths or just to have a rendering that is not too ugly for
> arbitrary sizes (as the keyboard size is unknown).
> 
> It seems unreasonable to me to have developers spending their time on
> optimizing their apps for some keyboard resizing performances instead of
> working on working on their application logic.
> 

I'm presenting data to says that this wont measurably improve performance. I'm not advocating that we shift work from platform developers onto gaia developers. I'm saying we shouldn't work on something that wont yield a worthwhile benefit because it means taking away from something that will.

If you have data to support that this requires a platform optimizations it would really help drive this forward.

> 
> > I'm actually going to propose that we WONTFIX this bug until we have
> > performance profiles, or other data, proving that this bug is worthwhile.
> 
> Just trying the first patch I made on some apps was clearly showing visual
> improvements last time I tried (more than one year ago tbh), without the
> need for any profiling. (not saying profiling is not needed!).
> 
> I'm curious, don't you see any visual improvements with the 2 original
> patches of this bug ?

I'd love to see real data here then if that's the case.

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #71)
> In the video profile, the "mozScrollAreaChanged" changed event that triggers
> a reflow (which is a bug on BrowserElementChildPreload.js) seems strange.
> I'm not sure why the keyboard is emitting such an event.
> 
> Was it the first time the keyboard was opened ?

No, I got a profile of the first keyboard open but left it out because the reflow performance matters even less in that case before we're doing a bunch of work to build the keyboard.

I'll post a profile of the other keyboard case as soon as I have it.

    
    

    
  
Here's the profile, I can do a video walk-through if requested:
http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0

This profile is rather similar. It's still mostly serial but there's a bit more concurrent process, but not a ton. If we're looking at making the keyboard faster I think we should focus on the general keyboard performance and not on a special optimization that will only benefit apps that 1) Opt into that behavior 2) That need ~600ms to reflow.

    
    

    
  
(In reply to Benoit Girard (:BenWa) from comment #73)
> Here's the profile, I can do a video walk-through if requested:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=3cb067126f3d74554d11a0469f83453d3b5a9cf0

I would be very interested in a video work-through; I will work with people in Taipei to understand this profile first.

> This profile is rather similar. It's still mostly serial but there's a bit
> more concurrent process, but not a ton. If we're looking at making the
> keyboard faster I think we should focus on the general keyboard performance
> and not on a special optimization that will only benefit apps that 1) Opt
> into that behavior 2) That need ~600ms to reflow.

    
    

    
  
So here is what Thinker taught me to read:

1. with "handleFocus" filter

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&search=handleFocus&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A22668,%22end%22%3A23286}]

forms.js does not actually start collecting data and notify the keyboard after the real touch event for some time. bug 1162360 is likely to help a bit in the content process, but something related to selection caret and clipboard etc. are being handled at the same time and blocked.

2. With the filter "input_window"

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&search=input_window&selection=0,1,14,15,16,15,246,27,15,297,15,299,300,120,27,15,297,15,301,15,302,303,304,15,306,307

Once can see how System app in the b2g process is handling the focus message. It sets the keyboard frame to visibility active, and also input method active.

3. For some reason, right when we set the keyboard frame to active, RecvFlushMemory kicks in and block as for ~140ms to do anything.

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&filter=[{%22type%22%3A%22FocusedCallstackPrefixSampleFilter%22,%22name%22%3A%22IPDL%3A%3APContent%3A%3ARecvFlushMemory%22,%22focusedCallstack%22%3A[0,710,712],%22appliesToJS%22%3Afalse}]

4. First thing after that is to repaint the frame we throw away. It took ~50ms.

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&filter=[{%22type%22%3A%22FocusedCallstackPrefixSampleFilter%22,%22name%22%3A%22PresShell%3A%3APaint%22,%22focusedCallstack%22%3A[0,710,148],%22appliesToJS%22%3Afalse}]

Another PVSync:Notify trigger another 38ms paint on the keyboard frame.

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A23106,%22end%22%3A23256},{%22type%22%3A%22FocusedCallstackPrefixSampleFilter%22,%22name%22%3A%22IPDL%3A%3APVsync%3A%3ARecvNotify%22,%22focusedCallstack%22%3A[0,710,810],%22appliesToJS%22%3Afalse}]

We thought bug 1154231 should help us here but it's unclear to me this profile comes with that patch.

5. Finally the information of the input is in the keyboard process. The profile is spotty here indicating the system is busying on something not captured. Notably, it took ic_init() 16ms just to install some DOMRequestHelper message listeners...

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&filter=[{%22type%22%3A%22FocusedCallstackPrefixSampleFilter%22,%22name%22%3A%22IPDL%3A%3APContent%3A%3ARecvAsyncMessage%22,%22focusedCallstack%22%3A[0,710,254],%22appliesToJS%22%3Afalse}]&selection=254,15

6. The keyboard app itself does not get to handle the focus all the way at 23232ms (filter w/ "state_manager") 

http://people.mozilla.org/~bgirard/cleopatra/#report=3cb067126f3d74554d11a0469f83453d3b5a9cf0&search=state_manager&selection=0,710,254,15,827,830,27,15,843

7. At 23264ms we called _updateHeight() indicates the keyboard app think itself is ready and trigger the rising transition, but only until 23304ms the System app receives the message and began the transition. It is unclear to me what has holding that for this long from the profile (40ms)

8. "_resize" filter indicates we resize the message frame at 23584ms. (280ms)

Based on the interpretation above keyboard app itself only takes 32ms ms to do it's things and the whole process takes more than 500ms. Fixing (1) and (3) is probably the better solution here.

BenWa, please correct me if anything above is incorrect. Thanks!

    
    

    
  
Yes, looks like a great analysis.

The profile was using a build from June 1st so the Gecko changes from bug 1154231 were landed.
Keywords: perf
OS: All → Gonk (Firefox OS)
Whiteboard: gfx-noted

    
    

    
  
B2G specific bug, closing as it is unlikely anybody will do this any more.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: