Closed
Bug 955639
Opened 10 years ago
Closed 10 years ago
Blinking scrollbar whenever timebubbles adds a pixel at the bottom of the conversation
Categories
(Instantbird Graveyard :: Conversation, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: florian, Assigned: florian)
Details
(Whiteboard: [1.5-blocking][regression])
Attachments
(1 file, 2 obsolete files)
3.56 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [1.5-blocking][regression]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [1.5-blocking][regression] → [1.5-blocking][regression][needs patch flo?]
Assignee | ||
Comment 1•10 years ago
|
||
*** 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 | ||
Updated•10 years ago
|
Assignee: nobody → florian
Assignee | ||
Comment 2•10 years ago
|
||
*** 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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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 5•10 years ago
|
||
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-
Comment 6•10 years ago
|
||
*** 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
Assignee | ||
Comment 7•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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() ?
Assignee | ||
Comment 9•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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(...)
Assignee | ||
Comment 11•10 years ago
|
||
*** 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.
Assignee | ||
Comment 12•10 years ago
|
||
*** 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)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Updated•10 years ago
|
Whiteboard: [1.5-blocking][regression][needs patch flo?] → [1.5-blocking][regression][checkin-needed]
Comment 15•10 years ago
|
||
*** Original post on bio 2194 at 2013-12-14 18:28:38 UTC *** http://hg.instantbird.org/instantbird/rev/478518d04c0a
Status: NEW → RESOLVED
Closed: 10 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.
Description
•