Closed Bug 1231517 Opened 4 years ago Closed 4 years ago

Input fields are not scrolled into view correctly

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: snorp, Assigned: rbarker)

References

Details

Attachments

(5 files, 10 obsolete files)

7.50 KB, patch
Details | Diff | Splinter Review
7.52 KB, patch
Details | Diff | Splinter Review
14.97 KB, patch
Details | Diff | Splinter Review
4.67 KB, patch
Details | Diff | Splinter Review
2.41 KB, patch
Details | Diff | Splinter Review
If you focus an input field that's at the bottom of the screen, it is not correctly scrolled into view.
Assignee: nobody → rbarker
Attachment #8709765 - Flags: review?(tnikkel)
Attachment #8709765 - Flags: review?(botond)
Attachment #8709766 - Flags: review?(bugmail.mozilla)
Attachment #8709770 - Flags: review?(snorp)
Who should I ask to review the changes in the nsDOMWindowUtils and nsIWidget patches?
Flags: needinfo?(snorp)
I can review those.
Flags: needinfo?(snorp)
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.
Attachment #8709770 - Flags: review?(snorp) → review+
Attachment #8709767 - Flags: review?(bugmail.mozilla)
Attachment #8709768 - Flags: review?(bugmail.mozilla)
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) {
  ...
Attachment #8709766 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8709767 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8709768 - Flags: review?(bugmail.mozilla) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Randall Barker [:rbarker] from comment #16)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
Attachment #8709765 - Flags: review?(botond) → review+
Address review comments.
Attachment #8709767 - Attachment is obsolete: true
Attachment #8710131 - Flags: review?(bugmail.mozilla)
Carry forward r+ from :botond
Attachment #8709765 - Attachment is obsolete: true
Attachment #8709765 - Flags: review?(tnikkel)
Carry forward r+ from :kats
Attachment #8709766 - Attachment is obsolete: true
Carry forward r+ from :kats
Attachment #8709768 - Attachment is obsolete: true
Attachment #8710131 - Flags: review?(bugmail.mozilla) → review+
Blocks: 1241332
Blocks: 1251001
You need to log in before you can comment on or make changes to this bug.