Summary: | Pressing Return at the bottom of an enclosed contenteditable div makes scrolling to jump | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Naganov <mnaganov> | ||||||||||||
Component: | HTML Editing | Assignee: | Mikhail Naganov <mnaganov> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, haraken, kadam, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 86649 | ||||||||||||||
Attachments: |
|
Description
Mikhail Naganov
2012-04-02 03:12:40 PDT
Created attachment 135056 [details]
A screenshot of what happens
Created attachment 137526 [details]
Patch
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 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 "Ctrl+Return" 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. This bug belongs in editing. 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? Created attachment 140946 [details]
Comments addressed
(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. (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> Yup, that's what I'm talking about. Comment on attachment 140946 [details]
Comments addressed
I'd say r- because of the problem we just agreed upon :)
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 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>
You should't be using isEndOfDocument here. That's not the right condition. 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 on attachment 141456 [details]
Added a text for the textarea case
Ugh... forgot about the discussion that isEndOfDocument was mis-named. r=me.
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. Does webkit-patch automatically post commit message on the bug now? If not, we don't normally post commit messages like this on bugzilla. Thanks, good to know. 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. 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. |