1231517 - Input fields are not scrolled into view correctly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

If you focus an input field that's at the bottom of the screen, it is not correctly scrolled into view.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Move-GetBoundingContentRect-to-nsLayoutUtils-16011917-21556e1.patch

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0002-Add-support-for-ZoomToRect-behavior-flags-16011917-278755e.patch

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0003-Add-support-for-ZoomToRect-to-nsIWidget-classes-16011917-18071e6.patch

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0004-Add-ZoomToFocusedInput-to-nsDOMWindowUtils-16011917-6b3cd5b.patch

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0005-Update-scrollToFocusedInput-to-use-nsDOMWindowUtils.zoomToFocusedInput-when-APZ-is-enabled-16011917-69e57ad.patch

Comment 6

[Randall Barker [:rbarker]]

Who should I ask to review the changes in the nsDOMWindowUtils and nsIWidget patches?

Comment 7

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

I can review those.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8709770 [details] [diff] [review]
0005-Update-scrollToFocusedInput-to-use-nsDOMWindowUtils.zoomToFocusedInput-when-APZ-is-enabled-16011917-69e57ad.patch

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

Good for now, but maybe put a link to this bug or a new one regarding the timeout usage.

Comment 9

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

Comment on attachment 8709766 [details] [diff] [review]
0002-Add-support-for-ZoomToRect-behavior-flags-16011917-278755e.patch

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +211,5 @@
>     * Kicks an animation to zoom to a rect. This may be either a zoom out or zoom
>     * in. The actual animation is done on the compositor thread after being set
>     * up. |aRect| must be given in CSS pixels, relative to the document.
>     */
>    void ZoomToRect(const ScrollableLayerGuid& aGuid,

Update the comment to say that |aFlags| is a combination of the ZoomToRectBehavior enum values. I would have preferred using the enum instead of uint32_t here but I see that we do this pretty inconsistently across APZCTreeManager already so we can clean it up in a follow-up bug.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3427,1 @@
>        zoomOut = true;

The if condition is getting hard to read. Please rewrite it to be like this:

bool zoomOut;
if (aFlags & DISABLE_ZOOM_OUT) {
  zoomOut = false;
} else {
  zoomOut = (aRect.IsEmpty() || ... || ...);
}
if (zoomOut) {
  ...

Comment 10

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

Comment on attachment 8709767 [details] [diff] [review]
0003-Add-support-for-ZoomToRect-to-nsIWidget-classes-16011917-18071e6.patch

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

Mostly fine, just a couple of things below I'd like to see addressed. Also a heads-up: depending on who lands first you may have to rebase this on top of bug 1020199, which moves the ZoomToRect function to a new APZChild.cpp file as part of the new PAPZ protocol. It should be a relatively straightforward rebase, since you're just modifying the signature rather than adding or removing a function.

::: widget/PuppetWidget.cpp
@@ +1434,5 @@
> +  }
> +
> +  uint32_t flags = layers::DISABLE_ZOOM_OUT;
> +  if (!Preferences::GetBool("formhelper.autozoom")) {
> +    flags |= layers::PAN_INTO_VIEW_ONLY;

I think the flags should be computed at the callsite of nsIWidget::ZoomToRect and passed in as a parameter, rather than being computed here. We may want to call ZoomToRect for other reasons later and this assumes that all those reasons will be related to form input which I don't think is reasonable. Also moving it to the call site (nsDOMWindowUtils.cpp in part 4) will avoid duplicating it in nsBaseWidget.cpp as well as here.

::: widget/nsBaseWidget.cpp
@@ +1813,5 @@
> +  }
> +  uint64_t layerId = mCompositorParent->RootLayerTreeId();
> +  uint32_t flags = layers::DISABLE_ZOOM_OUT;
> +  if (!Preferences::GetBool("formhelper.autozoom")) {
> +    flags |= layers::PAN_INTO_VIEW_ONLY;

Move to callsite, see previous comment.

::: widget/nsIWidget.h
@@ +2069,5 @@
>       * widget.  Note that this never returns nullptr.
>       */
>      NS_IMETHOD_(TextEventDispatcher*) GetTextEventDispatcher() = 0;
>  
> +    virtual void ZoomToRect(const uint32_t& aPresShellId,

IID bump in this file.

Comment 11

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Comment on attachment 8709766 [details] [diff] [review]
> 0002-Add-support-for-ZoomToRect-behavior-flags-16011917-278755e.patch
> 
> Review of attachment 8709766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/APZCTreeManager.h
> @@ +211,5 @@
> >     * Kicks an animation to zoom to a rect. This may be either a zoom out or zoom
> >     * in. The actual animation is done on the compositor thread after being set
> >     * up. |aRect| must be given in CSS pixels, relative to the document.
> >     */
> >    void ZoomToRect(const ScrollableLayerGuid& aGuid,
> 
> Update the comment to say that |aFlags| is a combination of the
> ZoomToRectBehavior enum values. I would have preferred using the enum
> instead of uint32_t here but I see that we do this pretty inconsistently
> across APZCTreeManager already so we can clean it up in a follow-up bug.
> 

I think using uint32_t is correct since it can hold more than one enum. So something like:

ZoomToRectBehavior flags = static_cast<ZoomToRectBehavior>(PAN_INTO_VIEW_ONLY | DISABLE_ZOOM_OUT);

The value of "flags" would be 3 which is not a valid ZoomToRectBehavior enum. I would have used const vars for the flag values but saw there was already an enum in APZCTreeManager.h defining flag values so I went with consistency.

Comment 12

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

Comment on attachment 8709768 [details] [diff] [review]
0004-Add-ZoomToFocusedInput-to-nsDOMWindowUtils-16011917-6b3cd5b.patch

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +2447,5 @@
> +  if (!shell) {
> +    return NS_OK;
> +  }
> +
> +  nsIScrollableFrame* rootScrollFrame = shell->GetRootScrollFrameAsScrollable(); 

nit: trailing whitespace

@@ +2462,5 @@
> +  FrameMetrics::ViewID viewId;
> +  if (APZCCallbackHelper::GetOrCreateScrollIdentifiers(document->GetDocumentElement(), &presShellId, &viewId)) {
> +    CSSRect bound = nsLayoutUtils::GetBoundingContentRect(content, rootScrollFrame);
> +    // Add margin to bound here.
> +    widget->ZoomToRect(presShellId, viewId, bound);

This is where I would put the flags computation.

Also, the "Add margin to bound here" comment - is that something you're planning on doing? It should be easy enough with bound.Inflate(5) or whatever.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1460,5 @@
>  
>    /**
> +   * Ask APZ to pan and zoom to the focused input element.
> +   */
> +  void zoomToFocusedInput();

UUID bump in this file.

Comment 13

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

(In reply to Randall Barker [:rbarker] from comment #11)
> I think using uint32_t is correct since it can hold more than one enum. So
> something like:
> 
> ZoomToRectBehavior flags =
> static_cast<ZoomToRectBehavior>(PAN_INTO_VIEW_ONLY | DISABLE_ZOOM_OUT);
> 
> The value of "flags" would be 3 which is not a valid ZoomToRectBehavior
> enum. I would have used const vars for the flag values but saw there was
> already an enum in APZCTreeManager.h defining flag values so I went with
> consistency.

That's fair. And yeah, the one in APZCTreeManager.h that's there already does this. However personally I prefer the approach used for CancelAnimationFlags in APZUtils.h, where we define a operator| just below it, so you can or multiple values together and it still returns an enum value (even though it's a bitfield-combination). Anyway, like I said, we do this inconsistently now and I can clean it up later.

Comment 14

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Comment on attachment 8709768 [details] [diff] [review]
> 0004-Add-ZoomToFocusedInput-to-nsDOMWindowUtils-16011917-6b3cd5b.patch
> 
> Review of attachment 8709768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, the "Add margin to bound here" comment - is that something you're
> planning on doing? It should be easy enough with bound.Inflate(5) or
> whatever.
> 

Yeah, I wasn't sure what we want to do here. In ZoomHelper.js we just add 30 to the width of the bounds and subtract 15 from the x position. I thought maybe we could do better, maybe it's a follow up bug.

Comment 15

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

For now I'd say just do the same, so bound.Inflate(15, 0) should do it. We can improve it later if need be.

Comment 16

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> For now I'd say just do the same, so bound.Inflate(15, 0) should do it. We
> can improve it later if need be.

It won't work exactly the same because the x value of the bounds is checked to make sure it doesn't go off the page. It if does, then it is reset. This has the side effect of making it 30 wider on the right side instead of 15.

Comment 17

[Randall Barker [:rbarker]]

(In reply to Randall Barker [:rbarker] from comment #16)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> > For now I'd say just do the same, so bound.Inflate(15, 0) should do it. We
> > can improve it later if need be.
> 
> It won't work exactly the same because the x value of the bounds is checked
> to make sure it doesn't go off the page. It if does, then it is reset. This
> has the side effect of making it 30 wider on the right side instead of 15.

What I'm describing is how it works in ZoomHelper.js. I noticed it wasn't very clear.

Comment 18

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

x -= 15 with width += 30 should pad it by 15 on either side. (i.e. XMost() will go up by 15). However you're right that it shouldn't go off the page, and that might need some plumbing. At least update the comment then to say that it needs to be bounds-checked after inflation.

Comment 19

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> x -= 15 with width += 30 should pad it by 15 on either side. (i.e. XMost()
> will go up by 15). However you're right that it shouldn't go off the page,
> and that might need some plumbing. At least update the comment then to say
> that it needs to be bounds-checked after inflation.

AsyncPanZoomController::ZoomToRect makes sure that we don't overscroll so having x go off the screen shouldn't cause any problems. I was just pointing out that AsyncPanZoomController::ZoomToRect and ZoomHelper.js handle this case differently so the end result is a little different (AsyncPanZoomController::ZoomToRect actually seems correct to me).

Comment 20

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1231517-part-3-Add-ZoomToRect-function-to-nsIWidget-classes-r-16012013-9699295.patch

Address review comments.

Comment 21

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1231517-part-1-Move-GetBoundingContentRect-to-nsLayoutUtils-r-botond-16012013-1a8bc15.patch

Carry forward r+ from :botond

Comment 22

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1231517-part-2-Add-support-for-APZ-ZoomToRectBehavior-flags-r-kats-16012013-d376c7f.patch

Carry forward r+ from :kats

Comment 23

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1231517-part-4-Add-ZoomToFocusedInput-function-to-nsDOMWindowUtils-r-kats-16012013-816b39b.patch

Carry forward r+ from :kats

Comment 24

[Randall Barker [:rbarker]]

Attachment: 0005-Bug-1231517-part-5-Update-scrollToFocusedInput-in-browser.js-to-use-nsDOMWindowUtils.zoomToFocusedInput-when-APZ-is-enabled-r-snorp-16012013-78e8eea.patch

Carry forward r+ from :snorp

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5be83ccccf
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab959594e900
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc568b890f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e6ed62772b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eff79b3a5a5

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b5be83ccccf
https://hg.mozilla.org/mozilla-central/rev/ab959594e900
https://hg.mozilla.org/mozilla-central/rev/cfc568b890f8
https://hg.mozilla.org/mozilla-central/rev/55e6ed62772b
https://hg.mozilla.org/mozilla-central/rev/2eff79b3a5a5

Comment 27

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc50c7b478f0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8d148f7366

Comment 28

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1231517-part-1-Move-GetBoundingContentRect-to-nsLayoutUtils-r-botond-16012109-e38214b.patch

Comment 29

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1231517-part-2-Add-support-for-APZ-ZoomToRectBehavior-flags-r-kats-16012109-446e01a.patch

Comment 30

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1231517-part-3-Add-ZoomToRect-function-to-nsIWidget-classes-r-kats-16012109-2a9d831.patch

Comment 31

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1231517-part-4-Add-ZoomToFocusedInput-function-to-nsDOMWindowUtils-r-kats-16012109-55239c9.patch

Comment 32

[Randall Barker [:rbarker]]

Attachment: 0005-Bug-1231517-part-5-Update-scrollToFocusedInput-in-browser.js-to-use-nsDOMWindowUtils.zoomToFocusedInput-when-APZ-is-enabled-r-snorp-16012109-84d6824.patch