942749 - [keyboard] forms.js may scroll caret position of textareas/contenteditables larger than the display area off the screen due to naive use of scrollIntoView on keyboard-triggered resize

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: form-scroll-snafu-v1.diff

Bug 837251 tries to make the e-mail app compose UI less horrible by making the textarea grow so that it is always large enough to hold all displayed text and no internal scrolling is required.  In the case that you are replying to an existing e-mail, the widget starts out larger than the display area.  Let us say you then click on the textarea to position the cursor at the top of the textarea.

What happens is the keyboard will animate into place, then the textarea will be scrolled so that its bottom is level with the bottom of the display area.  Unfortunately, this puts the top of the textarea and the caret way off the screen.  This is due to forms.js using scrollIntoView(false) when it sees a resize event, attempting to correct for whatever side-effects eating half of the screen real estate may have.  This a reasonable thing to do for single-line input controls or other smaller widgets, but is not appropriate for larger textareas or contenteditable elements.

The innards of the platform have an explicit mechanism for scrolling a selection into view, although it's not explicitly exposed to content space.  Chrome XPConnect code should be able to poke at most things, although some interfaces/methods are simply not scriptable.

I took a first pass at triggering the scrolling logic by re-establishing the ranges which explicitly trigger the scrolling as a side-effect.  I also cleaned up some related bugs/ambiguities in forms.js on the attached patch.  However, it does not appear to work and I am having difficulty investigating further because my trunk b2g-desktop builds crash at startup during GC.  I performed my testing by modifying the b2g downloaded using "make b2g" in Gaia and replacing the forms.js in toolkit.jar with my modified one.

I'm attaching it and feedback?-ing Fabrice since he did some of the more recent reviews of forms.js and he might be able to point me at others better suited to follow up on this.

I will attach my trivial HTML page that demonstrates the problem (browse to it with the Gaia browser) as the second patch.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: demonstration of textarea and contenteditable bugs

HTML file with a very large textarea and very large contenteditable.  Click near the top or middle on FxOS from the browser and see the scrolling not do the right thing.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8337624 [details] [diff] [review]
form-scroll-snafu-v1.diff

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

I haven't taken a look at the platform code, but could it be that your remove-ranges then add-ranges-back could be seen as a single "no change" operation if we don't go back to the event loop in between?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> I haven't taken a look at the platform code, but could it be that your
> remove-ranges then add-ranges-back could be seen as a single "no change"
> operation if we don't go back to the event loop in between?

In the platform code the only place I saw batching was some C++ RAII auto-transaction stuff for content-editable.  There definitely didn't seem to be any attempt to detect the status quo and fast-path out.  (There were optimizations to avoid doing silly things, like subtracting a range that is not part of the range, but that doesn't apply.)

Thanks for the quick feedback; just wanted to make sure there was nothing obvious.  I'll try and take another look at this while the native stuff is still somewhat fresh in my mind.  I'm hoping whatever is causing the GC death resolves itself in my next build.  If not I'll try an older gcc and/or just use the symbol server to fetch symbols for the download b2g.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: Scroll-content-editable-elements.patch

Hey Andrew - 

Here's a patch that seems to handle the contenteditable case for me. Can you take a quick look, and let me know if this fixes this for you as well?

With this patch and the blocked bug's patch, the compose area seems to function quite well for me!

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8345177 [details] [diff] [review]
Scroll-content-editable-elements.patch

Fabrice - not sure if you are the right person to review this. Any chance you could take a look or re-assign the review? Thanks!

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8345177 [details] [diff] [review]
Scroll-content-editable-elements.patch

This works nicely for me on the test HTML file I attached for contenteditable.  I feel very silly for not directly investigating the chrome options rather than chasing down the pure content path!  I think I may have been overly hung up on the noble WebAPI goal that content should be able to trigger equivalent behaviours too.

It looks like getPlaintextEditor should work for both plaintext and contenteditable fields.  Perhaps you can restructure the method to be more like
:

let editor = getPlaintextEditor(element);
if (editor) {
  // fancy scroll here!
} else {
  // lame scroll:
  element.scrollIntoView();
}

This is the type of thing that we really wouldn't want to regress, so it probably wants mochitest coverage.  (Maybe there's an existing test that just needs to be revised?)

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8345177 [details] [diff] [review]
Scroll-content-editable-elements.patch

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

r=me with nit addressed.

::: dom/inputmethod/forms.js
@@ +1024,5 @@
> + * Calls scrollSelectionIntoView for contentEditable elements.
> + */
> +function scrollSelectionOrElementIntoView(element) {
> +  if (isContentEditable(element)) {
> +    let editor = getPlaintextEditor(element);

that can return null, please bail out if so.

Comment 8

[Kevin Grandon :kgrandon]

Attachment: Bug-942749-keyboard-formsjs-contenteditable-scroll.patch

Comment 9

[Kevin Grandon :kgrandon]

Try: https://tbpl.mozilla.org/?tree=Try&rev=1f99d9517275

Comment 10

[Kevin Grandon :kgrandon]

Adding checkin-needed for: https://bugzilla.mozilla.org/attachment.cgi?id=8347335

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa26de1458fd

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa26de1458fd