Bug 82875 - Pressing Return at the bottom of an enclosed contenteditable div makes scrolling to jump
Summary: Pressing Return at the bottom of an enclosed contenteditable div makes scroll...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks: 86649
  Show dependency treegraph
 
Reported: 2012-04-02 03:12 PDT by Mikhail Naganov
Modified: 2012-05-17 04:22 PDT (History)
4 users (show)

See Also:


Attachments
Repro case (courtesy of carlanton@google.com) (276 bytes, text/html)
2012-04-02 03:12 PDT, Mikhail Naganov
no flags Details
A screenshot of what happens (40.55 KB, image/png)
2012-04-02 03:13 PDT, Mikhail Naganov
no flags Details
Patch (29.30 KB, patch)
2012-04-17 06:21 PDT, Mikhail Naganov
rniwa: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Comments addressed (11.06 KB, patch)
2012-05-09 08:00 PDT, Mikhail Naganov
rniwa: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Added a text for the textarea case (12.32 KB, patch)
2012-05-11 11:25 PDT, Mikhail Naganov
rniwa: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2012-04-02 03:12:40 PDT
Created attachment 135054 [details]
Repro case (courtesy of carlanton@google.com)

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.
Comment 1 Mikhail Naganov 2012-04-02 03:13:15 PDT
Created attachment 135056 [details]
A screenshot of what happens
Comment 2 Mikhail Naganov 2012-04-17 06:21:56 PDT
Created attachment 137526 [details]
Patch
Comment 3 Ryosuke Niwa 2012-04-22 22:36:44 PDT
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.
Comment 4 Ryosuke Niwa 2012-04-22 22:37:34 PDT
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.
Comment 5 Ryosuke Niwa 2012-04-22 22:38:08 PDT
This bug belongs in editing.
Comment 6 Mikhail Naganov 2012-05-09 07:59:07 PDT
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?
Comment 7 Mikhail Naganov 2012-05-09 08:00:55 PDT
Created attachment 140946 [details]
Comments addressed
Comment 8 Ryosuke Niwa 2012-05-09 11:02:44 PDT
(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.
Comment 9 Mikhail Naganov 2012-05-09 12:04:41 PDT
(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>
Comment 10 Ryosuke Niwa 2012-05-09 15:12:22 PDT
Yup, that's what I'm talking about.
Comment 11 Ryosuke Niwa 2012-05-10 00:37:49 PDT
Comment on attachment 140946 [details]
Comments addressed

I'd say r- because of the problem we just agreed upon :)
Comment 12 Mikhail Naganov 2012-05-11 11:25:09 PDT
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.
Comment 13 Ryosuke Niwa 2012-05-11 12:30:51 PDT
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>
Comment 14 Ryosuke Niwa 2012-05-11 12:31:52 PDT
You should't be using isEndOfDocument here. That's not the right condition.
Comment 15 Ryosuke Niwa 2012-05-16 10:12:59 PDT
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.
Comment 16 Ryosuke Niwa 2012-05-16 10:13:50 PDT
Comment on attachment 141456 [details]
Added a text for the textarea case

Ugh... forgot about the discussion that isEndOfDocument was mis-named. r=me.
Comment 17 Mikhail Naganov 2012-05-16 11:15:33 PDT
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.
Comment 18 Ryosuke Niwa 2012-05-16 11:24:18 PDT
Does webkit-patch automatically post commit message on the bug now? If not, we don't normally post commit messages like this on bugzilla.
Comment 19 Mikhail Naganov 2012-05-17 02:27:46 PDT
Thanks, good to know.
Comment 20 Ádám Kallai 2012-05-17 04:18:38 PDT
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.
Comment 21 Mikhail Naganov 2012-05-17 04:22:09 PDT
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.