1380274 - Overflow panel customizable area doesn't declare an anchor (like the hamburger panel) breaking share widget and potentially other things

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Andreas Wagner [:TheOne] [use NI]]

This is similar to bug 1380084, but for Add-on SDK toolbar buttons.

Gijs, could we apply the one-line fix from that bug to https://hg.mozilla.org/mozilla-central/file/tip/addon-sdk/source/lib/sdk/ui/button/view.js#l141 ?

Comment 1

[:Gijs (he/him)]

(In reply to Andreas Wagner [:TheOne] [use NI] from comment #0)
> This is similar to bug 1380084, but for Add-on SDK toolbar buttons.
> 
> Gijs, could we apply the one-line fix from that bug to
> https://hg.mozilla.org/mozilla-central/file/tip/addon-sdk/source/lib/sdk/ui/
> button/view.js#l141 ?

I don't see why. We've disabled tests that check things with the sdk for photon already, because they break, and our understanding is we'll remove the sdk for 57. Why bother making changes?

Comment 2

[Andreas Wagner [:TheOne] [use NI]]

You are probably right. I just thought a simple one-line fix wouldn't hurt.
But I guess since photon doesn't ride the trains before 57, it'll work for 56 beta and release.

Comment 3

[:Gijs (he/him)]

(In reply to Andreas Wagner [:TheOne] [use NI] from comment #2)
> You are probably right. I just thought a simple one-line fix wouldn't hurt.
> But I guess since photon doesn't ride the trains before 57, it'll work for
> 56 beta and release.

Certainly in the overflow panel, yes.

What happens if these items are in the (non-photon) hamburger panel on beta right now? Because tbh, they should work the same way, and I am confused as to why there'd be a difference. If they do behave the same way, then this is a pre-existing bug. If they don't, I'm still somewhat curious what the difference is... (the library button to which comment #0 referred is a bit special, so that doesn't explain webextension (bug 1380109) or sdk (this bug) behaviour.

Comment 4

[Andreas Wagner [:TheOne] [use NI]]

I'm not entirely sure I follow your question.

Here's what I did.

1. In Nightly, set 'browser.photon.structure.enabled' to false.
2. Right-click on the toolbar button and select 'Move to Menu'.
3. Click on the item in the hamburger menu.

Result:
The panel shows up just fine and stays open as expected.

Comment 5

[:Gijs (he/him)]

OK, having debugged this a bit, I don't think this is an issue in Firefox code.

Specifically, the add-on creates a toggle button like this:

var widget = ToggleButton({
    id: "acr-dialog",
    label: "Addon Compatibility Reporter",
    icon: {
        "16": self.data.url("image/extensionGeneric-16-" + os + ".png"),
        "32": self.data.url("image/extensionGeneric-" + os + ".png"),
    },
    onChange: function(state) {
        if (state.checked)
            reporterPanel.show({ position: widget });
    }
});

This isn't a "view" type widget like in the code comment #0 references.

Instead, this presumably manually tries to anchor against the position of the widget, and because of this change:

https://hg.mozilla.org/mozilla-central/rev/79cbec4c644c#l1.12

we used to report the anchor of an item in the overflow panel as the anchor of the overflow panel (so the panel the above code tries to show shows anchored on the overflow button when the button is in the overflow panel on a narrow window in release). However, now it will report the button itself, as in principle we can anchor subviews against the button in the overflow panel (iff the panelmultiview/photonpanelmultiview is present, which is only the case with photon).

It's kind of hard to follow the sdk panel code through its approximately 41235 layers of indirection (panel.js show(), panel/utils.js show(), open(), display(), side trip to core.js getNodeView(), but I *think* you could manually add/remove the cui-anchorid attribute on your button's node to the overflow button, and that *might* cause the SDK to do the right thing here.

But really, I would recommend transitioning to webextensions over that, of course. :-)

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (no reviews, mostly out until Jul 17) from comment #5)
> we used to report the anchor of an item in the overflow panel as the anchor
> of the overflow panel (so the panel the above code tries to show shows
> anchored on the overflow button when the button is in the overflow panel on
> a narrow window in release). However, now it will report the button itself,
> as in principle we can anchor subviews against the button in the overflow
> panel (iff the panelmultiview/photonpanelmultiview is present, which is only
> the case with photon).

actually, this only applies for recognized view widgets, too, which isn't the case here.

So then I guess this is because we don't define an anchor property when registering the new-style panel ( https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#320-323 ), which we try to use here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#4040-4046 . We could maybe fix that. I don't know if it has other unforeseen side effects...

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

Thanks again Gijs, the workaround works like a charm!
I'll let it up to you what to do with this bug.

Comment 8

[Andreas Wagner [:TheOne] [use NI]]

For the record, and potentially for others, this is the workaround:

Before the panel is being shown, add or remove 'cui-anchorid', depending on whether the button is in the overflow menu or not:

if (node.closest("#widget-overflow")) {
    node.setAttribute("cui-anchorid", "nav-bar-overflow-button");
} else {
    node.removeAttribute("cui-anchorid");
}

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Gijs, is this really SDK-specific? If so, it should be wontfixed. If not, the bug title maybe could be clarified.

Comment 10

[:Gijs (he/him)]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #9)
> Gijs, is this really SDK-specific? If so, it should be wontfixed. If not,
> the bug title maybe could be clarified.

The short answer is that I'm not sure, and also not 100% sure how to find out. I have some ideas, but I lack time. I will try finding some to poke at this more. Leaving ni for now.

Comment 11

[:Gijs (he/him)]

This also affects the share plane widget in Firefox, and potentially other stuff, so I guess we should just fix this.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1380274 - add an anchor to the definition of the overflow panel area,

Review commit: https://reviewboard.mozilla.org/r/162062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162062/

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890840 [details]
Bug 1380274 - add an anchor to the definition of the overflow panel area,

https://reviewboard.mozilla.org/r/162062/#review167588

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890840 [details]
Bug 1380274 - add an anchor to the definition of the overflow panel area,

https://reviewboard.mozilla.org/r/162062/#review167592

If we had a test for the share button we might have caught this sooner? Can you file a bug for adding a test?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18cb88892539
add an anchor to the definition of the overflow panel area, r=jaws

Comment 16

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8890840 [details]
> Bug 1380274 - add an anchor to the definition of the overflow panel area,
> 
> https://reviewboard.mozilla.org/r/162062/#review167592
> 
> If we had a test for the share button we might have caught this sooner? Can
> you file a bug for adding a test?

Filed bug 1385213.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18cb88892539

Comment 18

[u554753]

Any suggested STR to be able to verify? Thanks!

Comment 19

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #18)
> Any suggested STR to be able to verify? Thanks!

Pin the share plane in the overflow panel, then try to open it. Prior to this patch, it wouldn't open. Now it opens anchored on the overflow button.

Comment 20

[u554753]

There does not seem to be a share button/icon in the Customize window in 57. Is this intended?

Comment 21

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #20)
> There does not seem to be a share button/icon in the Customize window in 57.
> Is this intended?

Yes, it got removed in bug 1388902, after I wrote comment #19... you could verify with builds prior to that landing - off-hand I'm not sure anything else built-in uses these codepaths (which doesn't mean there isn't anything, just that I can't think what that would be right now), in which case I'd just close (ie verify) this out and move on.

Comment 22

[Abe - QA (:Abe_LV)]

Verified as fixed.
Using Firefox nightly (Build ID:20170815100349), I am able to open the sharing window by clicking the 'share plane' from the overflow panel.