Bug 82875 – Pressing Return at the bottom of an enclosed contenteditable div makes scrolling to jump

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Mikhail Naganov]

Created attachment 135054 [details]
Repro case (courtesy of [e-mail])

In the example provided, a contenteditable div is enclosed into a bigger size div. If the enclosing div doesn't fit into the view, typing Return when the caret has reached the bottom of the inner div scrolls the latter to put the caret into the center of the view.

If the window is tall enough to fit the outer div, then hitting Return at the bottom line doesn't jump-scroll. It only happens if window height is less than outer div height. And even in this case, once you inflate the inner div to fill the height of the outer div by inserting enough lines of text, jump-scrolling ceases to happen.

[Mikhail Naganov]

Created attachment 135056 [details]
A screenshot of what happens

[Mikhail Naganov]

Created attachment 137526 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 137526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137526&action=review

> LayoutTests/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Source/WebCore/editing/Editor.cpp:948
> +    VisiblePosition caret(m_frame->selection()->selection().visibleStart());

We don't normally prefer operator= over copy constructor (it's just a syntax sugar anyways). i.e.
VisiblePosition caret = m_frame->selection()->selection().visibleStart().

> Source/WebCore/editing/Editor.cpp:969
> +    VisiblePosition caret(m_frame->selection()->selection().visibleStart());
> +    bool alignToEdge = isEndOfDocument(caret);

This isn't right. We probably need to do the same whenever we're at the end of the current scrollable area.
Also see https://bugs.webkit.org/show_bug.cgi?id=64143.

r- because this isn't the right condition.

[Ryosuke Niwa]

Comment on attachment 137526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137526&action=review

> LayoutTests/editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable.html:12
> +<div>When the caret is scrolled out and resides at the end of the contenteditable, 
> +on pressing &quot;Ctrl+Return&quot; it must be scrolled to the bottom of the view,
> +not to the center to avoid undesirable content view jumping.</div>
> +<div style="height:1000px;">
> +  <div style="overflow:visible; height:100px;" contenteditable="true" id="input-contenteditable"></div>
> +</div>
> +<script>

There should be a way of testing this using dumpAsText test.

[Ryosuke Niwa]

This bug belongs in editing.

[Mikhail Naganov]

Comment on attachment 137526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137526&action=review

>> LayoutTests/ChangeLog:11
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the long description.

Done.

>> LayoutTests/editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable.html:12
>> +<script>
> 
> There should be a way of testing this using dumpAsText test.

Right, switched to dumpAsText.

>> Source/WebCore/ChangeLog:11
>> +        Reviewed by NOBODY (OOPS!).
> 
> Ditto.

Done.

>> Source/WebCore/editing/Editor.cpp:948
>> +    VisiblePosition caret(m_frame->selection()->selection().visibleStart());
> 
> We don't normally prefer operator= over copy constructor (it's just a syntax sugar anyways). i.e.
> VisiblePosition caret = m_frame->selection()->selection().visibleStart().

Done.

>> Source/WebCore/editing/Editor.cpp:969
>> +    bool alignToEdge = isEndOfDocument(caret);
> 
> This isn't right. We probably need to do the same whenever we're at the end of the current scrollable area.
> Also see https://bugs.webkit.org/show_bug.cgi?id=64143.
> 
> r- because this isn't the right condition.

The name 'isEndOfDocument' may be misleading. Actually it simply checks, whether the position is at the end of a nodes subtree, it doesn't need to be a document.

Or, perhaps, I don't fully understand your comment. Can you please elaborate on this?

[Mikhail Naganov]

Created attachment 140946 [details]
Comments addressed

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 137526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137526&action=review
> >> Source/WebCore/editing/Editor.cpp:969
> >> +    bool alignToEdge = isEndOfDocument(caret);
> > 
> > This isn't right. We probably need to do the same whenever we're at the end of the current scrollable area.
> > Also see https://bugs.webkit.org/show_bug.cgi?id=64143.
> > 
> > r- because this isn't the right condition.
> 
> The name 'isEndOfDocument' may be misleading. Actually it simply checks, whether the position is at the end of a nodes subtree, it doesn't need to be a document.

Right, and that won't work for input elements inside an element with overflow:auto; and width and height specified. We shouldn't be scrolling beyond the end of the document in such case.

Imagine the case of nested iframes, and then replace those iframes with elements with overflow:auto.

[Mikhail Naganov]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 137526 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137526&action=review
> > >> Source/WebCore/editing/Editor.cpp:969
> > >> +    bool alignToEdge = isEndOfDocument(caret);
> > > 
> > > This isn't right. We probably need to do the same whenever we're at the end of the current scrollable area.
> > > Also see https://bugs.webkit.org/show_bug.cgi?id=64143.
> > > 
> > > r- because this isn't the right condition.
> > 
> > The name 'isEndOfDocument' may be misleading. Actually it simply checks, whether the position is at the end of a nodes subtree, it doesn't need to be a document.
> 
> Right, and that won't work for input elements inside an element with overflow:auto; and width and height specified. We shouldn't be scrolling beyond the end of the document in such case.
> 
> Imagine the case of nested iframes, and then replace those iframes with elements with overflow:auto.

I think, I've got what you are talking about. If I'm trying the following snippet in a short window, I'm also observing a "jump scroll" after the caret goes out of view and I'm pressing Return once more.

<html>
  <body>
    <div style="overflow:auto;height:1000px;">
      <textarea rows="10" cols="20"></textarea>
    </div>
  </body>
</html>

[Ryosuke Niwa]

Yup, that's what I'm talking about.

[Ryosuke Niwa]

Comment on attachment 140946 [details]
Comments addressed

I'd say r- because of the problem we just agreed upon :)

[Mikhail Naganov]

Created attachment 141456 [details]
Added a text for the textarea case

I apologize, it seems that I was trying my textarea example on an unpatched WebKit, because with the patch, I observe no jumpscroll.

When the caret resides at the end of a textarea, this is still recognized as 'isEndOfDocument'. I've added a new test case for a textarea. Please take a look, if that's the situation we were previously talking about.

[Ryosuke Niwa]

Comment on attachment 141456 [details]
Added a text for the textarea case

No, I'm talking about the case with contenteditable elements. e.g. <div contenteditable></div>

[Ryosuke Niwa]

You should't be using isEndOfDocument here. That's not the right condition.

[Ryosuke Niwa]

Comment on attachment 141456 [details]
Added a text for the textarea case

View in context: https://bugs.webkit.org/attachment.cgi?id=141456&action=review

> Source/WebCore/editing/Editor.cpp:948
> +    bool alignToEdge = isEndOfDocument(caret);

Calling isEndOfDocument here is wrong.

[Ryosuke Niwa]

Comment on attachment 141456 [details]
Added a text for the textarea case

Ugh... forgot about the discussion that isEndOfDocument was mis-named. r=me.

[Mikhail Naganov]

Committed: http://trac.webkit.org/changeset/117307


Avoid jumpscroll when entering new text in a multi-line editor.
https://bugs.webkit.org/show_bug.cgi?id=82875

Reviewed by Ryosuke Niwa

Scroll caret to the edge of the viewport in case if a line break or a paragraph
separator is inserted at the end of a multi-line editor.  This avoids
undesirable jumpscroll in cases when there is content under the editor.

Tests: editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable.html
       editing/input/scroll-to-edge-if-line-break-at-end-of-document-textarea.html
       editing/input/scroll-to-edge-if-paragraph-separator-at-end-of-document-contenteditable.html

* editing/Editor.cpp:
(WebCore::Editor::insertLineBreak):
(WebCore::Editor::insertParagraphSeparator):
(WebCore::Editor::revealSelectionAfterEditingOperation):
* editing/Editor.h:
(Editor):

* editing/input/resources/reveal-utilities.js:
(performJumpAtTheEdgeTest):
* editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable-expected.txt: Added.
* editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable.html: Added.
* editing/input/scroll-to-edge-if-line-break-at-end-of-document-textarea-expected.txt: Added.
* editing/input/scroll-to-edge-if-line-break-at-end-of-document-textarea.html: Added.
* editing/input/scroll-to-edge-if-paragraph-separator-at-end-of-document-contenteditable-expected.txt: Added.
* editing/input/scroll-to-edge-if-paragraph-separator-at-end-of-document-contenteditable.html: Added.

[Ryosuke Niwa]

Does webkit-patch automatically post commit message on the bug now? If not, we don't normally post commit messages like this on bugzilla.

[Mikhail Naganov]

Thanks, good to know.

[Ádám Kallai]

This test fails on QT[Wk1] bots. (qt-4.8, qt-5.0-wk1)

Diff:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r117423%20(37481)/editing/input/scroll-to-edge-if-line-break-at-end-of-document-contenteditable-diff.txt

Could you check it please? Do you think this result is correct?

Thanks.

[Mikhail Naganov]

The test displays 'PASS', so it seems to pass.

I'd blame the Qt's implementation of DRT for not dumping empty lines from a contenteditable div.