955639 - Blinking scrollbar whenever timebubbles adds a pixel at the bottom of the conversation

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[Florian Quèze [:florian]]

*** Original post on bio 2194 at 2013-10-02 10:04:00 UTC ***

This is a moz24 regression, because scrollbars are now visible only while scrolling.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

*** Original post on bio 2194 as attmnt 3112 at 2013-12-04 07:02:00 UTC ***

I don't understand why this seemed so complicated last time I looked; maybe I was just tired. Or maybe I am today and I missed something :).

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

*** Original post on bio 2194 as attmnt 3113 at 2013-12-04 07:23:00 UTC ***

Alright, now I remember what made this complicated: preventing margin changes during updates of the text; but still doing the margin changes at the same time as the text update when switching between tabs.

The previous patch looked much better :-(.

Comment 3

[Florian Quèze [:florian]]

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

*** Original change on bio 2194 attmnt 3112 at 2013-12-04 07:23:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 4

[aleth [:aleth]]

Comment on attachment 8354897 [details] [diff] [review]
Patch v2

*** Original change on bio 2194 attmnt 3113 at 2013-12-04 10:42:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[aleth [:aleth]]

Comment on attachment 8354897 [details] [diff] [review]
Patch v2

*** Original change on bio 2194 attmnt 3113 at 2013-12-04 18:49:43 UTC ***

I am a little confused by the logic of this change. Every time a message is added, the scrollbar will blink as well. Isn't that distracting too? i.e. isn't there a mozilla bug here that should be filed?

>+function handleLastMessage(aUpdateTextOnly)
Please add a comment above this explaining what this function and in particular its parameter does. It's not obvious ;) It might help to call updateTextOnly allowNoSizeChanges or something like that, which may match better what its purpose is.

It's disappointing how unreadable this code is getting, but I don't see a good way to restructure it without duplication :(

This setTimeout call no longer makes sense after these changes I think:
http://lxr.instantbird.org/instantbird/source/instantbird/themes/messages/bubbles/Footer.html#157

Comment 6

[aleth [:aleth]]

*** Original post on bio 2194 at 2013-12-04 19:14:44 UTC ***

Possibly somewhat related https://bugzilla.mozilla.org/show_bug.cgi?id=869519, https://bugzilla.mozilla.org/show_bug.cgi?id=888027

Comment 7

[Florian Quèze [:florian]]

*** Original post on bio 2194 at 2013-12-04 23:17:06 UTC ***

(In reply to comment #3)

> Every time a message is
> added, the scrollbar will blink as well. Isn't that distracting too?

It's not more distracting than seeing the bubble appear. The problem here was really that the scrollbar blinks when "nothing" happens, because we are just tweaking the margins.

> >+function handleLastMessage(aUpdateTextOnly)
> Please add a comment above this explaining what this function and in particular
> its parameter does.

Is it the only reason for r-?

> This setTimeout call no longer makes sense after these changes I think:
> http://lxr.instantbird.org/instantbird/source/instantbird/themes/messages/bubbles/Footer.html#157

Why not? It will start a timer of 5 minutes, to make the text appear onces the last message is old enough.

Comment 8

[aleth [:aleth]]

*** Original post on bio 2194 at 2013-12-05 10:26:18 UTC ***

(In reply to comment #5)
> It's not more distracting than seeing the bubble appear. The problem here was
> really that the scrollbar blinks when "nothing" happens, because we are just
> tweaking the margins.

OK. It still sounds like distracting behaviour, I am surprised that decision was taken by mozille/apple/whoever.

> > >+function handleLastMessage(aUpdateTextOnly)
> > Please add a comment above this explaining what this function and in particular
> > its parameter does.
> 
> Is it the only reason for r-?

Yes, otherwise it looks fine. (If the code is hard to read we should add comments.)
 
> > This setTimeout call no longer makes sense after these changes I think:
> > http://lxr.instantbird.org/instantbird/source/instantbird/themes/messages/bubbles/Footer.html#157
> 
> Why not? It will start a timer of 5 minutes, to make the text appear onces the
> last message is old enough.

Why not replace setTimeout(handleMessage, 0) with handleMessage() ?

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 2194 at 2013-12-13 18:10:10 UTC ***

(In reply to comment #6)

> > Is it the only reason for r-?
> 
> Yes, otherwise it looks fine. (If the code is hard to read we should add
> comments.)

So is it r+ with a comment added? (I'm wondering if I can land this tonight, to have all the blockers fixed in the next nightly.)

> Why not replace setTimeout(handleMessage, 0) with handleMessage() ?

We use a setTimeout to ensure handleLastMessage is called only once when adding a batch of messages. It's used as an executeSoon here.

Comment 10

[aleth [:aleth]]

*** Original post on bio 2194 at 2013-12-13 18:13:42 UTC ***

(In reply to comment #7)
> So is it r+ with a comment added? (I'm wondering if I can land this tonight, to
> have all the blockers fixed in the next nightly.)

Yes, but...

> > Why not replace setTimeout(handleMessage, 0) with handleMessage() ?
> 
> We use a setTimeout to ensure handleLastMessage is called only once when adding
> a batch of messages. It's used as an executeSoon here.

...I think for it to work so it is actually only called once it should be
lastMessageTimeout = setTimeout(...)

Comment 11

[Florian Quèze [:florian]]

*** Original post on bio 2194 at 2013-12-13 21:49:06 UTC ***

(In reply to comment #8)
> (In reply to comment #7)

> > We use a setTimeout to ensure handleLastMessage is called only once when adding
> > a batch of messages. It's used as an executeSoon here.
> 
> ...I think for it to work so it is actually only called once it should be
> lastMessageTimeout = setTimeout(...)

The check is on lastMessageTimeoutTime, not lastMessageTimeout. So I think it works.

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v3

*** Original post on bio 2194 as attmnt 3123 at 2013-12-14 17:50:00 UTC ***

Added comment, as requested.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8354897 [details] [diff] [review]
Patch v2

*** Original change on bio 2194 attmnt 3113 at 2013-12-14 17:50:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[aleth [:aleth]]

Comment on attachment 8354908 [details] [diff] [review]
Patch v3

*** Original change on bio 2194 attmnt 3123 at 2013-12-14 17:55:52 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Patrick Cloke [:clokep]]

*** Original post on bio 2194 at 2013-12-14 18:28:38 UTC ***

http://hg.instantbird.org/instantbird/rev/478518d04c0a