Last Comment Bug 786477 - collapsing chatboxes on overflow is wonkey
: collapsing chatboxes on overflow is wonkey
Status: RESOLVED FIXED
[Fx17][qa?]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 18
Assigned To: Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 15:35 PDT by Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
Modified: 2012-09-24 12:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
overflow width.patch (855 bytes, patch)
2012-08-28 16:19 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
overflow width.patch (1.57 KB, patch)
2012-08-29 16:30 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
felipc: review+
Details | Diff | Review
overflow width.patch (1.58 KB, patch)
2012-08-31 11:00 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-28 15:35:10 PDT
after css changes in bug 785593 and bug 784272, collapsing chatboxes during overflow doesn't always work right.
Comment 1 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-28 16:19:36 PDT
Created attachment 656248 [details] [diff] [review]
overflow width.patch
Comment 2 :Felipe Gomes (needinfo me!) 2012-08-28 18:36:22 PDT
is it possible to find an alternative to not have to use this hardcoded value? otherwise every style change we make will end up breaking this again
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-08-29 15:34:55 PDT
Shane, can you use the XUL overflow event? https://developer.mozilla.org/en-US/docs/XUL/Events
Comment 4 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-29 15:43:58 PDT
(In reply to Jared Wein [:jaws] from comment #3)
> Shane, can you use the XUL overflow event?
> https://developer.mozilla.org/en-US/docs/XUL/Events

we do use overflow.  what doesn't work is using underflow to decide whether to show one of the collapsed chats.  underflow only happens the first underflow, we need to continue checking
Comment 5 :Felipe Gomes (needinfo me!) 2012-08-29 15:51:04 PDT
An idea: when we collapse a chat due to overflow, we could store its size as an attribute and then use it here.
Comment 6 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-29 15:57:24 PDT
(In reply to :Felipe Gomes from comment #5)
> An idea: when we collapse a chat due to overflow, we could store its size as
> an attribute and then use it here.

I was trying to avoid that, but it looks like boxObject.width == 0 when collapsed, so we may have to do that.
Comment 7 :Felipe Gomes (needinfo me!) 2012-08-29 16:02:20 PDT
slightly cleaner would be to assume all chatboxes are the same size and store the size in a field in the chatbar rather than on each chatbox
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-08-29 16:16:42 PDT
How does this patch handle minimized chats? They have a different width (160px + 4px margin start).
Comment 9 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-29 16:30:13 PDT
Created attachment 656661 [details] [diff] [review]
overflow width.patch

revised patch handles dynamic widths, works well with collapsed chats also.
Comment 10 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-30 11:12:27 PDT
Comment on attachment 656661 [details] [diff] [review]
overflow width.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 785593, 784272
User impact if declined: bad ux in socialapi chat boxes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none
Comment 11 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-30 11:12:38 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5a09186a68fc
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-30 17:20:36 PDT
I think you're not supposed to use boxObject.width and use getBoundingClientRect().width instead.
Comment 13 Neil Deakin 2012-08-31 06:51:35 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> I think you're not supposed to use boxObject.width and use
> getBoundingClientRect().width instead.

That's correct. boxObject.x/y/width/height are deprecated. They both should return the same value for width though, although the latter accounts for applied transforms.
Comment 14 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-08-31 11:00:30 PDT
Created attachment 657363 [details] [diff] [review]
overflow width.patch

[Approval Request Comment]
fixes poor overflow handling of chat windows
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-31 11:12:25 PDT
Comment on attachment 657363 [details] [diff] [review]
overflow width.patch

No need to ask for re-review for that kind of tweak.
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-31 12:16:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b449366e9663
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-31 13:16:37 PDT
checkin-needed for aurora, too
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-08-31 17:25:44 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b9a9033ae42
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-08-31 18:43:17 PDT
https://hg.mozilla.org/mozilla-central/rev/b449366e9663
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-18 15:53:19 PDT
Can I please get a better definition of "wonky" so I can verify this fixed? Thanks
Comment 21 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-24 12:52:06 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #20)
> Can I please get a better definition of "wonky" so I can verify this fixed?
> Thanks

IIRC you could reproduce by:

open 3 chat windows then resize the window, smaller then larger.  One of the chat windows will overflow off the edge of the browser window rather than collapsing into the button.

Note You need to log in before you can comment on or make changes to this bug.