946408 - Input fields are not scrollable with APZC enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

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.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 3

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

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.

Comment 4

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

Attachment: (WIP) Part 1 - Enable subapzc always

Comment 5

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

Attachment: (WIP) Part 2 - Refactor for readability

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

Comment 6

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

Attachment: (WIP) Part 3 - Layerize if focused

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: 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.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug946408.wip.patch

Oups. I attached the wrong version of the patch.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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 10

[Botond Ballo [:botond]]

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.

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 1 - Refactor for readability

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 2 - Always layerize a scrollable element if it has the focus

Comment 14

[Botond Ballo [:botond]]

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.

Comment 15

[Botond Ballo [:botond]]

(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.

Comment 16

[Botond Ballo [:botond]]

(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.

Comment 17

[Botond Ballo [:botond]]

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.

Comment 18

[Botond Ballo [:botond]]

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

Comment 19

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

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 20

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

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 21

[Timothy Nikkel (:tnikkel)]

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.

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 2 - Always layerize a scrollable element if it has the focus

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

Comment 23

[Botond Ballo [:botond]]

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

Looks clean.

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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