Closed Bug 1450657 Opened 3 years ago Closed 3 years ago

Remove ResolveTag usage from nsMenuPopupFrame.

Categories

(Core :: XUL, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

We never extend xul:tooltip, xul:menupopup or xul:popup, so this can go away.
Comment on attachment 8964260 [details]
Bug 1450657: Remove ResolveTag usage in nsMenuPopupFrame.

https://reviewboard.mozilla.org/r/233000/#review238460

https://searchfox.org/comm-central/source/calendar/base/content/calendar-menus.xml#13 does extend xul:menupopup

Please ping the comm-central folks to let them know this is going away.  Looks like this affects the "task-menupopup" tagname; they can presumably replace this with a "menupopup" with some attribute set.  Assuming this isn't dead code, of course; I see no uses of the "task-menupopup" tagname.
Attachment #8964260 - Flags: review?(bzbarsky) → review+
Read comment 2. I don't think I'm on a rush to land this, it's just general cleanup. So let me know if comm-central would like me to delay landing this a bit. Though per that comment I don't think it's relevant.
Flags: needinfo?(jorgk)
It looks like the bindings that extend task-menupopup (task-priority-menupopup and task-progress-menupopup) do get bound to non-menupopup elements (arrowscrollbox). Does the `extends="xul:menupopup"` also apply to child bindings that extend the xul: tag?
(In reply to Brian Grinstead [:bgrins] from comment #4)
> It looks like the bindings that extend task-menupopup
> (task-priority-menupopup and task-progress-menupopup) do get bound to
> non-menupopup elements (arrowscrollbox). Does the `extends="xul:menupopup"`
> also apply to child bindings that extend the xul: tag?

Yes, it looks through all the base binding chain... It's kinda weird that those are bound to arrowscrollboxes, but that indeed may be affected by this change in comm-central.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > It looks like the bindings that extend task-menupopup
> > (task-priority-menupopup and task-progress-menupopup) do get bound to
> > non-menupopup elements (arrowscrollbox). Does the `extends="xul:menupopup"`
> > also apply to child bindings that extend the xul: tag?
> 
> Yes, it looks through all the base binding chain... It's kinda weird that
> those are bound to arrowscrollboxes, but that indeed may be affected by this
> change in comm-central.

OK good to know. And yes, it is a bit confusing since that tag won't get the normal arrowscrollbox binding. As :bz pointed out it should be able to be modified to use a menupopup instead - or if it's not important that it gets considered a menupopup then task-menupopup could just remove it's [extends].
Richard is the expert in bindings. Calendar, being a non-bootstrapped add-on is non-functional right now, so a little hard to test anything. We might just file a follow-up bug to cover it.
Flags: needinfo?(jorgk) → needinfo?(richard.marti)
Calendar is actually broken because of the extension rework. I see only the element defined in CSS and in the XML file but not in other code, so I think it's okay to land now.
Flags: needinfo?(richard.marti)
> It's kinda weird that those are bound to arrowscrollboxes

Oh, I had missed that the selector there had a combinator.

I don't see how it can  make sense to attach an extends="xul:menupopup" thing to arrowscrollboxes.  But in any case, extends="" does not affect selector matching, and neither arrowscrollbox nor menupopup get boxes by tag name.  The only way to get an nsMenuPopupFrame is to have a -moz-popup display style, which afaict only happens in xul.css for popup/menupopup/panel/tooltip tagnames and in some comm-central stuff for #folderLocationPopup and .autocomplete-result-popup neither of which is in calendar and neither of which applies to these arrowscrollboxes afaict.

So those elements don't have an nsMenuPopupFrame anyway and aren't affected by this patch.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/479b6858ea6c
Remove ResolveTag usage in nsMenuPopupFrame. r=bz
https://hg.mozilla.org/mozilla-central/rev/479b6858ea6c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.