891926 - destroyWidget should actually get viewNode before trying to use it

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

http://hg.mozilla.org/projects/ux/file/fa7675b67853/browser/components/customizableui/src/CustomizableUI.jsm#l1460

  1460   destroyWidget: function(aWidgetId) {
  1461     let widget = gPalette.get(aWidgetId);
  1462     if (!widget) {
  1463       return;
  1464     }
  1465 
  1466     // This will not remove the widget from gPlacements - we want to keep the
  1467     // setting so the widget gets put back in it's old position if/when it
  1468     // returns.
  1469 
  1470     let area = widget.currentArea;
  1471     if (area) {
  1472       let buildArea = gBuildAreas.get(area);
  1473       for (let buildNode of buildArea) {
  1474         let widgetNode = buildNode.ownerDocument.getElementById(aWidgetId);
  1475         if (widgetNode) {
  1476           widgetNode.parentNode.removeChild(widgetNode);
  1477         }
  1478         for (let eventName of kSubviewEvents) {
  1479           let handler = "on" + eventName;
  1480           if (typeof widget[handler] == "function") {
  1481             viewNode.removeEventListener(eventName, widget[handler], false);
  1482           }
  1483         }
  1484       }
  1485     }


Been there for a few months now. Let's not ship that.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 773343 [details] [diff] [review]
Patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1474,5 @@
>          if (widgetNode) {
>            widgetNode.parentNode.removeChild(widgetNode);
>          }
> +        let viewNode = widget.type == "view" &&
> +                       buildNode.ownerDocument.getElementById(widget.viewId);

Nit: I prefer to avoid this construct - it makes it non-obvious what it's doing. Though more verbose, the following makes it more obvious IMO:
  if (widget.type == "view") {
    let viewNode = ...;
    ...

But, that may just simply be my own personal preference.

Comment 3

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #2)
> Comment on attachment 773343 [details] [diff] [review]
> Patch
> 
> Review of attachment 773343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +1474,5 @@
> >          if (widgetNode) {
> >            widgetNode.parentNode.removeChild(widgetNode);
> >          }
> > +        let viewNode = widget.type == "view" &&
> > +                       buildNode.ownerDocument.getElementById(widget.viewId);
> 
> Nit: I prefer to avoid this construct - it makes it non-obvious what it's
> doing. Though more verbose, the following makes it more obvious IMO:
>   if (widget.type == "view") {
>     let viewNode = ...;
>     ...
> 
> But, that may just simply be my own personal preference.

I think that's a fair point. Changed, and pushed: https://hg.mozilla.org/projects/ux/rev/5a294331b6b5 .

Comment 4

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/5a294331b6b5