Closed Bug 1577058 Opened 5 years ago Closed 5 years ago

PageUp/PageDown jumps to top or bottom of very tall contenteditable div.

Categories

(Core :: DOM: Selection, defect, P2)

Product:
Core
Component:
DOM: Selection
Version:
68 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: robert.maxton42, Assigned: masayuki)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Bug 1577058 - part 1: `nsFrameSelection::CommonPageMove()` should use page scroll amount if there is a scrollable element rather than height of click target r=smaug!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1577058 - part 3: Make test_scroll_per_page.html compute editing host height which needs to be too tall than the viewport with actual documentElement height r=smaug!
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Entering a sufficiently long volume of text into a very 'tall' (height larger than the actual screen), unscrollable contenteditable div produces a page that cannot be easily scrolled with the pageup/pagedown buttons.

For example, fill the div in the following data url with lorem ipsum, leave the cursor captured by the div, and attempt to scroll one page with the 'pageup/pagedown' buttons: data:text/html, <html><body><div contenteditable='true' style='border:2px solid black; width: 300; height: 2080; overflow-y: hidden; '>Your text here</div></body></html></html>

This may be related to a bug previously thought to be due to the XenForo forum software, where long posts in a scrollable div (but loaded down with scripts and live functionality) exhibit the same behavior.

Actual results:

The text box scrolls all the way to the very top of the div, or the very bottom.

Expected results:

One page-height of text, minus one line, should have been scrolled.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Editor
Product: Firefox → Core
Priority: -- → P3

Confirmed this is affecting Wikipedia's visual editor. Can be tested in our standalone demo:
https://en.wikipedia.org/w/extensions/VisualEditor/lib/ve/demos/ve/desktop-wikimediaui.html#!long

I bisected through releases, and this appears to be a regression in FF65.

(In reply to ejsanders from comment #2)

Confirmed this is affecting Wikipedia's visual editor. Can be tested in our standalone demo:
https://en.wikipedia.org/w/extensions/VisualEditor/lib/ve/demos/ve/desktop-wikimediaui.html#!long

I bisected through releases, and this appears to be a regression in FF65.

Thanks. This seems to be regression. I am looking for regression range.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Priority: P3 → P2

This seems to be regression by bug 1498816. Could you look this?

Flags: needinfo?(masayuki)

Hmm, looks like that ScrollSelectionIntoView() scroll it back to caret position if caret is not in the view when PageDown or PageUp is pressed.

I don't understand why we need to call it from PresShell::PageMove(). For sub scrollable frames??

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Editor → Selection
Flags: needinfo?(masayuki)
OS: Unspecified → All
Hardware: Unspecified → All

I'm still trying to understand what's going on...

When pressing PageDown or PageUp, Selection is also modified in our editor. So, I'm currently thinking that ScrollSelectionIntoView() is called for making sure of showing caret or focus node in the view. On the other hand, this does not work when first PageDown key press with this case:
<div id="editor" contenteditable style="height: 300px; overflow: auto;"><div style="height: 1500px;">abc<br>def<br></div></div>
When you put caret to the first line and press PageDown, caret is moved to the last line. However, the editing host scrolls one page. (Then, second key press meets same symptom as reported by this bug.) It seems that the reason is racing issue between smooth scroll of nsGfxScrollFrame and ScrollSelectionIntoView. Even if I unset nsISelectionController::SCROLL_SYNCHRONOUS when calling ScrollSelectionIntoView(), it seems that it runs before scrolling. I think that the cause must be using EarlyRunner starting from bug 1375491.

Currently, I'm thinking that we should use ScrollSelectionIntoView() only when selection is moved. However, with the long standing symptom, we cannot make consistency with that. And also I don't think that PageUp key should call ScrollSelectionIntoView() after PageDown handled twice or more since one PageUp key press scrolls up to the top in this case. This is not consistent behavior with PageDown.

I guess that we should not move caret with PageDown and PageUp if selection is not in the view. But I'm not sure how to know that.

Downstream issue at Wikimedia for reference: https://phabricator.wikimedia.org/T231988

Masayuki, is there any progress on this issue? This is affecting wikipedia which is one of the biggest sites in the world so I'd like to make sure this is not falling into the cracks. Thanks

Flags: needinfo?(masayuki)

Investigate Chrome's behavior. I think that it's reasonable to follow it. That is, when caret is moved by PageDown or PageUp, allow to jump scroll position to show caret. Otherwise, just scrolls a scrollable element. But I'm still struggle with how to do that in our code (just for clean implementation, not I don't understand around it). So, wait a couple of days...

Flags: needinfo?(masayuki)

Currently, nsFrameSelection::CommonPageMove() is called before every caller
calls nsFrameSelection::ScrollSelectionIntoView(). However, when an editing
host has focus, the scroll target may be outside of it. In such case, without
moving caret, user may want only to scroll the scrollable element.

Chrome behaves like so. Chrome also can scroll outside scrollable element
of focused editing host. However, it scrolls caret into view only when
caret is moved actually. Therefore, it makes sense to follow this behavior.

This patch makes nsFrameSelection::CommonPageMove() also call
nsFrameSelection::ScrollSelectionIntoView(). However, it newly takes
SelectionIntoView flag for making callers can choose the condition. I.e.,
ScrollSelectionIntoView() should be called always, or only when selection
is actually changed, or shouldn't be called.

Oh, I realized that I tried to fix different bug here sorry for the spam.

Attachment #9102894 - Attachment description: Bug 1577058 - Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too → Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too

nsFrameSelection::CommonPageMove() emulates click in selection limiter
when scrollable frame is outside of focused editing host. However, the
clicked position should be considered with scrollable element's page
scroll amount rather than height of editing host since the height may be
much taller than the scrollable frame.

Currently, nsFrameSelection::CommonPageMove() is called before every caller
calls nsFrameSelection::ScrollSelectionIntoView(). However, when an editing
host has focus, the scroll target may be outside of it. In such case, without
moving caret, user may want only to scroll the scrollable element.

Chrome behaves like so. Chrome also can scroll outside scrollable element
of focused editing host. However, it scrolls caret into view only when
caret is moved actually. Therefore, it makes sense to follow this behavior.

This patch makes nsFrameSelection::CommonPageMove() also call
nsFrameSelection::ScrollSelectionIntoView(). However, it newly takes
SelectionIntoView flag for making callers can choose the condition. I.e.,
ScrollSelectionIntoView() should be called always, or only when selection
is actually changed, or shouldn't be called.

Depends on D50177

Attachment #9102894 - Attachment is obsolete: true
Attachment #9103482 - Attachment description: Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug! → Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d00a7e091efd
part 1: `nsFrameSelection::CommonPageMove()` should use page scroll amount if there is a scrollable element rather than height of click target r=smaug
https://hg.mozilla.org/integration/autoland/rev/c556c5228132
part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug

https://hg.mozilla.org/mozilla-central/rev/d00a7e091efd
https://hg.mozilla.org/mozilla-central/rev/c556c5228132

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Ah, android... I think that we should disable the test on Android temporarily until we get the failure reason if I couldn't get it today.

Flags: needinfo?(masayuki)
Attachment #9103482 - Attachment description: Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug → Bug 1577058 - part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug!

On Android, window size depends on the screen size even though the window is
created with its size being specified. The last test of
test_scroll_per_page.html needs to test of scroll position of
documentElement. Previously, it was specified as a fixed value computed from
the specified window size, but for Anrdoid, it should be computed with the
window height at runtime.

Depends on D50178

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/33638cab50d2
part 1: `nsFrameSelection::CommonPageMove()` should use page scroll amount if there is a scrollable element rather than height of click target r=smaug
https://hg.mozilla.org/integration/autoland/rev/91a3d8c654ae
part 2: Make `nsFrameSelection::CommonPageMove()` handle `nsFrameSelection::ScrollSelectionIntoView()` too r=smaug
https://hg.mozilla.org/integration/autoland/rev/a7a46c060535
part 3: Make test_scroll_per_page.html compute editing host height which needs to be too tall than the viewport with actual documentElement height r=smaug

https://hg.mozilla.org/mozilla-central/rev/33638cab50d2
https://hg.mozilla.org/mozilla-central/rev/91a3d8c654ae
https://hg.mozilla.org/mozilla-central/rev/a7a46c060535

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

We are past early betas and we ship this bug since 65, let's let it ride the 72 train.

status-firefox71: affected → wontfix
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: