1450652 - Remove the ability to override the node name using XBL.

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

This is intended to remove support for changing element tag names using XBL and removing nsBindingManager::ResolveTag and the relevant ilk.

The relevant bits can be seen grepping for:

  extends="xul:

(There's also extends="html: and svg, but those are only in tests).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

(FWIW, this is already kinda broken, we apply this super inconsistently)

Comment 2

[:Paolo Amadini]

After the dependencies are fixed, it seems that the only elements left are buttons, see the selectors here:

  https://bgrins.github.io/xbl-analysis/tree/#display

Basically, it seems that button[type="menu"] and toolbarbutton[type="menu"] should get a menu frame instead of a button frame. Emilio, is this something that can be implemented in nsCSSFrameConstructor::FindXULTagData?

Brian, do you think we need to handle cases where this attribute changes, maybe because we set it after adding the element to the document? In this case, Emilio, is this something that is already handled, or would it need to be handled specially after you remove the platform code for "display" that you mentioned in comment 0?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

It can be done there with a bit of effort to reframe when that attribute changes[1]. I'm happy to write that patch.

[1]: https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/xul/nsXULElement.cpp#1180

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That being said, other than in tests there doesn't seem to be that many: https://searchfox.org/mozilla-central/search?q=button.*type%3D%22menu&case=false&regexp=true&path=*.xul

Are those something we could replace for actual menus easily? If we could avoid the special-case it'd be really neat.

Comment 5

[:Paolo Amadini]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> Are those something we could replace for actual menus easily? If we could
> avoid the special-case it'd be really neat.

This would also simplify some of the conversions to Custom Elements, so I'll look into this next.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450652 - Remove platform support for display="" in XBL bindings. r=mats

It will be unused as soon as the dependent bugs land and the [type="menu"] bits
mentioned in comment 2+ are fixed.

Comment 7

[Brian Grinstead [:bgrins]]

Jorg, if TB has any bindings that use the [display] / [extends] attribute on <binding> they will need to be migrated away from using it after this patch (which depends on a bit more work being done on the m-c side before it can land).

It looks like this is used in some TB folders: https://searchfox.org/comm-central/search?q=display%3D&path=.xml.

There are bugs blocking this one showing examples of migration - sometimes they can just be removed directly if it's just box/hbox or if it's an element we've already covered on the layout side (like toolbarpaletteitem was covered in Bug 1451256).

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks, I filed bug 1509139 for the TB work.

Comment 9

[:Paolo Amadini]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
> It can be done there with a bit of effort to reframe when that attribute
> changes[1]. I'm happy to write that patch.
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> 55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/xul/nsXULElement.cpp#1180

Emilio, would you be able to write the patch for this in bug 1512993? After experimenting with removing the menu frame altogether from menu buttons, we're thinking of keeping it to reduce code duplication. The toolbarbutton[type="menu"] binding removals in my previous patch would still apply, since switching based on the "type" attribute would be handled without relying on XBL. (We do have code that changes the attribute at runtime, for example this is done in the new tab button when containers are enabled or disabled.)

After this, bug 1507704 is probably the only remaining special case to get rid of before we can land this bug.

Comment 10

[Brian Grinstead [:bgrins]]

Paolo, I still see a couple of display="xul:button" in https://searchfox.org/mozilla-central/search?q=display%3D%22&path=xml (for the button and toolbarbutton bindings). Can those be removed as well, or is this related somehow to Bug 1512993? I don't see those instances being removed in the patch over in that bug.

Comment 11

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #10)

is this related somehow to Bug 1512993?

Yes, that bug is about either removing the display="xul:menu" variants, which Neil didn't recommend to avoid code duplication, or implementing dynamic frame switching when the "type" attribute changes as Emilio mentioned. The second solution would probably get rid of the "display" directive in the same patch.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Other than bug 1512993, do we know what we are going to do with display="xul:button"?

I looked at it a bit and it looks like we only map <xul:button> tag to XULButtonBoxFrame. There were no signs of references to <xul:toolbarbutton> that would map the tag to the frame.

https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/layout/base/nsCSSFrameConstructor.cpp#3960

• For the button binding, does the display="xul:button" value is actually redundant?
• For the toolbarbutton binding, does a simple copy-paste fix of the line above makes the display="xul:button" redundant?

Comment 13

[:Paolo Amadini]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)

• For the button binding, does the display="xul:button" value is actually redundant?

Yes, since the "button" binding is now only applied to the "button" element.

• For the toolbarbutton binding, does a simple copy-paste fix of the line above makes the display="xul:button" redundant?

Yes, although as I understand it both lines would need to change as soon as the frame type becomes conditional on the "type" attribute. Emilio knows what needs to happen here in that case.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I think it should be done in that bug, terribly sorry for the lag.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450652 - Remove platform support for display="" in XBL bindings. r=mats

https://phabricator.services.mozilla.com/D12586

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5edf9199407c
Remove platform support for display="" in XBL bindings. r=mats

Comment 17

[Natalia Csoregi [:nataliaCs]]

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

Comment 18

[:aceman]

(In reply to (Unavailable until March 11) Brian Grinstead [:bgrins] from comment #7)

Jorg, if TB has any bindings that use the [display] / [extends] attribute on
<binding> they will need to be migrated away from using it after this patch
(which depends on a bit more work being done on the m-c side before it can
land).

Why also 'extends'? Surely not all of them. m-c also uses a few of them.
So which cases of 'extends' (as in extending a XBL binding) and what is the replacement?

Comment 19

[:aceman]

If only extends="xul:*" is meant, then that's fine.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to :aceman from comment #19)

If only extends="xul:*" is meant, then that's fine/

Yes, only extends="xul:", which is effectively just display="xul:".

Comment 21

[:aceman]

What about <binding name=""> ?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

That has nothing to do with this.