Closed Bug 981418 Opened 6 years ago Closed 6 years ago

CustomizableUI type:view and type:button widgets need an onBeforeCreated handler

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: 4mr.minj, Assigned: Gijs)

References

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [Australis:P3])

Attachments

(1 file, 1 obsolete file)

This bug affects type:view widgets.

There appears to be an inherent race condition with view panel creation in the window listener of bootstrapped add-ons.

When new windows are opened add-on code in the window listener(s) and CustomizableUI code run independently. 

However, CustomizableUI needs a given view to exist before buildWidget is executed: http://lxr.mozilla.org/mozilla-aurora/source/browser/components/customizableui/src/CustomizableUI.jsm#1125

This means that if add-on code that creates the relevant view is sufficiently delayed for any reason, CustomizableUI fails to create the widget with the following error:
console.error:
[CustomizableUI]
Could not find the view node with id: myapp-view, for widget: myapp-toolbar-button.

There is an onBuild handler intended for custom widgets only.
Specifying type:custom and rewriting type:view code in onBuild() is not an ideal solution for what I hope are obvious reasons.

It seems to me that this should be fixed at the platform level. Namely, there should be an onBeforeBuild handler (at http://lxr.mozilla.org/mozilla-aurora/source/browser/components/customizableui/src/CustomizableUI.jsm#1075) to allow creating panelviews in time regardless of any circumstances.
I considered adding this for bug 978249, but Jorge reported that using DOMContentLoaded works. Why doesn't it work for you?
Flags: needinfo?(4mr.minj)
Right, I failed to find that bug, sorry. Changing the window listener indeed solves it, thank you.

This should be documented somewhere I think.
Flags: needinfo?(4mr.minj)
Actually, this does not solve the problem. It works for opening new windows, for sure. But when disabling/enabling add-on with already loaded windows, CustomizableUI still races ahead of panelview creation.
juggling this singleton and per-window code is a pain
(In reply to 4mr.minj from comment #3)
> Actually, this does not solve the problem. It works for opening new windows,
> for sure. But when disabling/enabling add-on with already loaded windows,
> CustomizableUI still races ahead of panelview creation.

Presumably you just shouldn't be calling createWidget before looping through the windows and having ensured that your panelview is in there... that doesn't seem terrible (and has little to do with what handlers you use for windows that haven't loaded yet).

In any case, maybe we should consider adding this. Blair, thoughts?
Flags: needinfo?(bmcbride)
Yea, we should definitely support this. Add-ons can do it via load events and when adding the widget - but they shouldn't have to if we can make it easier. And the view nodes may never need to exist - if we can easily support lazily creating the view, we should (anything to help reduce what we do on new window startup).
Blocks: australis-addonfixes
No longer blocks: australis-cust
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bmcbride)
onViewShowing can be used to do more lazy building. I think this is the simplest way to fix the problem. Adding more laziness would be possible, but comes with tracking in which windows we've ensured that the widget's view has the appropriate classes and handles. Additionally, not setting these immediately when the view is created might cause display issues in the multiview (which is obviously theoretical right now, but after the issues with the SDK widgets I'd rather not risk it). Jared, does this look right to you?
Attachment #8388901 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8388901 [details] [diff] [review]
add onBeforeBuild handler for Australis widget,

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

r=me with the issues below fixed.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -1082,5 @@
>      let node;
>      if (aWidget.type == "custom") {
>        if (aWidget.onBuild) {
> -        try {
> -          node = aWidget.onBuild(aDocument);

i checked and the removal of this is OK since these functions are wrapped by wrapWidgetEventHandler which puts a try/catch around the add-on supplied code.

@@ +1088,5 @@
>          ERROR("Custom widget with id " + aWidget.id + " does not return a valid node");
>      }
>      else {
> +      if (aWidget.onBeforeBuild) {
> +        aWidget.onBeforeBuild(aDocument);

This function should be renamed to onBeforeCreated since it never precedes onBuild, but does precede onCreated.

@@ +1971,5 @@
>      widget.disabled = aData.disabled === true;
>  
>      this.wrapWidgetEventHandler("onClick", widget);
>      this.wrapWidgetEventHandler("onCreated", widget);
> +    this.wrapWidgetEventHandler("onBeforeBuild", widget);

Alphabetical ordering here plz?

::: browser/components/customizableui/test/browser_981418-widget-onbeforebuild-handler.js
@@ +7,5 @@
> +
> +// Should be able to add broken view widget
> +add_task(function testAddOnBeforeBuildWidget() {
> +  let viewShownDeferred = Promise.defer();
> +  let onBeforeBuildCalled = false;

*don't forget to update variable names like this*

@@ +55,5 @@
> +      tempPanel.hidePopup();
> +      yield panelHiddenPromise;
> +
> +
> +      CustomizableUI.addWidgetToArea(kWidgetId, CustomizableUI.AREA_PANEL);

nit, can remove one of these blank lines.
Attachment #8388901 - Flags: review?(jaws) → review+
Whiteboard: [Australis:P4]
Landed with nits, updated docs in CustomizableUI.jsm as well as https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm/API-provided_widgets .

remote:   https://hg.mozilla.org/integration/fx-team/rev/fdebf60103c3
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
Summary: CustomizableUI widgets need an onBeforeBuild handler → CustomizableUI type:view and type:button widgets need an onBeforeCreated handler
Backed out because of persistent orange:

remote:   https://hg.mozilla.org/integration/fx-team/rev/ac800841c853
Keywords: addon-compat
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Thank you for doing this. After fixing the previous issues I encountered 
> viewNode is null panelUI.xml:186
when the button is the AREA_PANEL. 

It works fine initially after I move it there but not if I restart Firefox after that.
This wfm on my windows machine where I could reproduce the original error, but I'll push to try in a bit.
Attachment #8389508 - Flags: review?(jaws)
(In reply to 4mr.minj from comment #12)
> Thank you for doing this. After fixing the previous issues I encountered 
> > viewNode is null panelUI.xml:186
> when the button is the AREA_PANEL. 
> 
> It works fine initially after I move it there but not if I restart Firefox
> after that.

I'm not sure what you mean. Can you elaborate in a separate bug (ideally with code that triggers this issue, and a stack trace / STR)? It seems orthogonal to this issue.
Attachment #8388901 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to 4mr.minj from comment #12)
> > Thank you for doing this. After fixing the previous issues I encountered 
> > > viewNode is null panelUI.xml:186
> > when the button is the AREA_PANEL. 
> > 
> > It works fine initially after I move it there but not if I restart Firefox
> > after that.
> 
> I'm not sure what you mean. Can you elaborate in a separate bug (ideally
> with code that triggers this issue, and a stack trace / STR)? It seems
> orthogonal to this issue.

Disregard my post. I appended my view not to #PanelUI-multiView but elsewhere. Hence why got 'view not found'.
I was mislead by an erroneous example on FF add-ons blog.
Upping per discussion on IRC.
Whiteboard: [Australis:P4] → [Australis:P3]
(In reply to :Gijs Kruitbosch from comment #15)
>  https://tbpl.mozilla.org/?tree=Try&rev=cf23978927fb

Try looks great. :-)
Comment on attachment 8389508 [details] [diff] [review]
add onBeforeCreated handler for Australis widgets so add-ons have warning when their widget is created, and fix a race condition in the panelmultiview code that showed up in the automated test,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2727,5 @@
>     * - onBuild(aDoc): Only useful for custom widgets (and required there); a
>     *                  function that will be invoked with the document in which
>     *                  to build a widget. Should return the DOM node that has
>     *                  been constructed.
> +   * - onBeforeCreated(aDoc): Attached to all non-custom widgets; a function

Should we say which functions are optional?
Attachment #8389508 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8389508 [details] [diff] [review]
> add onBeforeCreated handler for Australis widgets so add-ons have warning
> when their widget is created, and fix a race condition in the panelmultiview
> code that showed up in the automated test,
> 
> Review of attachment 8389508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +2727,5 @@
> >     * - onBuild(aDoc): Only useful for custom widgets (and required there); a
> >     *                  function that will be invoked with the document in which
> >     *                  to build a widget. Should return the DOM node that has
> >     *                  been constructed.
> > +   * - onBeforeCreated(aDoc): Attached to all non-custom widgets; a function
> 
> Should we say which functions are optional?

All of them except onBuild are optional, which is only required/useful for custom widgets. So I didn't think it was useful to state it every time. Can you think of some other way to clarify this?
And into the fray once more:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c8a1458bfe7d
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c8a1458bfe7d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Comment on attachment 8389508 [details] [diff] [review]
add onBeforeCreated handler for Australis widgets so add-ons have warning when their widget is created, and fix a race condition in the panelmultiview code that showed up in the automated test,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: add-ons will struggle to dynamically create views for their widgets; small correctness fix to panels when closing them quickly
Testing completed (on m-c, etc.): on m-c, locally, has automated test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8389508 - Flags: approval-mozilla-aurora?
Attachment #8389508 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.