Panel api change to show() method breaks compatibility

RESOLVED FIXED in Firefox 23

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: canuckistani, Assigned: zer0)

Tracking

({addon-compat})

unspecified
mozilla25
x86
Mac OS X
addon-compat

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ fixed, firefox24+ fixed, firefox25 fixed)

Details

Attachments

(2 attachments)

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"
status-firefox22: --- → unaffected
status-firefox23: --- → affected
tracking-firefox23: --- → ?
Blocks: 732294
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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?
Flags: needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths)
Update: we can issue warnings, so let's add the warning now, in 23.
Assignee: nobody → zer0
Priority: -- → P1
tracking-firefox23: ? → +
Jorge - can you help get the affected developer's on this bug and search amo for others?
Flags: needinfo?(jorge)
Keywords: addon-compat
(Assignee)

Comment 6

4 years ago
Created attachment 761678 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

Pointer to Github pull-request
(Assignee)

Updated

4 years ago
Attachment #761678 - Flags: review?(evold)
Comment on attachment 761678 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

we need some tests
Attachment #761678 - Flags: review?(evold) → review-
(Assignee)

Comment 8

4 years ago
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.
Flags: needinfo?(evold)
(In reply to lsblakk@mozilla.com [: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.
Flags: needinfo?(jorge)
(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.
Flags: needinfo?(evold)
(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.
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?
Flags: needinfo?(zer0)
(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
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.
tracking-firefox23: + → -
tracking-firefox24: --- → ?
(Assignee)

Comment 15

4 years ago
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.
Flags: needinfo?(zer0)
(Assignee)

Updated

4 years ago
Attachment #761678 - Flags: review- → review?
(Assignee)

Updated

4 years ago
Attachment #761678 - Flags: review? → review?(evold)
(Assignee)

Comment 16

4 years ago
Updated the Pull Request with the required unit test.
Comment on attachment 761678 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1030

looks good thanks!
Attachment #761678 - Flags: review?(evold) → review+

Comment 18

4 years ago
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

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
tracking-firefox24: ? → +
Created attachment 773714 [details] [diff] [review]
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
Attachment #773714 - Flags: approval-mozilla-beta?
Attachment #773714 - Flags: approval-mozilla-aurora?
tracking-firefox23: - → +
Attachment #773714 - Flags: approval-mozilla-beta?
Attachment #773714 - Flags: approval-mozilla-beta+
Attachment #773714 - Flags: approval-mozilla-aurora?
Attachment #773714 - Flags: approval-mozilla-aurora+
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.
(In reply to lsblakk@mozilla.com [: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
https://hg.mozilla.org/releases/mozilla-aurora/rev/582ac0f3876f
https://hg.mozilla.org/releases/mozilla-beta/rev/5731fd9b6701
status-firefox23: affected → fixed
status-firefox24: --- → fixed
status-firefox25: --- → fixed
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.