Closed Bug 955639 Opened 7 years ago Closed 7 years ago

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

Categories

(Instantbird :: Conversation, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

Details

(Whiteboard: [1.5-blocking][regression])

Attachments

(1 file, 2 obsolete files)

*** 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.
Whiteboard: [1.5-blocking][regression]
Whiteboard: [1.5-blocking][regression] → [1.5-blocking][regression][needs patch flo?]
Attached patch Patch (obsolete) — Splinter Review
*** 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 :).
Attachment #8354896 - Flags: review?(aleth)
Assignee: nobody → florian
Attached patch Patch v2 (obsolete) — Splinter Review
*** 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 :-(.
Attachment #8354897 - Flags: review?(aleth)
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 ***
Attachment #8354896 - Attachment is obsolete: true
Attachment #8354896 - Flags: review?(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 ***
Attachment #8354897 - Attachment is patch: true
Attachment #8354897 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #8354897 - Flags: review?(aleth) → review-
*** 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
*** 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.
*** 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() ?
*** 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.
*** 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(...)
*** 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.
Attached patch Patch v3Splinter Review
*** Original post on bio 2194 as attmnt 3123 at 2013-12-14 17:50:00 UTC ***

Added comment, as requested.
Attachment #8354908 - Flags: review?(aleth)
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 ***
Attachment #8354897 - Attachment is obsolete: true
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 ***
Attachment #8354908 - Flags: review?(aleth) → review+
Whiteboard: [1.5-blocking][regression][needs patch flo?] → [1.5-blocking][regression][checkin-needed]
*** Original post on bio 2194 at 2013-12-14 18:28:38 UTC ***

http://hg.instantbird.org/instantbird/rev/478518d04c0a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [1.5-blocking][regression][checkin-needed] → [1.5-blocking][regression]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.