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

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
RESOLVED FIXED
a month ago
3 days ago

People

(Reporter: TheOne, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

55 Branch
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
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 ?
(Reporter)

Updated

a month ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 1

a month ago
(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]
(Reporter)

Comment 2

a month ago
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)
(Assignee)

Comment 3

a month ago
(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)
(Reporter)

Comment 4

a month ago
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.
(Assignee)

Comment 5

a month ago
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
Last Resolved: a month ago
Flags: needinfo?(awagner)
Resolution: --- → INVALID
(Assignee)

Comment 6

a month ago
(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 → ---
(Assignee)

Updated

a month ago
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)
(Reporter)

Comment 7

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

Comment 8

a month ago
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");
}

Updated

a month ago
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)
(Assignee)

Comment 10

a month ago
(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.
(Assignee)

Comment 11

29 days ago
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]
(Assignee)

Updated

29 days ago
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
Comment hidden (mozreview-request)

Updated

22 days ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Priority: P2 → P1

Comment 13

21 days ago
mozreview-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/#review167588
Attachment #8890840 - Flags: review?(jaws) → review+

Comment 14

21 days ago
mozreview-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?

Comment 15

21 days ago
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
(Assignee)

Updated

21 days ago
Blocks: 1385213
(Assignee)

Comment 16

21 days ago
(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

21 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18cb88892539
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago21 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1387512
Any suggested STR to be able to verify? Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 19

6 days ago
(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)
You need to log in before you can comment on or make changes to this bug.