Closed Bug 492760 Opened 11 years ago Closed 11 years ago

element.scrollIntoView sometimes scrolls at the top of the document

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: florian, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file testcase
I think this appeared after the landing of bug 67752 but I haven't verified the exact regression date. Bug 492575 may also be related.
I first noticed it in Instantbird nightlies when new content is added in already large conversations.

Steps to reproduce:
 1. Load the attached testcase in a recent trunk build
 2. Move the mouse over the document.

Expected result:
 Moving the mouse does nothing visible to the document.

Actual result:
 While moving the mouse, the top of the document is displayed, instead of the last inserted lines. On my machine the bug starts to be visible with about 300 lines of text. Starting at 1000 or 2000 lines, as long as I move the mouse only the top of the document is visible and the document doesn't move any more.
What that page does is repeatedly blow away all content on the page and replace it with all-new content.  This content then has to all be reflowed to look correct.  It used to be that scrollIntoView forced an immediate layout; now it still does a reflow, but an interruptible one.  If you're moving the mouse around, we'll interrupt it (see bug 492145 for some more discussion of this).  Since the interrupt happens near the beginning of the document, typically, there is nothing to scroll to (the frame for the content you're trying to scroll to hasn't been laid out yet).  If the page didn't keep blowing the content away, we'd eventually get to that line, lay it out, and scroll it into view...

Is that testcase based on an actual page, by any chance?  The idea with interruptible reflow is that certain kinds of rendering glitches are acceptable if the user is trying to interact with the page: the interaction takes priority.  This might just be such a situation.
Depends on: 492145
(In reply to comment #2)
> What that page does is repeatedly blow away all content on the page and replace
> it with all-new content.
I'm not sure I understand what you mean here. The content already in the page is not touched by DOM manipulations, there's only a call to appendChild to add new elements to the document.

> Is that testcase based on an actual page, by any chance?
The testcase is derived from the code I use in Instantbird to append new IMs to a conversation.
I haven't found a way to reliably reproduce the exact bug we have (which doesn't involve mouse events, both hands are on the keyboard when I send/receive IMs).
It happens only with already large conversations (at least 200 or 300 messages I think) and not always. When it happens, the call to scrollIntoView scrolls the document to the top and very quickly after scrolls it to the right position.

I read in a bug or code comment that scrollIntoView first does a "best-effort" scroll and then scrolls to the correct position when it is known. Would it be possible to disable the first best-effort scroll if it's going to scroll to a position which is very unlikely to be right (like in this case the top of the document)?

For now I added a call to getBoundingClientRect in my code like you did in a test in bug 492575 and it seems to make the issue go away.
> there's only a call to appendChild to add new elements to the document.

Oh, indeed.  I misread the innerHTML set...

So given that, the issue is more likely that we'll interrupt after lines we didn't actually reflow.  It might be worth changing that, perhaps.

> The testcase is derived from the code I use in Instantbird 

OK, makes sense.  The mouse vs key event thing is as expected; interrupts can happen on all sorts of user events.

> When it happens, the call to scrollIntoView scrolls the document to the top
> and very quickly after scrolls it to the right position.

Yeah, that would make sense given how interrupts work.

> Would it be possible to disable the first best-effort scroll if it's going to
> scroll to a position which is very unlikely to be right

_That_ might be a good idea.  I'll do that in this bug.  Should be trivial to detect cases when we're scrolling to a frame that's never been reflowed (and hence has completely bogus coordinates).
Attached patch Like so, perhapsSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #377446 - Flags: superreview?(roc)
Attachment #377446 - Flags: review?(roc)
Attachment #377446 - Flags: superreview?(roc)
Attachment #377446 - Flags: superreview+
Attachment #377446 - Flags: review?(roc)
Attachment #377446 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/5f4ee25d609c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Blocks: Instantbird
You need to log in before you can comment on or make changes to this bug.