954962 - Autoscroll broken after moz13 update when restoring from hold and using Bubbles

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[aleth [:aleth]]

*** Original post on bio 1530 at 2012-06-20 15:45:00 UTC ***

STR
Take a conversation long enough to be displayed in chunks on reopening.
Use the mouse wheel to scroll up a bit, and then to the end of the conversation.
Put on hold and reopen.
The scroll position is not restored properly and we end up scrolled to an intermediate point ein the conversation.

Suspected reason: Scroll position is restored before all the messages have been added (and/or is somehow absolute and not relative).

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1530 as attmnt 1647 at 2012-06-20 18:43:00 UTC ***

Calling _updateAutoScrollEnabled after adding the new element could lead to autoScroll being turned off due to the new element, probably because of speed improvements in gecko. There is no need to call _updateAutoScrollEnabled here anyway, as adding a new message shouldn't change the autoScroll state, so this patch removes it.

I had a look at the scroll event handler (browserScroll) and both if clauses that prevent _updateAutoScrollEnabled are triggered, the first when you would expect, the second very rarely when resizing. Most of the time, but not always, turning off the if clauses has no effect as the insertion of new elements is fast enough. Probably better to keep that code as-is though?

Comment 2

[aleth [:aleth]]

Comment on attachment 8353404 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1647 at 2012-06-20 21:05:34 UTC ***

Actually does not fix the original STR when there are /many/ messages to be displayed. Will investigate.

Comment 3

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1530 as attmnt 1649 at 2012-06-20 23:54:00 UTC ***

Two further bugfixes:
- A regression caused by me in
http://hg.instantbird.org/instantbird/rev/2eeb8a68d5a3
http://hg.instantbird.org/instantbird/rev/3b67cfcfc0ad
- The remaining issue on opening a conversation from hold: Two scroll events occurring while the messages were being displayed would cause autoScrollEnabled to be unset, as scrolling to the last added message is suppressed during that period.
And some improved comments.

Comment 4

[aleth [:aleth]]

Comment on attachment 8353404 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1647 at 2012-06-20 23:54:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[aleth [:aleth]]

Comment on attachment 8353406 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1649 at 2012-06-20 23:58:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1530 as attmnt 1650 at 2012-06-21 00:01:00 UTC ***

Scratch the first listed bug in the previous comment (issue was caused by an intermediate patch).

Comment 7

[aleth [:aleth]]

Comment on attachment 8353406 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1649 at 2012-06-21 00:01:43 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[aleth [:aleth]]

*** Original post on bio 1530 at 2012-06-21 00:03:31 UTC ***

This patch assumes that no browserScroll events can happen *during* the displayMessage loop in displayPendingMessages. (Otherwise the added code would have to be after each displayMessage call).

Comment 9

[aleth [:aleth]]

Attachment: Alternative patch

*** Original post on bio 1530 as attmnt 1759 at 2012-07-27 13:10:00 UTC ***

Flo reported another aspect of this bug that is not fixed by the existing patch (which only resolves the bug on restoring from hold):
"I've been talking with Even for a while, and at some point I scroll in the scrollback to find a quote or read again a message. Then I scroll to the bottom of the conversation. And I discover that Even has actually sent a few messages only when I send a message myself and don't see it, because I need to scroll to see it and re-enable the autoscroll."

While I can't reproduce this, I've attached an alternative version of my patch which _may_ help, for testing. It changes the calculation in updateAutoScrollEnabled from the body element to window properties. While this is technically less ideal, it has the advantage that it will not trigger a synchronous relayout of the whole page to get the values. WFM, but I don't know if it addresses the issue.

Comment 10

[aleth [:aleth]]

Attachment: Patch for 1.2

*** Original post on bio 1530 as attmnt 1766 at 2012-08-01 16:36:00 UTC ***

This patch only fixes the bug when restoring from hold, while not touching the "just to be sure" updateAutoScrollEnabled call.

STR for other occurrences of this bug are unknown.

Comment 11

[aleth [:aleth]]

Comment on attachment 8353527 [details] [diff] [review]
Patch for 1.2

*** Original change on bio 1530 attmnt 1766 at 2012-08-02 09:27:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

Comment on attachment 8353520 [details] [diff] [review]
Alternative patch

*** Original change on bio 1530 attmnt 1759 at 2012-08-02 09:28:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[aleth [:aleth]]

Comment on attachment 8353407 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1650 at 2012-08-02 09:28:06 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[Florian Quèze [:florian]]

*** Original post on bio 1530 at 2012-08-02 10:27:23 UTC ***

We don't have a good understanding of what's going on here, so it can't block 1.2. However we know this is really annoying, so we still really want to fix it, but it may be after 1.2 ships.

Comment 15

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1530 as attmnt 1775 at 2012-08-02 19:43:00 UTC ***

This is a fix for the problem on restoring from hold, which turns out to be due to a race condition when using Bubbles. I could not reproduce with other message styles when ensuring there were no user-triggered scroll events. STR are simply to restore a conversation from hold which is long enough to trigger a progress bar, but not so long that the scroll position hardly changes when messages are added. As it's a race condition this will not "work" for everyone.

Without Bubbles, this is a typical sequence on restoring from hold:
message block displayed
message block displayed
**show progress bar after this block
message block displayed   <-- The progress bar will be shown after
                              this step, triggering a scroll event...
**show progress bar after this block
message block displayed
browserScroll scrollingintoview   <-- ...which is handled here.
message block displayed
...
Since this._scrollingIntoView was set, this does not lead to updateAutoScrollEnabled being called and possibly turning off autoScrollEnabled.
After browserScroll handles the event, this._scrollingIntoView==false, but there is only one scroll event.

With Bubbles, there is also the scrollIntoView triggered by the timeout on adding messages if enough time elapses between messages. When adding messages from hold this is usually not the case. However, displaying the progress bar takes enough time so that a scroll may occur, at least on some machines:
message block displayed
message block displayed
**show progress bar after this block
message block displayed
--scrollIntoView Bubbles
**show progress bar after this block
message block displayed
browserScroll scrollingintoview
message block displayed
...
In this case there is also no problem as there is still only one scroll event fired. But when the timing is just right this can happen:
message block displayed
message block displayed
**show progress bar after this block
message block displayed
browserScroll scrollingintoview #ubuntu
--scrollintoview Bubbles        <-- Here this scroll happens AFTER the first
                                    scroll event has been handled, triggering
                                    another scroll event...
**show progress bar after this block
message block displayed
browserScroll update      <-- which is handled here and causes the bug.
**show progress bar after this block
message block displayed

The fix in the patch is for Bubbles to call convbrowser._scrollToElement rather than scrollIntoView. This resets this._scrollIntoView=true (just as when autoscrolling to the last message after inserting it), thus preventing the bug.

I hope I found the best place to add the scrollToElement method to the browser window. (The convbrowser init method is called too early.)

Comment 16

[aleth [:aleth]]

Comment on attachment 8353407 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1650 at 2012-08-02 19:43:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[aleth [:aleth]]

Comment on attachment 8353520 [details] [diff] [review]
Alternative patch

*** Original change on bio 1530 attmnt 1759 at 2012-08-02 19:43:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[aleth [:aleth]]

Comment on attachment 8353527 [details] [diff] [review]
Patch for 1.2

*** Original change on bio 1530 attmnt 1766 at 2012-08-02 19:43:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8353536 [details] [diff] [review]
Patch

*** Original change on bio 1530 attmnt 1775 at 2012-08-03 13:17:17 UTC ***

Looks good. We rephrased the comment a bit over IRC.

Comment 20

[Patrick Cloke [:clokep]]

*** Original post on bio 1530 at 2012-08-03 18:10:47 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/fe0c0fb1c1af