If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

chatbar overflow does not handle more than one child

VERIFIED FIXED in Firefox 18

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: markh)

Tracking

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18+ verified, firefox19 verified)

Details

Attachments

(1 attachment, 10 obsolete attachments)

33.48 KB, patch
jaws
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
If you go full screen, and open enough chat windows to overflow (thus creating the overflow menubutton), then you reduce the screen size enough that more than one chat would overflow, you will see a cut off chat window on the right.

fix coming soon.
(Reporter)

Comment 1

5 years ago
Created attachment 668214 [details] [diff] [review]
overflow width.patch
Attachment #668214 - Flags: review?(jaws)
(Reporter)

Updated

5 years ago
tracking-firefox17: --- → ?
Attachment #668214 - Flags: review?(felipc)

Updated

5 years ago
tracking-firefox17: ? → +
Comment on attachment 668214 [details] [diff] [review]
overflow width.patch

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

Should you attempt to skip the selected chat and only hide it if not even 1 can be left open?
(Reporter)

Comment 3

5 years ago
(In reply to :Felipe Gomes from comment #2)

> Should you attempt to skip the selected chat and only hide it if not even 1
> can be left open?

That is what happens in this.firstRemovableChild.  

however, this made me look at the patch again, and I see a problem in the patch I'll have to fix.
(Reporter)

Comment 4

5 years ago
Created attachment 668838 [details] [diff] [review]
overflow width.patch

v2 ensures we break out of the while loop if no removable children exist
Attachment #668214 - Attachment is obsolete: true
Attachment #668214 - Flags: review?(jaws)
Attachment #668214 - Flags: review?(felipc)
Attachment #668838 - Flags: review?(jaws)
Attachment #668838 - Flags: review?(felipc)
(Reporter)

Comment 5

5 years ago
Created attachment 668839 [details] [diff] [review]
overflow width.patch

oops, last one was an interdiff
Attachment #668838 - Attachment is obsolete: true
Attachment #668838 - Flags: review?(jaws)
Attachment #668838 - Flags: review?(felipc)
Attachment #668839 - Flags: review?(jaws)
Attachment #668839 - Flags: review?(felipc)
(Reporter)

Comment 6

5 years ago
Comment on attachment 668839 [details] [diff] [review]
overflow width.patch

Markh is around, moving review to him
Attachment #668839 - Flags: review?(mhammond)
Attachment #668839 - Flags: review?(jaws)
Attachment #668839 - Flags: review?(felipc)
Attachment #668839 - Flags: review?
(Reporter)

Updated

5 years ago
Attachment #668839 - Flags: review?
(Assignee)

Comment 7

5 years ago
Created attachment 669007 [details] [diff] [review]
takes margin into account and handles children already being collapsed

A new version, collaborated on by me and Shane over IRC.  r+ from me, but I'm hoping Jaws can have a quick look too
Attachment #668839 - Attachment is obsolete: true
Attachment #668839 - Flags: review?(mhammond)
Attachment #669007 - Flags: review?(jaws)
(Assignee)

Comment 8

5 years ago
oops - collaborated on IRC*
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 9

5 years ago
Comment on attachment 669007 [details] [diff] [review]
takes margin into account and handles children already being collapsed

Pushing my luck here, but as Gavin is soon to see waking hours, flagging him for review too.
Attachment #669007 - Flags: review?(gavin.sharp)
Comment on attachment 669007 [details] [diff] [review]
takes margin into account and handles children already being collapsed

This code is pretty layout flush-happy. I wonder what impact it has on window resizing performance while chatboxes are open/overflowed...

I'm a bit nervous about the |while (this.emptyWidth < 1)| loop not terminating somehow, but I haven't thought that through very much. The rest of the patch changes makes sense.
Attachment #669007 - Flags: review?(gavin.sharp) → feedback+
Comment on attachment 669007 [details] [diff] [review]
takes margin into account and handles children already being collapsed

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

::: browser/base/content/socialchat.xml
@@ +351,5 @@
> +
> +        // we may need to collapse more than one child, e.g. moving from
> +        // fullscreen to a small window size.  Our spacer will be zero
> +        // size until enough are collapsed
> +        while (this.emptyWidth < 1) {

The emptyWidth property is using boxObject.width, which is deprecated. Can you switch emptyWidth to use getBoundingClientRect?

Also, instead of a while loop that collapses and then recomputes, could we first do a loop to determine how many to collapse and then just collapse those? That would give us less reflows.
Attachment #669007 - Flags: review?(jaws) → feedback+
Assignee: mixedpuppy → mhammond
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Assignee)

Comment 12

5 years ago
Created attachment 671414 [details] [diff] [review]
large refactoring and extensive tests

As I test this I'm finding a number of other edge cases.  I've refactored some of the test code to remove them and have extensive tests for resizing and collapsing.  There are a couple of test failures which I'm confident are test related, but I'm uploading it now for feedback while I sleep :)  Note it also implements that there can never be zero chats visible (bug 801534).  I'll get back onto this tomorrow, but given the changes are significant I thought I'd try and get early feedback for all our sakes :)
Attachment #671414 - Flags: feedback?(mixedpuppy)
Attachment #671414 - Flags: feedback?(jaws)
Attachment #671414 - Flags: feedback?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #671414 - Attachment is patch: true
(Reporter)

Comment 13

5 years ago
Comment on attachment 671414 [details] [diff] [review]
large refactoring and extensive tests

does this still have the reflow issue that was brought up in one of the previous feedback comments?

Other that resolving bug 801534 first, this direction looks ok.
Attachment #671414 - Flags: feedback?(mixedpuppy) → feedback+
(Reporter)

Updated

5 years ago
Depends on: 801534
(Assignee)

Comment 14

5 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> does this still have the reflow issue that was brought up in one of the
> previous feedback comments?

I *think* it addresses it :)  We don't perform a reflow as each chat is removed - instead we calculate how many chats to collapse, then collapse them all in one go.  However, "collapse them all in one go" is still a loop, and each iteration of the loop performs a reflow as each collapse asks for the chats bounding rect.  It might even be possible to avoid that, especially if we assume each chat has an identical width.
(Assignee)

Comment 15

5 years ago
Created attachment 671832 [details] [diff] [review]
Even more revamping

Lots of tests!  Moar refactoring!  Much less reflow (we no longer rely on the width of the spacer to help resize, so can do all resizing in batch.)  Supports allowing all chats to collapse or optionally allowing the last to remain open but truncated (and as a result, can no longer rely on overflow in all cases - so just a single resize handler) - tests don't succeed in that case, but should be trivial to get them going if we take that route.  All in all, it works well :)

Does have some issues regarding the order in which chats are collapsed and expanded, but these issues already exist now - this patch doesn't address them but makes it simpler to do so.
Attachment #669007 - Attachment is obsolete: true
Attachment #671414 - Attachment is obsolete: true
Attachment #671414 - Flags: feedback?(jaws)
Attachment #671414 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 16

5 years ago
Created attachment 672143 [details] [diff] [review]
updated

This version has excellent test coverage and works really well.
Attachment #671832 - Attachment is obsolete: true
Attachment #672143 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #672143 - Flags: feedback? → feedback?(mixedpuppy)
(Assignee)

Comment 17

5 years ago
Created attachment 672583 [details] [diff] [review]
Updated to handle test boxes without necessary screen width

Changes only to the tests.

Try runs show that a couple of the test boxes don't have the necessary screen width to perform all tests, so we detect that situation and skip those tests.  We still insist on a minimum width to get started, so this should not trigger false positives and test skipping if something else goes wrong.
Attachment #672143 - Attachment is obsolete: true
Attachment #672143 - Flags: feedback?(mixedpuppy)
Attachment #672583 - Flags: feedback?(mixedpuppy)
(Reporter)

Comment 18

5 years ago
Comment on attachment 672583 [details] [diff] [review]
Updated to handle test boxes without necessary screen width

I still think we should have min-width when there is a chatbox open.
Attachment #672583 - Flags: feedback?(mixedpuppy) → feedback+
(Assignee)

Comment 19

5 years ago
Comment on attachment 672583 [details] [diff] [review]
Updated to handle test boxes without necessary screen width

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Comment on attachment 672583 [details] [diff] [review]
> Updated to handle test boxes without necessary screen width
> 
> I still think we should have min-width when there is a chatbox open.

Thanks.  Agree we need to so something better, but we can track that in bug 801534
Attachment #672583 - Flags: review?(jaws)
(Assignee)

Comment 20

5 years ago
It would be nice to get this landed (or reworked) before it goes too stale and it prevents chats being hidden and impossible to get to when large resize operations happen.
Comment on attachment 672583 [details] [diff] [review]
Updated to handle test boxes without necessary screen width

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

Sorry for the long delay in reviewing this. This is mostly good, but I'd like to take another look at the next version of this patch.

::: browser/base/content/socialchat.xml
@@ +171,1 @@
>            }

Can this be simplified to:
return !!this.querySelector("[collapsed]");

@@ +205,5 @@
> +              yield child;
> +          /* NOTE: if we wanted to allow the selected one to close too, we'd just...
> +          if (this.selectedChat)
> +            yield this.selectedChat;
> +          */

If we don't allow the selected one to collapse, what happens if the content area gets too narrow to fit a single chat?

@@ +259,5 @@
>          <body><![CDATA[
> +          if (aChatbox.minimized)
> +            aChatbox.minimized = false;
> +          if (this.selectedChat != aChatbox)
> +            this.selectedChat = aChatbox;

The setter of "selectedChat" should have this at the beginning:
> if (this._selectedChat == val)
>   return;
which can simplify these two lines here.

@@ +268,5 @@
> +          // to take room...
> +          this.resize();
> +        ]]></body>
> +      </method>
> +      

nit: whitespace here and in a couple other places.

@@ +361,5 @@
> +        //   reflow regardless of how many we change.
> +        // So we go the more complicated but more efficient second option...
> +        let availWidth = this.getBoundingClientRect().width;
> +        let currentWidth = 0;
> +        if (!this.menupopup.parentNode.collapsed) // the nub is visible.

I think the .parentNode approach is a little too fragile and isn't as easy to read. I think we can replace that with either another member variable, or a temporary getElementById.

@@ +377,5 @@
> +            currentWidth -= this.getTotalWidthOf(child);
> +          }
> +          if (toCollapse.length)
> +            for (let child of toCollapse)
> +              this.collapseChat(child);

style nit: if the block for the if is more than one line, use curly brackets on the if-statement.

@@ +398,5 @@
> +            this.menupopup.parentNode.collapsed = true;
> +          }
> +        } else {
> +          // we are pixel-perfect!
> +        }

I'd just remove the |else { }| part, but there should be some achievement or award for the user in this case :)

::: browser/base/content/test/browser_social_chatwindow.js
@@ +181,5 @@
> +  // check removeAll does the right thing
> +  testRemoveAll: function(next, mode) {
> +    let port = Social.provider.getWorkerPort();
> +    port.postMessage({topic: "test-init"});
> +    get3ChatsForCollapsing(mode || "normal", function(first, second, third) {

|first, second, third| are unused.

@@ +357,5 @@
> +// Does a callback passing |true| if the window is now big enough or false
> +// if we couldn't resize large enough to satisfy the test requirement.
> +function resizeWindowToChatAreaWidth(desired, cb) {
> +  let current = window.SocialChatBar.chatbar.getBoundingClientRect().width;
> +  let delta = desired-current;

nit: space around operators, |desired - current|.

@@ +384,5 @@
> +      info("skipping this as we can't resize chat area to " + desired + " - got " + newSize);
> +    }
> +    cb(sizedOk);
> +  });
> +  window.resizeBy(delta, 0);

Do you need to set dom.disable_window_move_resize=true ? Note that this will only work in popups with a single tab when that pref is enabled. This pref was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=565541#c24.
Attachment #672583 - Flags: review?(jaws) → review-
(Assignee)

Comment 22

5 years ago
> If we don't allow the selected one to collapse, what happens if the content area
> gets too narrow to fit a single chat?

This is the part Shane objects to.  In this patch, the final chat is clipped on the right hand side.  Not ideal (and we do need something better), but IMO having it also vanish is worse as when it is in that state, using the nub or starting a new chat will do nothing at all.  At least with this patch the user might notice a partially shown chat swap to the user they just tried to use, and it should be obvious to them that a resize will give the width back.

> Do you need to set dom.disable_window_move_resize=true ? Note that this will
> only work in popups with a single tab when that pref is enabled.

I'm not sure sure of the context of this - certainly the tests all work and do actually resize the app window while running.
(In reply to Mark Hammond (:markh) from comment #22)
> > If we don't allow the selected one to collapse, what happens if the content area
> > gets too narrow to fit a single chat?
> 
> This is the part Shane objects to.  In this patch, the final chat is clipped
> on the right hand side.  Not ideal (and we do need something better), but
> IMO having it also vanish is worse as when it is in that state, using the
> nub or starting a new chat will do nothing at all.  At least with this patch
> the user might notice a partially shown chat swap to the user they just
> tried to use, and it should be obvious to them that a resize will give the
> width back.

Ok, yeah I think we should always show at least one chatbox. Showing none gets weird when a user tries to switch to one and they see nothing happen.

> > Do you need to set dom.disable_window_move_resize=true ? Note that this will
> > only work in popups with a single tab when that pref is enabled.
> 
> I'm not sure sure of the context of this - certainly the tests all work and
> do actually resize the app window while running.

Ok, then disregard this comment :)
(Assignee)

Comment 24

5 years ago
Created attachment 676034 [details] [diff] [review]
updated patch addressing review comments

> ::: browser/base/content/socialchat.xml
> @@ +171,1 @@
> >            }

> Can this be simplified to:
> return !!this.querySelector("[collapsed]");

Done.

> If we don't allow the selected one to collapse, what happens if the content area gets too narrow to fit a single chat?

As discussed, it is cropped - see comment 23

> @@ +259,5 @@
> >          <body><![CDATA[
> > +          if (aChatbox.minimized)
> > +            aChatbox.minimized = false;
> > +          if (this.selectedChat != aChatbox)
> > +            this.selectedChat = aChatbox;

> The setter of "selectedChat" should have this at the beginning:
> > if (this._selectedChat == val)
> >   return;
> which can simplify these two lines here.

It actually makes it a little more complex seeing we also must take care to remove the 'activity' attribute even when the currently selected chat is already selected.  However, the new version does avoid clearing and setting the 'selected' attribute in this case, which I guess is a bit of a win, so I've done it.

> nit: whitespace here and in a couple other places.

Should all be addressed.

> I think the .parentNode approach is a little too fragile and isn't as easy to read. 
> I think we can replace that with either another member variable, or a temporary 
> getElementById.

this.nub was already in place for this, so done.

> style nit: if the block for the if is more than one line, use curly brackets on the if-statement.

Done.

> I'd just remove the |else { }| part, but there should be some achievement or award for the user in this case :)

I removed the else but left a single line comment.

> |first, second, third| are unused.
> nit: space around operators, |desired - current|.

Both done.

> Do you need to set dom.disable_window_move_resize=true ? Note that this will only work in 
> popups with a single tab when that pref is enabled. This pref was introduced in 
> https://bugzilla.mozilla.org/show_bug.cgi?id=565541#c24.

As discussed in IRC, the tests all work fine without this and the app window is resized during the tests, so it seems to be unnecessary.

New try run at https://tbpl.mozilla.org/?tree=Try&rev=d530331566a6
Attachment #672583 - Attachment is obsolete: true
Attachment #676034 - Flags: review?(jaws)
(Assignee)

Comment 25

5 years ago
Created attachment 676076 [details] [diff] [review]
Tests updated for try failure on linux

wtf is up on Linux with asking for a window size 660px and actually getting 659.5!?  Anyway, try failed on the linux boxes due to this.  This new patch has just some test changes for that.

New try at https://tbpl.mozilla.org/?tree=Try&rev=eff56479dadd - not sure why TBPL isn't showing this as a revision, but the linux logs show it is there.
Attachment #676034 - Attachment is obsolete: true
Attachment #676034 - Flags: review?(jaws)
Attachment #676076 - Flags: review?(jaws)
Whiteboard: [Fx17]
tracking-firefox17: + → ---
tracking-firefox18: --- → +
Comment on attachment 676076 [details] [diff] [review]
Tests updated for try failure on linux

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

basically r+, but I want to see what it would look like if we could cache the widths.

::: browser/base/content/socialchat.xml
@@ +213,5 @@
> +              yield child;
> +          /* NOTE: if we wanted to allow the selected one to close too, we'd just...
> +          if (this.selectedChat)
> +            yield this.selectedChat;
> +          */

I would just remove this commented out code. It may confuse people who don't notice that it is dead. The comment a few lines above already explains that the selectedChat is left out.

@@ +370,5 @@
> +        // So we go the more complicated but more efficient second option...
> +        let availWidth = this.getBoundingClientRect().width;
> +        let currentWidth = 0;
> +        if (!this.nub.collapsed) // the nub is visible.
> +          currentWidth += this.getTotalWidthOf(this.nub);

I would prefer if we could cache the width of the nub, minimized and restored chats. Those three sizes should never change on a given platform + theme, and would reduce the number of reflows that resize may be triggering.
Attachment #676076 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 27

5 years ago
Created attachment 677975 [details] [diff] [review]
review comments, including using cached widths

The cached widths doesn't seem to make the code more horrible :)
Attachment #676076 - Attachment is obsolete: true
Attachment #677975 - Flags: review?(jaws)
Comment on attachment 677975 [details] [diff] [review]
review comments, including using cached widths

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

::: browser/base/content/socialchat.xml
@@ +133,5 @@
> +      <constructor>
> +        // to avoid reflows we cache the values for various widths.
> +        this.cachedWidthOpen = null;
> +        this.cachedWidthMinimized = null;
> +        this.cachedWidthNub = null;

nit: These should be initialized to 0.

@@ +254,5 @@
> +          // have a width of zero as they are not shown).
> +          if (aChatbox.minimized) {
> +            if (!this.cachedWidthMinimized) {
> +              if (aChatbox.collapsed)
> +                throw new Error("can't calculate size of collapsed chat!");

The |if (aChatbox.collapsed) { throw new Error(...) }| is duplicated in this function. I would move it up above the |if (aChatbox.minimized)| line so the duplication can be removed.
Attachment #677975 - Flags: review?(jaws) → review+
(Assignee)

Comment 29

5 years ago
> The |if (aChatbox.collapsed) { throw new Error(...) }| is duplicated in 
> this function. I would move it up above the |if (aChatbox.minimized)| line 
> so the duplication can be removed.

Actually, it needs to go where it is as it is only hit when the cached values are not in place.

IOW, it is fine to call this on a collapsed chat so long as we are using the cached value.  This is why the code that looks useless:

          // we ensure that the cached width for a child of this type is
          // up-to-date so we can use it when resizing.
          this.getTotalChildWidth(aChatbox);

exists - to ensure we have the cached version before we collapse it, so we can never hit that error.

Does that make sense?
(In reply to Mark Hammond (:markh) from comment #29)
> > The |if (aChatbox.collapsed) { throw new Error(...) }| is duplicated in 
> > this function. I would move it up above the |if (aChatbox.minimized)| line 
> > so the duplication can be removed.
> 
> Actually, it needs to go where it is as it is only hit when the cached
> values are not in place.
> 
> Does that make sense?

Makes sense. Disregard my comment about that then.
(Assignee)

Comment 31

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/837ae727d237

Note that bug 807001 means I had to change the patch to the test code treats map.size as a property rather than a method.
https://hg.mozilla.org/mozilla-central/rev/837ae727d237
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19

Updated

5 years ago
Depends on: 809258
(Assignee)

Comment 33

5 years ago
Comment on attachment 677975 [details] [diff] [review]
review comments, including using cached widths

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Social chat windows may become hidden
Testing completed (on m-c, etc.): On m-c since November 6
Risk to taking this patch (and alternatives if risky): Minor risk and contained to the social chat feature.
String or UUID changes made by this patch: None
Attachment #677975 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #677975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/93bd1c5aaca4
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Unfortunately, this had to be backed out on Aurora for mochitest failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac7418002275

https://tbpl.mozilla.org/php/getParsedLog.php?id=17010275&tree=Mozilla-Aurora

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_social_chatwindow.js | chatboxForURL map should be empty - Got function size() {
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_social_chatwindow.js | chatboxForURL map should be empty - Got function size() {
status-firefox18: fixed → affected
Mark, the failure is because in Fx19 Map.size is a getter, but in Fx18 it is still a function. See https://bugzilla.mozilla.org/show_bug.cgi?id=801393 for more information.
(Assignee)

Comment 37

5 years ago
Yeah - they actually changed it from a function to a getter just as I was landing it on m-c :)  I've a new patch ready to go and I'm just trying to verify the test on a local aurora.
(Assignee)

Comment 38

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6786219223ab
status-firefox18: affected → fixed
(Assignee)

Comment 39

5 years ago
*sigh* - forgot to qref, so pushed the exact same thing :(  Philor said it was OK to push the 1-line fix as a followup.

https://hg.mozilla.org/releases/mozilla-aurora/rev/149ad5c9f94d
Looks like this patch has tests -- is that correct?
Flags: in-testsuite?
(Assignee)

Comment 41

5 years ago
it has *serious* tests ;)
Flags: in-testsuite? → in-testsuite+
Ha ha, thanks Mark. Marking this verified based on tests.
Status: RESOLVED → VERIFIED
status-firefox18: fixed → verified
status-firefox19: fixed → verified
You need to log in before you can comment on or make changes to this bug.