Because of bug 825692, input fields don't get layerized and so are not scrollable with APZ is enabled. To test this I went to the contacts app screen to add a new contact, entered a really long name in the field, and tried to scroll the input field. The field scrolls with APZ disabled but not with APZ enabled. It scrolls if I revert bug 825692. I don't know if reverting bug 825692 now is safe or if it will still result in performance problems. At the very least we should allow layerizing input fields with focus even if we don't layerize all of them.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0) > Because of bug 825692, input fields don't get layerized and so are not > scrollable with APZ is enabled. To test this I went to the contacts app > screen to add a new contact, entered a really long name in the field, and > tried to scroll the input field. The field scrolls with APZ disabled but not > with APZ enabled. It scrolls if I revert bug 825692. > > I don't know if reverting bug 825692 now is safe or if it will still result > in performance problems. At the very least we should allow layerizing input > fields with focus even if we don't layerize all of them. Layerizing input with focus sounds a nice approach.

Talking to Vivien on IRC, this needs to be done for 1.3. Botond, is this something you can look at this week?

I worked on this a little bit after I thought I was done with bug 947337 (which it turns out I'm not). I have WIPs that I think should work but that I haven't tested at all. Will upload in a sec.

Not sure if this is 100% functionally equivalent but I think conceptually accomplishes the same thing.

The above patches were breaking panning in apps for me. So I played a little bit with the idea and comes with this wip to see how it works. With this editable elements are layerize and panning works generally but I still see 2 issues: - Panning the bottom input of the sms app does not work very well and strange things happened. I wonder if this is related to the fact that this textbox is actually not a textbox but a contenteditable div. - When deleting characters, the layer is not repositioned correctly and we what you're currently editing can be out of view. It repositionned itself correctly as soon as you start to pan the editable element.

Oups. I attached the wrong version of the patch.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7) > Created attachment 8347616 [details] [diff] [review] > bug946408.wip.patch > > The above patches were breaking panning in apps for me. So I played a little > bit with the idea and comes with this wip to see how it works. > > With this editable elements are layerize and panning works generally but I > still see 2 issues: > - Panning the bottom input of the sms app does not work very well and > strange things happened. I wonder if this is related to the fact that this > textbox is actually not a textbox but a contenteditable div. > - When deleting characters, the layer is not repositioned correctly and we > what you're currently editing can be out of view. It repositionned itself > correctly as soon as you start to pan the editable element. An other issue is that once you have started to pan the input field the page pan as well. Very annoying.

Comment on attachment 8345884 [details] [diff] [review] (WIP) Part 1 - Enable subapzc always (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7) > The above patches were breaking panning in apps for me. The problem is the part 1 patch. It enables APZ in the parent process, and that causes some problems right now (see bug 950934 for details). Since enabling APZ in the parent process doesn't really have anything to do with making input fields scrollable, I moved the part 1 patch to bug 950934.

Uploaded updated WIP patches. (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7) > - Panning the bottom input of the sms app does not work very well and > strange things happened. I wonder if this is related to the fact that this > textbox is actually not a textbox but a contenteditable div. This now works with my patches. The trick was to find the right content element to check for being focused. > - When deleting characters, the layer is not repositioned correctly and we > what you're currently editing can be out of view. It repositionned itself > correctly as soon as you start to pan the editable element. This is still an issue. I will investigate. (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9) > An other issue is that once you have started to pan the input field the page > pan as well. Very annoying. This is caused by scroll handoff from the child APZC to the parent APZC. Kats said on IRC that this issue probably shouldn't block this bug from landing, so I will open a new bug for it.

(In reply to Botond Ballo [:botond] from comment #14) > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9) > > An other issue is that once you have started to pan the input field the page > > pan as well. Very annoying. > > This is caused by scroll handoff from the child APZC to the parent APZC. > Kats said on IRC that this issue probably shouldn't block this bug from > landing, so I will open a new bug for it. I filed bug 951793.

(In reply to Botond Ballo [:botond] from comment #14) > > - When deleting characters, the layer is not repositioned correctly and we > > what you're currently editing can be out of view. It repositionned itself > > correctly as soon as you start to pan the editable element. > > This is still an issue. I will investigate. This is no longer an issue with latest trunk, thanks to the removal of UpdateScrollOffset. The problem before the removal of UpdateScrollOffset was that the scroll events generated by Gecko for scrolling the caret into view after a delete operation were not received by the scroll event handler in TabChild (not sure why - I didn't investigate this as that code path was removed anyways) and thus didn't make it to APZ via UpdateScrollOffset. With the removal of UpdateScrollOffset, the new scroll offset makes it to APZ via NotifyLayersUpdated and all is well. I think that means these patches are ready for review.

Note for reviewers: Vivien's patch additionally conditioned the extra layerization on 1) the platform being B2G and 2) the content being editable. My patch doesn't have these conditions, but I can add them if you'd like.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=77cb08630109

Comment on attachment 8349559 [details] [diff] [review] Part 1 - Refactor for readability Review of attachment 8349559 [details] [diff] [review]: ----------------------------------------------------------------- I shouldn't review this since I wrote the original version

Comment on attachment 8349562 [details] [diff] [review] Part 2 - Always layerize a scrollable element if it has the focus Review of attachment 8349562 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2234,5 @@ > + // for a text input field, are inside anonymous subtrees, but the focus > + // manager always reports a non-anonymous element as the focused one, so > + // walk up the tree until we reach a non-anonymous element. > + while (aContent && aContent->IsInAnonymousSubtree()) > + aContent = aContent->GetParent(); I like braces on single-line bodies and it looks like that's the prevailing style in this file too. @@ +2236,5 @@ > + // walk up the tree until we reach a non-anonymous element. > + while (aContent && aContent->IsInAnonymousSubtree()) > + aContent = aContent->GetParent(); > + > + return nsContentUtils::IsFocusedContent(aContent); aContent could be null here, we should probably return false if that is the case.

Comment on attachment 8349559 [details] [diff] [review] Part 1 - Refactor for readability The scrollbox check is there from bug 825692 to specifically not build layers for inputs. This is not clear from the code, but that is how it was before you got there.

Updated part 2 patch to address review comments. Carrying r+.

(In reply to Botond Ballo [:botond] from comment #18) > Try push: https://tbpl.mozilla.org/?tree=Try&rev=77cb08630109 Looks clean.

https://hg.mozilla.org/integration/b2g-inbound/rev/635a00e98554 https://hg.mozilla.org/integration/b2g-inbound/rev/c0da64bf920c

https://hg.mozilla.org/mozilla-central/rev/635a00e98554 https://hg.mozilla.org/mozilla-central/rev/c0da64bf920c

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c2fd03413da https://hg.mozilla.org/releases/mozilla-aurora/rev/fef0548e5714