1377752 - Page is unexpectedly scrolled to the bottom

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Regression Web page layout

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/587daa4bdc4b40b7053f4ca3b74723ca747f3b52
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0 ID:20170630150342

Reproducible : always

Steps to reproduce:
1. Open http://www.insomnia.gr/topic/638548-amd-zen-am4-socket-part-2/page-448
2. Log in
   --- observe scroll position
3. Click "2" or "3" pagination link
   --- observe scroll position

Actual Results:
Page is automatically scrolled to textarea at the bottom of page

Expected Results:
After step2: Page should not be scrolled 
After step3: Page should be at top(initial)


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5d0ce2805e5b0fe5f941c804caf2675e0ed73a5f&tochange=22e1a32de5b88f1f56a156aefa95614b5cda204c

Regressed by: Bug 1318312 


And the other web pages well also be affected. (reported http://forums.mozillazine.org/viewtopic.php?p=14741915#p14741915 )

Comment 1

[Alice0775 White]

Sorry, the link of STR is wrong. Correct link is as follows:

2. Open https://www.minhembio.com/forum/index.php?showtopic=331542

Comment 2

[Alice0775 White]

Attachment: possible testcase html

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Page is automatically scrolled to textarea at the bottom of page

Is it really a <textarea>? Or an element whose contenteditable is true?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, you attached really helpful testcase.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Chrome doesn't scroll <iframe> when I set selection into a contenteditable element in an <iframe> which doesn't have focus.

So, I think that we can use nsIFocusManager::FLAG_NOSCROLL when the document doesn't have focus.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9794e184973edc9cf3f5065306ce77c24c4194f

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1377752 - part1: Add automated test for checking scroll position and focused document when setting selection into a contenteditable element in an iframe element

When an iframe doesn't have focus, moving selection into a contenteditable element in it shouldn't cause moving focus to the document, moving focus to the editable element nor scrolling to the editor but the editor should become active element.

When an iframe has focus, moving selection into a contenteditable element in it should cause moving focus to the editor and scroll it into the view.

Review commit: https://reviewboard.mozilla.org/r/154486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154486/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1377752 - part2: Selection::NotifySelectionListeners() should make nsFocusManager not scroll new focused element into the view if it's not focused document

When changing selection into a contenteditable element in non-focused document, new focused editor shouldn't be scrolled into the view for compatibility with the other browsers.

Review commit: https://reviewboard.mozilla.org/r/154488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154488/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8883624 [details]
Bug 1377752 - part1: Add automated test for checking scroll position and focused document when setting selection into a contenteditable element in an iframe element

https://reviewboard.mozilla.org/r/154486/#review159646

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8883625 [details]
Bug 1377752 - part2: Selection::NotifySelectionListeners() should make nsFocusManager not scroll new focused element into the view if it's not focused document

https://reviewboard.mozilla.org/r/154488/#review159710

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9955dd87b169
part1: Add automated test for checking scroll position and focused document when setting selection into a contenteditable element in an iframe element r=smaug
https://hg.mozilla.org/integration/autoland/rev/617800a7300f
part2: Selection::NotifySelectionListeners() should make nsFocusManager not scroll new focused element into the view if it's not focused document r=smaug

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9955dd87b169
https://hg.mozilla.org/mozilla-central/rev/617800a7300f

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Masayuki, should we consider uplifting this to Beta55?

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build (7-7 build)? Thanks!

Comment 15

[Alice0775 White]

I confirm this bug has been fixed in the latest Nightly56.0a1.

https://hg.mozilla.org/mozilla-central/rev/4bd7db49d22847111dff9c1dd63ed573903faa5b
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0 ID:20170706060058

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, I'd like to. I was thinking that I should request to uplift them after tested for a couple of days. However, according to the number of testers in each channel, uplifting as soon as possible might be safer.

I'll check if they are graftable cleanly.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8883624 [details]
Bug 1377752 - part1: Add automated test for checking scroll position and focused document when setting selection into a contenteditable element in an iframe element

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1318312.

[User impact if declined]:
Really not useful in some pages like some comments in this bug report. Automatically scrolled to the bottom of a page when it's loaded unexpectedly.

[Is this code covered by automated tests?]:
Yes (although, this patch includes only the test).

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This adds only automated test.

[String changes made/needed]:
No.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part 2 for Beta (since the file is renamed from layout/generic/nsSelection.cpp to dom/base/Selection.cpp)

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1318312.

[User impact if declined]:
Really not useful in some pages like some comments in this bug report. Automatically scrolled to the bottom of a page when it's loaded unexpectedly.

[Is this code covered by automated tests?]:
Yes (the previous patch).

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
Not so risky.

[Why is the change risky/not risky?]:
Complicated case is tested by the automated tests and when simple case like loading following data URI to a tab directly doesn't cause unexpected affect to current behavior.
> data:text/html,<div style="height: 5000px;"></div><div id="editor" contenteditable>editor</div><script>window.onload = ()=>{ var range = document.createRange(); range.setStart(document.getElementById("editor"), 0); var selection = document.getSelection(); selection.removeAllRanges(); selection.addRange(range); };</script>

(The behavior is different from Chrome, but current behavior is better consistency with our <textarea> or <input>.)

[String changes made/needed]:
No.

Comment 19

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8883624 [details]
Bug 1377752 - part1: Add automated test for checking scroll position and focused document when setting selection into a contenteditable element in an iframe element

regression fix for beta55

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8fd07c6b8c0e
https://hg.mozilla.org/releases/mozilla-beta/rev/e39f3951ae1c

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> [Is this code covered by automated tests?]:
> Yes (the previous patch).
> 
> [Has the fix been verified in Nightly?]:
> Yes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No.

Setting qe-verify- based on Masayuki Nakano's assessment on manual testing needs and the fact that this fix has automated coverage.