Closed Bug 953706 Opened 11 years ago Closed 8 years ago

Scrolling new messages into view doesn't work for messages with large images

Categories

(Instantbird Graveyard :: Conversation, defect)

Product:
Instantbird Graveyard
Component:
Conversation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Instantbird 47

People

(Reporter: florian, Assigned: freaktechnik)

References

Details

(Whiteboard: [wanted])

Attachments

(1 file, 4 obsolete files)

bug953706-fix-v5.patch
7.20 KB, patch
aleth
: review+
Details | Diff | Splinter Review 
*** Original post on bio 261 at 2009-10-27 10:38:00 UTC ***

The scrollIntoView call for new messages inserted into a conversation happens right after the message is inserted, but the emoticon images are asynchronously so they appear after that. If an emoticon is taller than the line height of the message, then the height of the message increases when the emoticon appears, and the scrollbar is no longer at the bottom; a part of the new message may not be visible.

A possible way to fix that would be to add an onload listener that would be fired when the emoticon has finished loading. When this event listener would be fired, scroll again to ensure the scrollbar stays at the right position.
Whiteboard: [0.2-wanted]
Whiteboard: [0.2-wanted] → [wanted]
Attached patch bug953706-fix-v1.patch (obsolete) — — Splinter Review 
This patch scrolls to the last inserted message whenever an image inside the conversation browser is loaded. It worked fine in slower chat rooms, but in very busy chat rooms the auto scroll still eventually breaks.

I'm not sure how a test for this patch would look - mostly because I haven't encountered this problem with vanilla Instantbird.
Attachment #8720723 - Flags: review?(florian)
Comment on attachment 8720723 [details] [diff] [review]
bug953706-fix-v1.patch

Review of attachment 8720723 [details] [diff] [review]:
-----------------------------------------------------------------

Do you have any idea of why this isn't fully solving the problem for busy channels?

::: chat/content/convbrowser.xml
@@ +781,5 @@
> +      <method name="contentElementLoad">
> +        <parameter name="event"/>
> +        <body>
> +          <![CDATA[
> +            if (event.target.tagName.toLowerCase() === "img" &&

I think we usually use .localName rather than .tagName.toLowerCase().
Also, we use == rather than ===, unless there's a specific reason for using ===.

In this case, I wonder if the test shouldn't rather be:
 event.target instanceof Ci.nsIImageLoadingContent

@@ +917,5 @@
>                if (this.progressBar)
>                  this.progressBar.hidden = true;
>  
> +              this.contentDocument.getElementById("Chat")
> +                .addEventListener("load", this.contentElementLoad.bind(this), true);

nit: align the '.' at the beginning of the second line with the first . on the first line.
Attachment #8720723 - Flags: review?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Do you have any idea of why this isn't fully solving the problem for busy
> channels?

My blind assumption would be that _lastElement doesn't point to the last element and then autoScrollEnabled() determines that auto scroll has been disabled by the user when a message gets added. Maybe checking for _scrollingIntoView in the autoScrollEnabled method would fix that, I'll try that tonight.

> ::: chat/content/convbrowser.xml
> @@ +781,5 @@
> > +      <method name="contentElementLoad">
> > +        <parameter name="event"/>
> > +        <body>
> > +          <![CDATA[
> > +            if (event.target.tagName.toLowerCase() === "img" &&
> 
> I think we usually use .localName rather than .tagName.toLowerCase().
> Also, we use == rather than ===, unless there's a specific reason for using
> ===.
> 
> In this case, I wonder if the test shouldn't rather be:
>  event.target instanceof Ci.nsIImageLoadingContent

I just assumed XPCOM would be sandboxed away for actual browser content.

I'll update my patch as soon as I've tested if it's autoScrollEnabled() breaking auto scrolling in busy rooms.
Attached patch bug953706-fix-v2.patch (obsolete) — — Splinter Review 
This should address the nits. I've looked into the failures in bigger rooms and it turns out they are not related to image loading. Auto scrolling gets disabled in-between messages without an image being loaded, however I didn't have time yet to analyze it in detail.
Attachment #8720723 - Attachment is obsolete: true
Attachment #8721057 - Flags: review?(florian)
Attachment #8721057 - Flags: review?(florian) → review+
I've looked at the scrolling issues in "faster" conversations in detail, and found a code path that will still cause this issue due to emoticons: If a message gets added with auto scrolling disabled by a function parameter and that message contains an emoticon that loads before a newer message is scrolled into view, it will cause the scroll event listener to disable auto scroll.

The only solution I've come up with so far would involve reverting that decision when the image is loaded, however this could feel like a little jitter if the user is currently scrolling manually.
Assignee: nobody → martin
Comment on attachment 8721057 [details] [diff] [review]
bug953706-fix-v2.patch

Review of attachment 8721057 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/content/convbrowser.xml
@@ +782,5 @@
> +        <parameter name="event"/>
> +        <body>
> +          <![CDATA[
> +            if (event.target.localName == "img" &&
> +                this._autoScrollEnabled && this._lastElement) {

Is there a potential race here with an _updateAutoScrollEnabled call that may reset _autoScrollEnabled?

(Why are you not using this.autoScrollEnabled? A comment would be helpful for the future.)

@@ +917,5 @@
>                if (this.progressBar)
>                  this.progressBar.hidden = true;
>  
> +              this.contentDocument.getElementById("Chat")
> +                  .addEventListener("load", this.contentElementLoad.bind(this), true);

Does this survive after a tab is moved to another window? (if not, modify swapDocShells)
(In reply to Martin Giger [:freaktechnik] from comment #5)
> I've looked at the scrolling issues in "faster" conversations in detail, and
> found a code path that will still cause this issue due to emoticons: If a
> message gets added with auto scrolling disabled by a function parameter and
> that message contains an emoticon that loads before a newer message is
> scrolled into view, it will cause the scroll event listener to disable auto
> scroll.
> 
> The only solution I've come up with so far would involve reverting that
> decision when the image is loaded, however this could feel like a little
> jitter if the user is currently scrolling manually.

How about having your listener check whether messages are still pending to be displayed and if that is the case, do nothing? Does that work?
(In reply to aleth [:aleth] from comment #6)
> ::: chat/content/convbrowser.xml
> @@ +782,5 @@
> > +        <parameter name="event"/>
> > +        <body>
> > +          <![CDATA[
> > +            if (event.target.localName == "img" &&
> > +                this._autoScrollEnabled && this._lastElement) {
> 
> Is there a potential race here with an _updateAutoScrollEnabled call that
> may reset _autoScrollEnabled?
I don't know the specifics, but if a scroll event that happened since the load wasn't process until this point, it might scroll, even though the user just scrolled somewhere else. Not sure if there is a better way to handle it, since updating the auto scroll setting within this handler is dangerous and might disable it, so render the whole code useless.

I'm not using autoScrollEnabled() because similar code (resize handler) did not use it either. All that would change, is that auto scroll would potentially get enabled if it wasn't before, which is not something that should ever happen when an image is loaded.

> 
> (Why are you not using this.autoScrollEnabled? A comment would be helpful
> for the future.)
> 
> @@ +917,5 @@
> >                if (this.progressBar)
> >                  this.progressBar.hidden = true;
> >  
> > +              this.contentDocument.getElementById("Chat")
> > +                  .addEventListener("load", this.contentElementLoad.bind(this), true);
> 
> Does this survive after a tab is moved to another window? (if not, modify
> swapDocShells)

I haven't tried that yet and I don't know well enough how <browser>s work to just know that. I'll look into it and report back, thanks for the tip!

(In reply to aleth [:aleth] from comment #7)
> (In reply to Martin Giger [:freaktechnik] from comment #5)
> > The only solution I've come up with so far would involve reverting that
> > decision when the image is loaded, however this could feel like a little
> > jitter if the user is currently scrolling manually.
> 
> How about having your listener check whether messages are still pending to
> be displayed and if that is the case, do nothing? Does that work?

No, that will not work. Because the auto srcolling is disabled without this patch when images in such messages are loaded in the scroll listener. So the code in my patch would not even be ran, since _autoScrollEnabled is false.
Okay, I've tested moving a conversation into a different window and turns out that the listener wasn't attached there. Should the listener in the old context be cleaned up, or does it not matter?

Checking for _messageDisplayPending in browserScroll and immediately returning fixes the issue with non-scrolling messages breaking auto scroll. However this will not work if something else decides to set aNoAutoScroll. One possible solution would be to store the value of aNoAutoScroll in a property, however I'm not sure how to make it so manual scrolling will still break auto scroll, which is currently impossible with my workaround with _messageDisplayPending. I guess it could work similar to _scrollingIntoView, which is reset once it immediately returned in the scroll listener. However there could be multiple images per message, so multiple potential scroll events per message.
(In reply to Martin Giger [:freaktechnik] from comment #9)
> Okay, I've tested moving a conversation into a different window and turns
> out that the listener wasn't attached there. Should the listener in the old
> context be cleaned up, or does it not matter?

It shouldn't matter, as the listener is on a DOM element in the content. I'm actually surprised the listener gets removed.

> Checking for _messageDisplayPending in browserScroll and immediately
> returning fixes the issue with non-scrolling messages breaking auto scroll.
> However this will not work if something else decides to set aNoAutoScroll.

What "something else" are you thinking of? That parameter is set by the pending message queue when messages are still pending, to avoid nonstop scrolling while messages are being added.

> however I'm not sure how to make it so manual scrolling will still
> break auto scroll

The important thing is that after all messages have been added, if the user scrolls to an earlier position, further message adds don't trigger a scroll to the end.
(In reply to aleth [:aleth] from comment #10)
> What "something else" are you thinking of? That parameter is set by the
> pending message queue when messages are still pending, to avoid nonstop
> scrolling while messages are being added.

Since the method uses a parameter and not the value of _messageDisplayPending directly for example extensions could also set the parameter for their own purposes. Or if someone would expand code later on, it would be non-trivial that there would be code that depends on aNoAutoScroll being equal to _messageDisplayPending.

> The important thing is that after all messages have been added, if the user
> scrolls to an earlier position, further message adds don't trigger a scroll
> to the end.

So ignoring scroll events while queued messages are being appended isn't a bad thing then. But there's still the possibility of the method parameter being used for something else in the future.
(In reply to Martin Giger [:freaktechnik] from comment #11)
> (In reply to aleth [:aleth] from comment #10)
> > What "something else" are you thinking of? That parameter is set by the
> > pending message queue when messages are still pending, to avoid nonstop
> > scrolling while messages are being added.
> 
> Since the method uses a parameter and not the value of
> _messageDisplayPending directly for example extensions could also set the
> parameter for their own purposes. Or if someone would expand code later on,
> it would be non-trivial that there would be code that depends on
> aNoAutoScroll being equal to _messageDisplayPending.

Extensions and non-convbrowser code should never call displayMessage, they should use appendMessage. Generally this code could do with more comments I suppose ;) Feel free to add some.
(In reply to aleth [:aleth] from comment #12)
> Extensions and non-convbrowser code should never call displayMessage, they
> should use appendMessage.
This assumes that appendMessage satisfies all the needs external code would have, which is a fallacy, in my opinion. If you expose a method that has an explicit parameter, then the whole component should behave correctly when the parameter is set.

> Generally this code could do with more comments I
> suppose ;) Feel free to add some.
Having a comment like "This parameter is assumed to have had the same value as _messageDisplayPending" doesn't exactly seem like good design.

My proposed solution would be to remember aNoAutoScroll in some property (_messageDidNotAutoScroll or something like that) and if it is set, skip the scroll event listener. This is currently functionally equivalent to _messageDisplayPending but doesn't depend on aNoAutoScroll always having the value of _messageDisplayPending.
This will - as previously explained - break the user aborting ato scroll while messages are appeneded (or if aNoAutoScroll is set for any other reason) until it's back to false again.
(In reply to Martin Giger [:freaktechnik] from comment #13)
> (In reply to aleth [:aleth] from comment #12)
> > Extensions and non-convbrowser code should never call displayMessage, they
> > should use appendMessage.
> This assumes that appendMessage satisfies all the needs external code would
> have, which is a fallacy, in my opinion. If you expose a method that has an
> explicit parameter, then the whole component should behave correctly when
> the parameter is set.

There's no real API for these bindings. The only reason displayMessage isn't _displayMessage is historical - the queue around it to avoid jank was added later, and since then the method to call is appendMessage. If an extension calls displayMessage, it could lead to messages inserted in the wrong order.
Attached patch bug953706-fix-v3.patch (obsolete) — — Splinter Review 
This fixes the listener getting lost when detaching a conversation from its current window and the scrolling getting disabled when message batches are appended and an image loads during that process.
Attachment #8721057 - Attachment is obsolete: true
Attachment #8723199 - Flags: review?(florian)
Comment on attachment 8723199 [details] [diff] [review]
bug953706-fix-v3.patch

Review of attachment 8723199 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. I haven't read the whole exchange in comments between aleth and you, I assume you've updated the patch until he was satisfied :).

::: chat/content/convbrowser.xml
@@ +797,5 @@
> +          <![CDATA[
> +            if (event.target.localName == "img" &&
> +                this._autoScrollEnabled && !this._messageDisplayPending &&
> +                this._lastElement) {
> +              // An image loadedm auto-scroll is enabled and no messages are

typo: "loadedm"
Attachment #8723199 - Flags: review?(florian) → review+
Comment on attachment 8723199 [details] [diff] [review]
bug953706-fix-v3.patch

Review of attachment 8723199 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/content/convbrowser.xml
@@ +313,5 @@
>              }
>  
>              this.uninitMagicCopy();
> +
> +            this.contentDocument.getElementById("Chat")

contentDocument.getElementbyId("Chat") seems to be used often enough now that it is worth adding a getter property for it to reduce duplication.

@@ +789,5 @@
>            ]]>
>          </body>
>        </method>
>  
> +      <field name="contentElementLoad">null</field>

Please add a comment explaining what the field is for.

I'd suggest calling the method and field onContentElementLoad to make it clear it's an event handler.

Explain in the comment when the event is fired (what elements being added trigger it?)

@@ +798,5 @@
> +            if (event.target.localName == "img" &&
> +                this._autoScrollEnabled && !this._messageDisplayPending &&
> +                this._lastElement) {
> +              // An image loadedm auto-scroll is enabled and no messages are
> +              // currently being appened. Scroll the last element fully back into view.

Typo (appended).
Attached patch bug953706-fix-v4.patch (obsolete) — — Splinter Review 
Addressed the nits from aleth. Also requesting review from aleth, since flo didn't have any nits on the last version.
Attachment #8723199 - Attachment is obsolete: true
Attachment #8723332 - Flags: review?(aleth)
Missed a typo, sorry.
Attachment #8723332 - Attachment is obsolete: true
Attachment #8723332 - Flags: review?(aleth)
Attachment #8723333 - Flags: review?(aleth)
Comment on attachment 8723333 [details] [diff] [review]
bug953706-fix-v5.patch

Review of attachment 8723333 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!
Attachment #8723333 - Flags: review?(aleth) → review+
Severity: minor → normal
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Scrolling new messages into view doesn't work for messages with big emoticons → Scrolling new messages into view doesn't work for messages with large images
Target Milestone: --- → Instantbird 47
Depends on: 1251959
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: