878877 - Panel api change to show() method breaks compatibility

Status: RESOLVED FIXED

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

Description

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

From :

https://github.com/mozilla/addon-sdk/commit/dbd07b38531d48745d295370b9ca2a182da6ab26#commitcomment-3340382

"@ZER0 Having the options first breaks backward compatibility with other modules that pass the anchor only. @erikvold's toolbarbutton.js is one such and as a result is broken in Aurora. Could the options simply be the second parameter? I noticed that widget.js passes null to .show() and changing the toolbarbutton module to do the same works but breaks the anchoring in stable so the panel appears unanchored in the document."

This is because ZER0 changed the public panel api from:

show: function show(anchor) {

to:

show: function show(options, anchor) {

Effect: 1Password and likely any other users of Erik's toolbarbutton library are likely broken on Aurora, showing this error:

"Error: RequirementError: The option "width" must be one of the following types: number, undefined, null"

Comment 1

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

From my comment in github: https://github.com/mozilla/addon-sdk/commit/dbd07b38531d48745d295370b9ca2a182da6ab26#commitcomment-3347037

That was expected and discussed (internally, and also in the community, see StackOverflow for instance). The second argument of show panel was never documented intentionally because was an "hack": you can see it's no in any documentation, the show method takes no argument, and was always said that shouldn't be use – and be aware that could be removed.
The XUL node anchor argument was intended only for internal use, by definition High Level API doesn't have as input or ouput a raw XUL element, everything is wrapped – any other High Level API does that.

Currently the anchor is moved as second argument, and it's still not officially documented for the same reason as above. Shortly it will be removed to be replaced by new the panel position API and new UI Component. That together should be cover all the use cases, without using hacks, neither exposes low level stuff to high level API.

Comment 2

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

My suggestion to limit the issue is check if the first argument is a Node. If it's a node, we log a deprecate message and makes it works as before, until we remove also the second argument and provide the full solution (panel position API + new UI Australis Components) to cover all the cases.

Is it sounds reasonable, jeff?

Comment 3

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #2)

> Is it sounds reasonable, jeff?

Not as long as people are using Erik's toolbar module, which currently is the single most popular 3rd party module and way ahead of most internal ones.

I say we introduce code that supports that toolbar module, then only start issuing a deprecation notice once 2 things have happened:

1. we've released the complete Australis UI work
2. following that, analysis of addons on AMO yields diminishing popularity for the toolbar module.

Deprecation notices will prevent extensions from getting full review status, so I want to be careful about when we issue them.

Comment 4

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

Update: we can issue warnings, so let's add the warning now, in 23.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Jorge - can you help get the affected developer's on this bug and search amo for others?

Comment 6

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

Pointer to Github pull-request

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 761678 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

we need some tests

Comment 8

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

Which kind of tests are you suggesting? Just create a DOM node and passing as argument and check if the warning is logged or what?

At the moment we do not have a test for checking if the panel is anchored to a widget, in order to have this such test we need to properly implemented `getNodeView` for `Widget`, and I don't think such test should be a blocker for this fix. We have to land it as soon as we can, because it break the compatibility with your module.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to [e-mail] [:lsblakk] from comment #5)
> Jorge - can you help get the affected developer's on this bug and search amo
> for others?

I don't think that searching for "show" will yield any useful results in the MXR. We would need to do something much more involved to know who is affected, though according to other comments it is known that it affects many developers.

Maybe the deprecation message should also include a telemetry ping so we can measure this more accurately in terms of usage.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> Which kind of tests are you suggesting? Just create a DOM node and passing
> as argument and check if the warning is logged or what?

At least a test that the warning is logged yes.

> At the moment we do not have a test for checking if the panel is anchored to
> a widget, in order to have this such test we need to properly implemented
> `getNodeView` for `Widget`, and I don't think such test should be a blocker
> for this fix. We have to land it as soon as we can, because it break the
> compatibility with your module.

I guess we can't wait for this test, you're right.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #10)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> > At the moment we do not have a test for checking if the panel is anchored to
> > a widget, in order to have this such test we need to properly implemented
> > `getNodeView` for `Widget`, and I don't think such test should be a blocker
> > for this fix. We have to land it as soon as we can, because it break the
> > compatibility with your module.
> 
> I guess we can't wait for this test, you're right.

The test has landed now, Bug 859592.

Comment 12

[Alex Keybl [:akeybl]]

If we don't have enough time to get telemetry, and we're not sure about the add-on impact, why wouldn't we back bug 732294 out of FF23 immediately and follow up in a later release?

Comment 13

[Dave Townsend [:mossop]]

(In reply to Alex Keybl [:akeybl] from comment #12)
> If we don't have enough time to get telemetry, and we're not sure about the
> add-on impact, why wouldn't we back bug 732294 out of FF23 immediately and
> follow up in a later release?

I think that's fine

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

I'll track the backout in bug 732294, but nomming this for FF24 in case it needs to be tracked for landing there once the telemetry is ready.

Comment 15

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

Sorry for the late reply. If we back bug 732294 out of FF23, we have to be sure that the other fixes that are relying on that feature are updated or backed out too. I'm adding the unit test to the bug right now, but especially because was a urgent bug I wouldn't block it from landing because the unit test – in other cases we opened a separate bug for unit testing.

Comment 16

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

Updated the Pull Request with the required unit test.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 761678 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

looks good thanks!

Comment 18

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/2581a98ae411c5a84285a31b5167bbf8380f942d
Bug 878877 - Panel api change to show() method breaks compatibility

- Added warning
- Updated test suite

https://github.com/mozilla/addon-sdk/commit/6e2cad71dc074a5677d5faa824819e512a4f2107
Merge pull request #1030 from ZER0/panel-show/878877

fix Bug 878877 - Panel api change to show() method breaks compatibility

Comment 19

[Dave Townsend [:mossop]]

Attachment: uplift patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 732294
User impact if declined: A small number of add-ons may break
Testing completed (on m-c, etc.): Landed on m-c today. Ran through try server with all green tests
Risk to taking this patch (and alternatives if risky): The alternative is backing out the feature that landed. Its a large amount of code to back out and there is at least one patch on top of it. At this point it is less risky to take the simple uplift patch.
String or IDL/UUID changes made by this patch: None

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Can someone put the m-c landing link in here? We do want this in today's beta so it would be good to confirm it's on central.

Comment 21

[Dave Townsend [:mossop]]

(In reply to [e-mail] [:lsblakk] from comment #20)
> Can someone put the m-c landing link in here? We do want this in today's
> beta so it would be good to confirm it's on central.

It landed in https://hg.mozilla.org/mozilla-central/rev/e4c650dfee0e

Comment 22

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/582ac0f3876f
https://hg.mozilla.org/releases/mozilla-beta/rev/5731fd9b6701