1407718 - Developer button in overflow menu is missing the '>' to indicate a subview

Status: NEW

(Firefox :: Theme, defect, P4)

Description

[Sam Foster [:sfoster] (he/him)]

When in the overflow menu, the "Developer" button should have a ">" on the right (LTR) to indicate that clicking it opens a sub-view (menu) of choices. 

As of bug 1395674, this works for e.g. the Library button. 

STR: 
* From the Customize screen, drag the "Developer" button to the overflow menu area. 
* Click done
* Click the overflow menu in the toolbar (>>) to see the Developer button in the menu

Expected results: 
The developer button has a ">>" next to it, clicking the button opens a Developer sub-menu

Actual results: 
The developer button has not ">>" next to it (but clicking the button still opens a Developer sub-menu)

Comment 1

[:Gijs (he/him)]

This is happening because it's not a builtin widget, but is constructed externally by devtools.

Now that we no longer have XUL add-ons, we can probably make createWidget not enforce the source to be non-builtin, and only make calls from webextension code pass the alternative source.

Comment 2

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (blocked reviews; no response to ni until Nov 2 (PTO)) from comment #1)
> This is happening because it's not a builtin widget, but is constructed
> externally by devtools.

Right, we added this guard during review in bug 1395674: http://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#1481

> Now that we no longer have XUL add-ons, we can probably make createWidget
> not enforce the source to be non-builtin, and only make calls from
> webextension code pass the alternative source.

I'm not sure I understand. We have bug 1403666 to address one of the issues that prevent us from handling webextensions properly in this context. The problem outlined there is that a webextension can add/remove/change its panel (subview here) at anytime. So while it might not have a panel (subview) when buildWidget is called, it might add one at some point later. But I'm not sure that's what you are talking about here? 

I'm a bit in ignorance corner here, so I'm probably missing obvious connections. For example, why do we have a appMenu-developer-button hardcoded into panelUI.inc.xul, but this button is from an external widget? 

Is this actually just a dupe of bug 1403666? Or is devtools different in some useful way that would let us fix this in the short-term?

Comment 3

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #2)
> (In reply to :Gijs (blocked reviews; no response to ni until Nov 2 (PTO))
> from comment #1)
> > Now that we no longer have XUL add-ons, we can probably make createWidget
> > not enforce the source to be non-builtin, and only make calls from
> > webextension code pass the alternative source.
> 
> I'm not sure I understand. We have bug 1403666 to address one of the issues
> that prevent us from handling webextensions properly in this context. The
> problem outlined there is that a webextension can add/remove/change its
> panel (subview here) at anytime. So while it might not have a panel
> (subview) when buildWidget is called, it might add one at some point later.
> But I'm not sure that's what you are talking about here? 

No, that's pretty much why we added the check for the source of the widget.

> I'm a bit in ignorance corner here, so I'm probably missing obvious
> connections.

Not so much obvious (so not your fault!) as historical, I suspect.

Once upon a time, while working up to Australis, It Was Decided that when the user used "restore defaults" in customize mode, we needed to restore Firefox-only, and put any add-on items in the palette.

In order to do that, add-on items had to be distinguishable. This is the reason that SOURCE_BUILTIN and SOURCE_EXTERNAL exist.

At the time, webextensions didn't exist and so XUL add-ons were free to call whatever APIs they liked. To avoid a situation where add-ons "got around" this requirement, we made it non-trivial for any consumers other than CustomizableWidgets.jsm to add SOURCE_BUILTIN items, so that add-ons would always be marked SOURCE_EXTERNAL, and couldn't "cheat" by pretending they were SOURCE_BUILTIN. Specifically, createWidget hardcodes the source as SOURCE_EXTERNAL: http://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#2253 .

This requirement could be relaxed now. We could just give createWidget a second argument, and make the default value SOURCE_EXTERNAL (so that webextension consumers still get marked SOURCE_EXTERNAL), but from the devtools call (seems to be here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#394 and here: https://dxr.mozilla.org/mozilla-central/source/devtools/shim/devtools-startup.js#328 ) pass SOURCE_BUILTIN explicitly.

In fact, it might be worth auditing non-test createWidget callsites to see if we should do the same thing in other places as well.

As a side-effect, this will essentially fix bug 1023319. We'd still eventually want to fix bug 1245074 so that mozilla-signed webextensions could mark their add-ons as 'builtin' (well, maybe), but otherwise we should be OK.

> For example, why do we have a appMenu-developer-button
> hardcoded into panelUI.inc.xul, but this button is from an external widget? 

Because it used to be builtin, and then devtools moved it into their own code. Really, they should be constructing the view themselves, too, but that wasn't done at the time. I don't really know why not. Probably worth a separate bug, really.

> Is this actually just a dupe of bug 1403666? Or is devtools different in
> some useful way that would let us fix this in the short-term?

As noted above, yes, we can fix this separately.

Comment 4

[:Gijs (he/him)]

Note that, like a lot of CustomizableUI APIs, you'd need to add the arg to both the 'public' and 'internal' `createWidget` method.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Ok, I think I follow. I'll work up a patch. I guess this would need a test or 2 as well.

Comment 6

[Sam Foster [:sfoster] (he/him)]

2nd thoughts, and sorry to vacillate here, I have a few reservations about this. Partly this is due to the timing - a patch implementing the createWidget signature change suggested in comment #3 would likely not be taken for 57 at this point. And I'm also not sure its the right fix. Currently, we are using the source of the widget to infer something about the predictability of the panel/view behavior - i.e. that unlike webextensions, if they say they have a view when we create the button for the widget, we can trust they will continue to have a view by the time the user clicks on it. As it turns out we don't need (or cant use) a quick fix, ISTM fixing bug 1403666 and adopting/adapting that solution for devtools etc. is a better way forward.

Comment 7

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #6)
> As it turns out we don't need (or cant use) a quick fix,
> ISTM fixing bug 1403666 and adopting/adapting that solution for devtools
> etc. is a better way forward.

Well, we could hardcode a quick fix for this item in CSS, I imagine, but it's a bit hacky, and we've just missed 57.

I'm not clear on what you mean by "adopting/adapting that solution for devtools". The button isn't a webextension browser action right now, it's just some XUL + JS. Can you clarify? And, related, do you want to remain assigned, or should we update this bug to be assignee:[e-mail] and P3/P4 again?

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (no reviews; PTO recovery mode) from comment #7)
> Well, we could hardcode a quick fix for this item in CSS, I imagine, but
> it's a bit hacky, and we've just missed 57.

I had been led to believe that the window for something like this was effectively already closed when I wrote that comment (else I wouldn't have sat on it for the last 2 week. Promise!)

> I'm not clear on what you mean by "adopting/adapting that solution for
> devtools". The button isn't a webextension browser action right now, it's
> just some XUL + JS. Can you clarify? 

I mean defining and implementing a more consistent API for *any* widget that wants to allow drill-down of panels/subviews in a menu. I don't think we want to carry around one mechanism for builtin stuff, another for devtools and another for extensions. They should all present a property or event when "attached" that informs the hosting chrome toolbar/panel what presentation needs they require. No?

Off the cuff, maybe at the initial attached event/moment the target says to the toolbar/panel "I'm now attached to you and here's my properties." And if those change while attached, another event with the details (e.g. I now have a subview/I no longer have a subview). Something like that anyhow which is feasible both in the same (parent) process and also via cross-process async messaging. 

I know that's not fleshed out enough to be actionable and may not be feasible, but that's at least how I would naively expect this to work. 

> And, related, do you want to remain
> assigned, or should we update this bug to be assignee:[e-mail] and
> P3/P4 again?

I don't not actively working on this so I'll unassign for now and set to P4.