Closed Bug 1380274 Opened 3 years ago Closed 3 years ago

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

Categories

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

55 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- fixed

People

(Reporter: TheOne, Assigned: Gijs)

References

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

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 ?
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(awagner)
Whiteboard: [photon-structure] → [photon-structure][triage]
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.
Flags: needinfo?(awagner)
(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.
Flags: needinfo?(awagner)
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.
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. :-)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(awagner)
Resolution: --- → INVALID
(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...
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: When moving an Add-on SDK toolbar button to the overflow panel, it closes the panel when clicked (doesn't open subview) → When moving an Add-on SDK toolbar button to the overflow panel, it closes the panel when clicked (doesn't open its own panel anchored on overflow button)
Thanks again Gijs, the workaround works like a charm!
I'll let it up to you what to do with this bug.
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");
}
Status: REOPENED → NEW
Flags: qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Gijs, is this really SDK-specific? If so, it should be wontfixed. If not, the bug title maybe could be clarified.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
This also affects the share plane widget in Firefox, and potentially other stuff, so I guess we should just fix this.
Flags: needinfo?(gijskruitbosch+bugs)
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Summary: When moving an Add-on SDK toolbar button to the overflow panel, it closes the panel when clicked (doesn't open its own panel anchored on overflow button) → Overflow panel customizable area doesn't declare an anchor (like the hamburger panel) breaking share widget and potentially other things
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Priority: P2 → P1
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
Attachment #8890840 - Flags: review?(jaws) → review+
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?
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/18cb88892539
add an anchor to the definition of the overflow panel area, r=jaws
Blocks: 1385213
(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.
https://hg.mozilla.org/mozilla-central/rev/18cb88892539
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1387512
Any suggested STR to be able to verify? Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
Flags: needinfo?(gwimberly)
There does not seem to be a share button/icon in the Customize window in 57. Is this intended?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.