1011598 - fix toolbar overflow support for marks and status buttons

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

When marks or status buttons are in the overflow menu, they are opened unattached to anything, in potentially random locations.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: marks button support for overflow

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: better marks panel support when button in menu panel

Share and social marks will generally use pre-existing dialog pages which wont be responsive enough (if at all) to fit into the menu panel.  This changes the marks button to just use it's own panel rather than trying to stuff itself into the menu panel.  We'll leave status panels the way they are (opening inside the menu panel), since those are generally pages custom made for socialapi.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: status button overflow support

only f? because there is some weird problem when the status button, only when it is badged, that causes the overflow panel to close right away *every other time you open it*.  I suspect some weird layering/css/panel thing is going on.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8423999 [details] [diff] [review]
marks button support for overflow

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

Generally, this seems sane, apart from the same comments as in the other bug. There's also some stray code, and I'd ideally like to see tests for this (and a try run to see that this doesn't break existing tests).

::: browser/base/content/socialmarks.xml
@@ +32,5 @@
> +      </property>
> +      <property name="inMenuPanel">
> +        <getter>
> +          let widgetGroup = CustomizableUI.getWidget(this.getAttribute("id"));
> +          let widget = widgetGroup.forWindow(window);

You don't actually use widget here.

@@ +130,5 @@
>          let aURI = gBrowser.currentURI;
> +        let disabled = !aURI || !(aURI.schemeIs('http') || aURI.schemeIs('https'));
> +        // when overflowed in toolbar, we must have the attribute set
> +        this.setAttribute("disabled", disabled);
> +        this.disabled = disabled;

Here, too, the attribute by itself is enough.

@@ +241,5 @@
>                                CustomizableUI.AREA_PANEL);
>          } else {
> +          this._anchorBtn = this;
> +          if (widget.overflowed) {
> +            this._anchorBtn = document.getElementById("nav-bar-overflow-button");

Same comment here, I'm afraid - widget.anchor should just be giving you the right thing here. Does it not?

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3855,5 @@
>          if (!this._toolbar.hasAttribute("overflowing")) {
>            CustomizableUI.addListener(this);
>          }
>          this._toolbar.setAttribute("overflowing", "true");
> +        CustomizableUIInternal.notifyListeners("onWidgetOverflowed", child, this._target);

Err, did you mean to add this? You don't seem to actually be using it...

Comment 5

[:Gijs (he/him)]

Comment on attachment 8424003 [details] [diff] [review]
better marks panel support when button in menu panel

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

This seems even more sane than the other patches. Still one more question (+ the anchor thing), but this is close, I think.

::: browser/base/content/socialmarks.xml
@@ +27,5 @@
> +        <getter><![CDATA[
> +          // if we're a slice in the hambuger, anchor off that button
> +          let widgetGroup = CustomizableUI.getWidget(this.getAttribute("id"));
> +          let windowWrapper = widgetGroup.forWindow(window);
> +  

Nit: whitespace

@@ +34,5 @@
> +            anchorBtn = PanelUI.menuButton;
> +          } else if (widgetGroup.areaType == CustomizableUI.TYPE_TOOLBAR && windowWrapper.overflowed) {
> +            anchorBtn = document.getElementById("nav-bar-overflow-button");
> +          }
> +          return anchorBtn;

More windowWrapper.anchor goodness (I hope! If not, we should fix that getter...). :-)

@@ +346,5 @@
>        ]]></handler>
>        <handler event="popuphidden"><![CDATA[
>          this.dispatchPanelEvent("socialFrameHide");
>          this._anchorBtn.removeAttribute("open");
> +        this.update();

Is this related to this fix? Can you explain how this works / why it's necessary?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8424004 [details] [diff] [review]
status button overflow support

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

I need to look into the problem you described here. Can you needinfo / feedback me again when all the patches are updated, and I'll have a look at the panel issue you described.

Just for clarity, on what platform did you see that weirdness, and/or does it repro on both Windows and OS X?

::: browser/base/content/browser-social.js
@@ +1309,5 @@
>      let provider = Social._getProviderFromOrigin(origin);
>  
> +    // if we're overflowed, our anchor needs to be the overflow button
> +    let widget = CustomizableUI.getWidget(aToolbarButton.getAttribute("id"));
> +    let windowWrapper = widget.forWindow(window);

Again, use the anchor getter. This will also simplify code a bit further down, I think.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8424003 [details] [diff] [review]
> better marks panel support when button in menu panel
> 
> Review of attachment 8424003 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +346,5 @@
> >        ]]></handler>
> >        <handler event="popuphidden"><![CDATA[
> >          this.dispatchPanelEvent("socialFrameHide");
> >          this._anchorBtn.removeAttribute("open");
> > +        this.update();
> 
> Is this related to this fix? Can you explain how this works / why it's
> necessary?

It was part of a handler, which handled both popuphidden and ViewHidding, that is removed in the patch.  Upon closing the panel we update the buttons "marked" state if necessary.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: marks button updates for overflow/menu panel

combining the two marks button patches since that will make these changes a bit clearer.  current tests which are pretty extensive to pass locally.  an older try is here (I'll run another try soon as well)

https://tbpl.mozilla.org/?tree=Try&rev=dc3ebe42d5c0

There are no tests right now for the marks button inside the overflow menu or the menu panel.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8424004 [details] [diff] [review]
> status button overflow support
> 
> Review of attachment 8424004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need to look into the problem you described here. Can you needinfo /
> feedback me again when all the patches are updated, and I'll have a look at
> the panel issue you described.
> 
> Just for clarity, on what platform did you see that weirdness, and/or does
> it repro on both Windows and OS X?

I haven't tried on windows, just on osx.  With a clean profile, install the demo provider from http://mixedpuppy.github.io/socialapi-demo/

Before clicking the fake login in the sidebar (or if you don't see a login, click logout), shrink the window until all the new buttons go into overflow.  The overflow button should work normally when opening/closing it.

In the sidebar click on the fake login button.  That will initiate an update to the notifications badge on the status button.  If you happened to expand the window after the above, reduce window size again until buttons go into overflow.  Open the overflow button, you'll see it closes right away.  Click again, it stays open.  Rinse/Repeat.  Every second open it stays open.

I initially thought I was closing it somewhere, spent a chunk of time investigating, am not sending any event to close it that I could find.  It only happens when the badge is on the button.

Last patch update will be here shortly.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: status button overflow support

updated for comments

Comment 11

[:Gijs (he/him)]

I've been trying to debug this panel thing and I've not been getting anywhere with it, but it's a bad enough bug that I don't really want to review the patch until I've figured that out. I'll look at this more tomorrow, leaving the needinfo for now. Sorry for the delays in reviewing. :-(

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> I've been trying to debug this panel thing and I've not been getting
> anywhere with it, but it's a bad enough bug that I don't really want to
> review the patch until I've figured that out. I'll look at this more
> tomorrow, leaving the needinfo for now. Sorry for the delays in reviewing.
> :-(

Err, so I can reproduce this even without these patches? Shane, is that the case for you as well?

Comment 13

[:Gijs (he/him)]

Comment on attachment 8424388 [details] [diff] [review]
status button overflow support

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

::: browser/base/content/browser-social.js
@@ +1296,5 @@
>      }
>      panel.addEventListener(hidingEvent, function onpopuphiding() {
>        panel.removeEventListener(hidingEvent, onpopuphiding);
> +      if (!inMenuPanel)
> +        anchorBtn.removeAttribute("open");

Should this happen for the overflow case as well?

@@ +1318,5 @@
>            dynamicResizer.start(panel, notificationFrame);
>          dispatchPanelEvent("socialFrameShow");
>        };
>        if (!inMenuPanel)
> +        anchorBtn.setAttribute("open", "true");

Same question here.

@@ +1337,3 @@
>                            CustomizableUI.AREA_PANEL);
>      } else {
> +      // in overflow, the anchor is a normal toolbarbutton, in toolbar it is a badge button

Orthogonal: this sounds like it might be related to the panel closure stuff...

Comment 14

[:Gijs (he/him)]

Comment on attachment 8424386 [details] [diff] [review]
marks button updates for overflow/menu panel

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

This looks fine to me.

If you don't have any clues regarding the panel issue, can you file a followup bug and make it part of the upcoming iteration? Ideally, I don't think we should release with a bug like that...

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > I've been trying to debug this panel thing and I've not been getting
> > anywhere with it, but it's a bad enough bug that I don't really want to
> > review the patch until I've figured that out. I'll look at this more
> > tomorrow, leaving the needinfo for now. Sorry for the delays in reviewing.
> > :-(
> 
> Err, so I can reproduce this even without these patches? Shane, is that the
> case for you as well?

sigh.  yes.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8424388 [details] [diff] [review]
status button overflow support

Unfortunately just getting back to this now.

Comment 17

[Shane Caraveo (:mixedpuppy)]

bug 1029937 for followup on the overflow menu issue discovered here.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8424388 [details] [diff] [review]
status button overflow support

bitrot :(

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: status button overflow support

Some of the status button code got refactored for reuse with loop.  The patch is otherwise the same.  f? Standard8 so he is aware of the changes as they may affect loop (for the better I suspect).

Comment 20

[:Gijs (he/him)]

Comment on attachment 8445613 [details] [diff] [review]
status button overflow support

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

Looks sensible enough to me.

::: browser/modules/PanelFrame.jsm
@@ -115,3 @@
>      // if we're a slice in the hamburger, use that panel instead
> -    let widgetGroup = CustomizableUI.getWidget(aToolbarButton.getAttribute("id"));
> -    let widget = widgetGroup.forWindow(aWindow);

Could just reuse the same variable names as you used here and add the anchor ref? It'd save you updating a lot of the rest of the file. I don't really feel strongly though.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8445613 [details] [diff] [review]
status button overflow support

I just applied this on top of fixing Loop's button overflow, and it worked fine. Thanks for checking.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: fix marks/status buttons for overflow menu

rollup of the two patches, along with last review comments from Gijs.  carry forward r+

https://tbpl.mozilla.org/?tree=Try&rev=5199b7715a62

Comment 23

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/10862c0379de

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/10862c0379de

Comment 25

[Shane Caraveo (:mixedpuppy)]

@Gijs, do we want to uplift this?

Comment 26

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> @Gijs, do we want to uplift this?

I defer to your judgment on this one, I think...

Comment 27

[u279076]

Florin, normally I'd ask Paul to test this since he's our primary tester on Nightly. However, I know he is out on PTO this week. Could you have someone test this to make sure it's working, ideally before uplift.

Comment 28

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've added this to the team's To Do list, but will likely verify it only next week after the release for Firefox 31. I'm keeping the needinfo so I won't miss it.

Comment 29

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0

I`ve tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4 using clean profile on latest Nightly (buildID: 20140716030202). 

Using this steps:
1. Open Nightly
2. Install mixedpuppy.github.io/socialapi-demo/
3. Resize Nightly until the social buttons (Share this page, Demo Social Service and Save Page to Demo Social Service) are hidden in overflow button
4. Click 'Sign in' in the Demo Social Service sidebar.
5. Click the overflow button.

Expected result: Overflow panel shows items.

Actual result: Overflow panel closes right away.

Notes:
1. If I skip step 4 I get the expected result. 
2. On Ubuntu 14.04 32bit this seems to be fixed, the issue does not reproduce on latest Nightly.

Based on my testing I see that this issue is still reproducible. Shane can you please take a look at this?

Comment 30

[:Gijs (he/him)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0
> 
> I`ve tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4 using
> clean profile on latest Nightly (buildID: 20140716030202). 
> 
> Using this steps:
> 1. Open Nightly
> 2. Install mixedpuppy.github.io/socialapi-demo/
> 3. Resize Nightly until the social buttons (Share this page, Demo Social
> Service and Save Page to Demo Social Service) are hidden in overflow button
> 4. Click 'Sign in' in the Demo Social Service sidebar.
> 5. Click the overflow button.
> 
> Expected result: Overflow panel shows items.
> 
> Actual result: Overflow panel closes right away.
> 
> Notes:
> 1. If I skip step 4 I get the expected result. 
> 2. On Ubuntu 14.04 32bit this seems to be fixed, the issue does not
> reproduce on latest Nightly.
> 
> Based on my testing I see that this issue is still reproducible. Shane can
> you please take a look at this?

As noted in earlier comments on this bug, this was reproducible before and after this patch, and it's tracked in bug 1029937.

Comment 31

[Shane Caraveo (:mixedpuppy)]

as Gijs said, that particular problem is bug 1029937

Comment 32

[Bogdan Maris, Desktop QA]

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #30)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> > Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0
> > 
> > I`ve tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4 using
> > clean profile on latest Nightly (buildID: 20140716030202). 
> > 
> > Using this steps:
> > 1. Open Nightly
> > 2. Install mixedpuppy.github.io/socialapi-demo/
> > 3. Resize Nightly until the social buttons (Share this page, Demo Social
> > Service and Save Page to Demo Social Service) are hidden in overflow button
> > 4. Click 'Sign in' in the Demo Social Service sidebar.
> > 5. Click the overflow button.
> > 
> > Expected result: Overflow panel shows items.
> > 
> > Actual result: Overflow panel closes right away.
> > 
> > Notes:
> > 1. If I skip step 4 I get the expected result. 
> > 2. On Ubuntu 14.04 32bit this seems to be fixed, the issue does not
> > reproduce on latest Nightly.
> > 
> > Based on my testing I see that this issue is still reproducible. Shane can
> > you please take a look at this?
> 
> As noted in earlier comments on this bug, this was reproducible before and
> after this patch, and it's tracked in bug 1029937.

Yes, I must have missed that, sorry..

I reproduced the issue from this bug on Nightly (2014-05-16), verified as fixed on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4 using latest Nightly (2014-07-21)