931092 - Jetpack widgets in the panel should display a label and be either a "wide widget" or minimum-sized to a single button's size

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

Two such add-ons:

Mozilla Lightbeam: https://addons.mozilla.org/firefox/downloads/latest/363974/addon-363974-latest.xpi?src=search
Save Everything: https://people.mozilla.org/~dhenein/addons/save-everything/save-everything.xpi

See screenshot: http://i.imgur.com/aVvSaMY.png

That scissor icon (Save Everything) and the Mozilla Lightbeam icon aren't playing nicely in the grid.

DOM Inspector reveals that both of these add-ons are using single iframes in order to display an icon. That's pretty heavy-weight, and not a use-case the Australis team ever imagined.

Is this iframe thing necessary? What's the deal here?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey ZER0, any ideas for what we can do about this?

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

This is why we introduced the new Button API in Australis; the Widget API will be deprecated and removed soon or later. If the devs want to have only a button with an icon, they can use the new Button API.

The Widget API were made to allow custom HTML inside – e.g. displays canvas, more complex UI without needs to use XUL – so we cannot change them without break previous add-ons.

We could probably trying to reduce the cases, for those add-ons that are using `contentURL` only to display icon, and the widget doesn't have `width` and `contentScript*`; but it'll be still an issue for those add-ons that doesn't match these criteria.

Comment 3

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

If we can't figure out the changes we'll need to make for customization to not break, then we should start considering the ramifications of marking add-ons that use the Widget API as incompatible with the Australis release.

Comment 4

[Dave Townsend [:mossop]]

I'm confused about why this has only just come up. We've been talking about widgets and Australis for months and work has been put in to support them in Australis and now suddenly we're suggesting dropping them in Australis when we have no replacement in place for developers to migrate to? That doesn't work with the deprecation policy that we have in place for the add-on SDK.

Comment 5

[Justin Dolske [:Dolske]]

Yeah, any deprecation talk is quite premature.

I'm also kind of confused about what's happening here. My understanding was that Jetpack widgets would (and did) work just fine in Australis, with the exception of "wide" widgets which are just inherently not a good fit -- but also a minor use case (see bug 894390 comment 14). Jetpack widgets work fine in the navbar, so what's causing them to not work fine in the menu panel? And what needs to be done to make that work?

If the Lightbeam and Save Everything addons are doing something unique and crazy then that's one thing, but the discussion here is making it sound like this is a more-global problem. (Do we know if that's actually true?) I think the first step here is to figure out exactly why these two addons are not playing nicely with Australis, and if they're edge-cases or something we need to fix.

Comment 6

[:Gijs (he/him)]

So from 5 minutes of looking at this, it looks like we should teach the SDK widget module to:
- show a label when in the menupanel
- center their iframe into the grid shape

For widgets that are too wide to have this happen, we should probably treat them as 'wide widgets' and make them take up one panel row, probably without a label, too.

This will need to happen in the SDK, I suspect. I can look at this tomorrow if there's no clear other prospective assignee.

I do think that the lightbeam folks should have used Erik Vold's toolbarbutton module instead of the widget module, but that's a separate bug that ought to be filed against lightbeam. (I've not looked at the save everything widget)

I think the reason we've not thought about this too much is because this is a bizarre use of the module from a technology perspective: why use something like an iframe if you only need 1 icon? We've been worried about things which are too big, rather than too small. Which is logical, to a degree, but clearly not enough.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #6)
> I do think that the lightbeam folks should have used Erik Vold's
> toolbarbutton module instead of the widget module, but that's a separate bug
> that ought to be filed against lightbeam. (I've not looked at the save
> everything widget)
> 

In Lightbeam's defense, the Widget API is the approach used in the SDK tutorial for adding items to the toolbar:

https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/tutorials/adding-toolbar-button.html

> I think the reason we've not thought about this too much is because this is
> a bizarre use of the module from a technology perspective: why use something
> like an iframe if you only need 1 icon? We've been worried about things
> which are too big, rather than too small. Which is logical, to a degree, but
> clearly not enough.

I have a sneaking suspicion that this bizarre use is widespread, due to the above tutorial.

Comment 8

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > I do think that the lightbeam folks should have used Erik Vold's
> > toolbarbutton module instead of the widget module, but that's a separate bug
> > that ought to be filed against lightbeam. (I've not looked at the save
> > everything widget)
> > 
> 
> In Lightbeam's defense, the Widget API is the approach used in the SDK
> tutorial for adding items to the toolbar:
> 
> https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/
> tutorials/adding-toolbar-button.html

Ugh.

> > I think the reason we've not thought about this too much is because this is
> > a bizarre use of the module from a technology perspective: why use something
> > like an iframe if you only need 1 icon? We've been worried about things
> > which are too big, rather than too small. Which is logical, to a degree, but
> > clearly not enough.
> 
> I have a sneaking suspicion that this bizarre use is widespread, due to the
> above tutorial.

We should check that your suspicion holds using addons-mxr and take appropriate steps. In any case, the steps I outlined will help - it's just going to mean the add-on is going to look a bit silly, using about 1/5 of the space it has for an icon to display that icon (well, depending on the size they specify for their iframe!) but it shouldn't look as busted as it does now.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to Mike Conley (:mconley) from comment #7)

> I have a sneaking suspicion that this bizarre use is widespread, due to the
> above tutorial.

My impression was that this was common usage.


(In reply to :Gijs Kruitbosch from comment #8)

> [...] it's just
> going to mean the add-on is going to look a bit silly, using about 1/5 of
> the space it has for an icon to display that icon (well, depending on the
> size they specify for their iframe!) but it shouldn't look as busted as it
> does now.

I think that's fine; we don't need heroic measures for making it look elegant, but making the panel look not-totally-busted is important.

Comment 10

[:Gijs (he/him)]

Attachment: refactor insertion code so we can update individual nodes,

So in order for this to work, the nodes need to have the right anchor/areatype attributes. Right now they don't. That's because I wrote some ugly roll-your-own hacks in the SDK at the time. Fixing it means we need an API on our side. Fortunately that's not hard given the existing refactoring.

Comment 11

[:Gijs (he/him)]

Attachment: diff -w

Diff -w (sadly imperfect because of variable->arguments renames)

Comment 12

[:Gijs (he/him)]

Attachment: use APIs instead of our own insertion stuff,

So this makes the SDK use our API. I still noticed a lot of issues with the symbiont because the docshell was nonexistent. So I fixed it to not complain if it doesn't have a docshell yet. I ran all the testpkgs tests against my UX build, they all passed. Matteo, what do you think?

Comment 13

[:Gijs (he/him)]

Attachment: add styling specifically for widgets,

So the previous patch added an attribute which we can use to recognize SDK widgets and style them appropriately. I really would have preferred to do this somewhere in SDK land, but they don't have any stylesheets, so I think we should do it here. Note also that the above patches, AFAICT, fix a decent amount of the issues Matteo was seeing in bug 894390. This is not perfect because it doesn't add the hover style, and because the SDK sets some hardcoded margins which might not be overridden yet, and so on. Unfortunately... I'm not really sure if/how that should be fixed. We have no way of knowing of what kind of crazy stuff is in the iframe. The default hover style is just as likely to be wrong. Likewise, clicking the newly added label doesn't 'work', because the SDK only listens for clicks on the iframe. That last part could potentially be fixed... not 100% sure. Matteo? In the meantime, this is enough to get us out of P1 omgwtfbbq land, as far as I'm concerned.

Comment 14

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

(In reply to Jared Wein [:jaws] from comment #3)
> If we can't figure out the changes we'll need to make for customization to
> not break, then we should start considering the ramifications of marking
> add-ons that use the Widget API as incompatible with the Australis release.

Having read subsequent comments in this bug it feels to me like we *don't* have to do this, right?

Comment 15

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

(In reply to :Gijs Kruitbosch from comment #6)
...
> I do think that the lightbeam folks should have used Erik Vold's
> toolbarbutton module instead of the widget module, but that's a separate bug
> that ought to be filed against lightbeam. (I've not looked at the save
> everything widget)

I'm needinfo'ing Dethe here.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 824069 [details] [diff] [review]
refactor insertion code so we can update individual nodes,

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

This looks fine to me. Thanks Gijs!

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1858,5 @@
> +    let areaNodes = gBuildAreas.get(placement.area);
> +    if (!areaNodes) {
> +      return false;
> +    }
> +    let container = [...areaNodes].filter((n) => n.ownerDocument.defaultView == aWindow);

A shame we have to iterate the nodes like this - maybe it'd be nice if it was hashed on aWindow...

Bah, I'm just thinking out loud here. It's not actually that bad.

Comment 17

[Dethe Elza]

In Lightbeam, I think I did what Atul already had in the earlier version of Collusion, and he probably got it from the tutorial? I'm not attached to this way of doing it, and in fact we've had a lot of issues because it is hard to find. I've noticed other add-ons put their "trigger me" button in the Navigation or Bookmarks toolbars that are more likely to be visible than the add-ons toolbar, but I don't know how kosher that is with the add-ons team.

Comment 18

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

(In reply to Dethe Elza from comment #17)
> ...but I don't know how kosher that is with the add-ons
> team.

There are a very large number of Jetpacks already using Erik's toolbar library to great effect, although finding it has been a little difficult due to some repos going away. You can always ping Erik to get the latest and greatest or copy the code out of here:

https://github.com/canuckistani/toolbar-template/

Comment 19

[:Gijs (he/him)]

(In reply to Jeff Griffiths (:canuckistani) from comment #14)
> (In reply to Jared Wein [:jaws] from comment #3)
> > If we can't figure out the changes we'll need to make for customization to
> > not break, then we should start considering the ramifications of marking
> > add-ons that use the Widget API as incompatible with the Australis release.
> 
> Having read subsequent comments in this bug it feels to me like we *don't*
> have to do this, right?

No, I don't think that's necessary, although many of these widgets would probably be better served using a different API.

Comment 20

[:Gijs (he/him)]

Comment on attachment 824073 [details] [diff] [review]
use APIs instead of our own insertion stuff,

Redirecting review per discussion on IRC.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 824073 [details] [diff] [review]
use APIs instead of our own insertion stuff,

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

I know basically nothing about symbiont so I don't think I can be trusted to review this. Irakli?

Comment 22

[:Gijs (he/him)]

I've landed the first patch as https://hg.mozilla.org/projects/ux/rev/2666f7e02336, so it doesn't bitrot. Waiting with the other UX patch until the jetpack parts have review.

Comment 23

[:Gijs (he/him)]

Comment on attachment 824069 [details] [diff] [review]
refactor insertion code so we can update individual nodes,

I'll take obsolete as a poor man's checkin+... which we don't seem to have in the add-on SDK component. :-(

Comment 24

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 824073 [details] [diff] [review]
use APIs instead of our own insertion stuff,

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

::: addon-sdk/source/lib/sdk/content/symbiont.js
@@ +108,5 @@
>      this._frame = frame;
>  
> +    if (getDocShell(frame)) {
> +      this._reallyInitFrame(frame);
> +    } else {

In jetpack – unfortunately – our guidelines says to put the `else` block to the new line. So:

if (...) {

}
else {

}

::: addon-sdk/source/lib/sdk/widget.js
@@ +729,5 @@
>  
> +  // For use in styling by the browser
> +  node.setAttribute("sdkstylewidget", "true");
> +  // Mark wide widgets as such:
> +  if (this.window.CustomizableUI && this._widget.width > 75) {

It's not clear where `75` comes from, and why is need. It would be better if you add a `const` with this value, and maybe a comment to says where it comes from / what's the purpose

Comment 25

[:Gijs (he/him)]

Attachment: use APIs instead of our own insertion stuff,

Updated patch for checkin

Comment 26

[:Gijs (he/him)]

Comment on attachment 830885 [details] [diff] [review]
use APIs instead of our own insertion stuff,

Carrying over r+

Comment 27

[:Gijs (he/him)]

Comment on attachment 824076 [details] [diff] [review]
add styling specifically for widgets,

https://hg.mozilla.org/projects/ux/rev/be920ff7556b

Comment 28

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/125945ec6b01da4886d97c58279f3cd3e84f6ec5
Bug 931092 - Jetpack widgets in the panel should display a label and be either a "wide widget" or minimum-sized to a single button's size

https://github.com/mozilla/addon-sdk/commit/5f35823e9c91153ee5419ce8c1f6469e8446b385
Merge pull request #1289 from ZER0/widget/931092

Bug 931092 - Jetpack widgets in the panel should display a label and be ... r=@ZER0

Comment 29

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/2666f7e02336
https://hg.mozilla.org/mozilla-central/rev/be920ff7556b