1111828 - showMenu("loop") shouldn't open the menu panel if the toolbarbutton is in the palette

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Matthew N. [:MattN]]

Apparently the node can still exist but not be in the document when the button is in the customization palette. The following code that we have isn't enough.

>       if (!toolbarButton || !toolbarButton.node) {
>         return;
>       }

The page should be able to workaround this by checking availableTargets before calling showMenu

Comment 1

[Alex Gibson [:agibson]]

Thanks for the heads up - will make sure we work around this in the page.

Comment 2

[Matthew N. [:MattN]]

Attachment: MozReview Request: bz://1111828/MattN

Comment 3

[Matthew N. [:MattN]]

/r/1581 - Bug 1111828 - UITour: Don't allow opening the Loop menu if the toolbarbutton isn't in the document. r=dolske

Pull down this commit:

hg pull review -r c9491bfbe37020828c3dd241a403ddd2d37e0177

Comment 4

[Matthew N. [:MattN]]

This is a simple workaround. Gijs/Mike, I thought that node was null in the past. Do you know if the current beheaviour is expected or what changed?

Comment 5

[Justin Dolske [:Dolske]]

https://reviewboard.mozilla.org/r/1579/#review963

Ship It!

Comment 6

[Matthew N. [:MattN]]

I guess the question is whether onBuild should be called for custom widgets in the palette. https://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#925

Comment 7

[Matthew N. [:MattN]]

/r/1581 - Bug 1111828 - UITour: Don't allow opening the Loop menu if the toolbarbutton has no placement. r=dolske

Pull down this commit:

hg pull review -r c03f099cb54fca081b223c893841479fe6c817bd

Comment 8

[Matthew N. [:MattN]]

/r/1581 - Bug 1111828 - UITour: Don't allow opening the Loop menu if the toolbarbutton has no placement. r=dolske

Pull down this commit:

hg pull review -r c03f099cb54fca081b223c893841479fe6c817bd

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] from comment #4)
> This is a simple workaround. Gijs/Mike, I thought that node was null in the
> past. Do you know if the current beheaviour is expected or what changed?

Well, what I *think* is happening here is that the node is constructed in `populatePalette()` and when the widget wrapper is destructed upon exiting customize mode, the widget node drifts away in space without a parent.

I think your workaround is valid.

Comment 10

[Mike de Boer [:mikedeboer]]

https://reviewboard.mozilla.org/r/1579/#review977

Ship It!

Comment 11

[:Gijs (he/him)]

The placement is null, and that's really what you should be checking:

CustomizableUI.getPlacementOfWidget

In particular, the current code will fail if the node is in the menu panel but the panel hasn't been opened yet since the node will not have been constructed yet.

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> The placement is null, and that's really what you should be checking:
> 
> CustomizableUI.getPlacementOfWidget
> 
> In particular, the current code will fail if the node is in the menu panel
> but the panel hasn't been opened yet since the node will not have been
> constructed yet.

I did also think that the unused-but-already-created nodes get added as kids of the toolbarpalette, meaning the node's parentNode could be non-null, too... so I would be/am surprised if/that your current patch works to detect that failure mode.

Comment 13

[Matthew N. [:MattN]]

/r/1581 - Bug 1111828 - UITour: Don't allow opening the Loop menu if the toolbarbutton has no placement. r=Gijs

Pull down this commit:

hg pull review -r 43b3ef93841c357c1472af9b0e47d390414ac8f1

Comment 14

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/1579/#review1003

Ironically, toolbarButton is a single widget wrapper - the group widget wrapper would have the placement info anyway. We should probably just expose said info on the single wrapper, but then, that'd be a separate bug and not upliftable. Also not sure if you have a group widget wrapper lying around somewhere that you could use. In its absence, this will do.

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/e770c9dd2500

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 8538261 [details]
MozReview Request: bz://1111828/MattN

[Triage Comment]

Needed for Fx35 Hello tour. No unusual risk, and is limited specifically to the Hello tour.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be7097c9f060

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e770c9dd2500

Comment 19

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/afe0f1e8e520

Comment 21

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1111828 - UITour: Don't allow opening the Loop menu if the toolbarbutton has no placement. r=Gijs