Page is unexpectedly scrolled to the bottom

VERIFIED FIXED in Firefox 55

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: alice0775, Assigned: masayuki)

Tracking

({regression})

55 Branch
mozilla56
regression
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56 fixed)

Details

(Whiteboard: [parity-Chrome][parity-Edge], URL)

Attachments

(4 attachments)

(Reporter)

Description

a year ago
[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 )
Flags: needinfo?(masayuki)
(Reporter)

Updated

a year ago
Has Regression Range: --- → yes
Has STR: --- → yes
(Reporter)

Comment 1

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

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

Comment 2

a year ago
Created attachment 8882880 [details]
possible testcase html
(Reporter)

Updated

a year ago
Whiteboard: [parity-Chrome][parity-Edge]
> Page is automatically scrolled to textarea at the bottom of page

Is it really a <textarea>? Or an element whose contenteditable is true?
Flags: needinfo?(masayuki) → needinfo?(alice0775)
Ah, you attached really helpful testcase.
Flags: needinfo?(alice0775)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Priority: -- → P1
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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
Attachment #8883624 - Flags: review?(bugs) → review+

Comment 10

a year ago
mozreview-review
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
Attachment #8883625 - Flags: review?(bugs) → review+

Comment 11

a year ago
Pushed by masayuki@d-toybox.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9955dd87b169
https://hg.mozilla.org/mozilla-central/rev/617800a7300f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Hi Masayuki, should we consider uplifting this to Beta55?
tracking-firefox55: ? → +
Flags: needinfo?(masayuki)
Hi Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build (7-7 build)? Thanks!
Flags: needinfo?(alice0775)
(Reporter)

Comment 15

a year ago
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(alice0775)
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.
Flags: needinfo?(masayuki)
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.
Attachment #8883624 - Flags: approval-mozilla-beta?
Created attachment 8884204 [details] [diff] [review]
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.
Attachment #8884204 - Flags: review+
Attachment #8884204 - Flags: approval-mozilla-beta?
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
Attachment #8883624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8884204 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.