Closed Bug 1111828 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tours, defect)

defect
Not set
minor
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Flags: qe-verify-
Flags: firefox-backlog+
Thanks for the heads up - will make sure we work around this in the page.
Attached file MozReview Request: bz://1111828/MattN (obsolete) —
Attachment #8538261 - Flags: review?(dolske)
/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
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?
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: 2 → 1
Flags: needinfo?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8538261 - Flags: review?(dolske) → review+
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
Attachment #8538261 - Flags: review+ → review?(dolske)
/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
Attachment #8538261 - Flags: review?(mdeboer)
Attachment #8538261 - Flags: review?(gijskruitbosch+bugs)
Attachment #8538261 - Flags: review?(dolske)
/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
(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.
Flags: needinfo?(mdeboer)
Attachment #8538261 - Flags: review?(mdeboer) → review+
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8538261 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
Attachment #8538261 - Flags: review?(gijskruitbosch+bugs)
Attachment #8538261 - Flags: review-
Attachment #8538261 - Flags: review+
/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
Attachment #8538261 - Flags: review?(gijskruitbosch+bugs) → review+
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 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.
Attachment #8538261 - Flags: approval-mozilla-beta+
Attachment #8538261 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e770c9dd2500
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Attachment #8538261 - Attachment is obsolete: true
Attachment #8618928 - Flags: review+
You need to log in before you can comment on or make changes to this bug.