Remove the ability to override the node name using XBL.

RESOLVED FIXED in Firefox 67

Status

()

P3
normal
RESOLVED FIXED
a year ago
13 days ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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).
(Assignee)

Updated

a year ago
Depends on: 1450653
(Assignee)

Updated

a year ago
Depends on: 1450654
(Assignee)

Comment 1

a year ago
(FWIW, this is already kinda broken, we apply this super inconsistently)
(Assignee)

Updated

a year ago
Depends on: 1450657
(Assignee)

Updated

a year ago
Depends on: 1450705
(Assignee)

Updated

a year ago
Depends on: 1450721
(Assignee)

Updated

a year ago
Depends on: 1450617
(Assignee)

Updated

a year ago
Depends on: 1451256
Depends on: 1451387
(Assignee)

Updated

a year ago
Depends on: 1451400
Depends on: 1451408
Depends on: 1451410
Priority: -- → P3
(Assignee)

Updated

10 months ago
Depends on: 1463747
Depends on: 1499428
Depends on: 1507704

Comment 2

4 months ago
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?
Flags: needinfo?(emilio)
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 3

4 months ago
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
(Assignee)

Comment 4

4 months ago
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.
Flags: needinfo?(paolo.mozmail)

Comment 5

4 months ago
(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.
Flags: needinfo?(paolo.mozmail)

Updated

4 months ago
Flags: needinfo?(bgrinstead)

Updated

4 months ago
Flags: needinfo?(emilio)
(Assignee)

Comment 6

4 months ago
It will be unused as soon as the dependent bugs land and the [type="menu"] bits
mentioned in comment 2+ are fixed.
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).
Flags: needinfo?(jorgk)

Updated

4 months ago
Depends on: 1509139

Comment 8

4 months ago
Thanks, I filed bug 1509139 for the TB work.
Flags: needinfo?(jorgk)

Updated

4 months ago
Depends on: 1509388

Updated

3 months ago
Depends on: 1512993

Comment 9

3 months ago
(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.
Flags: needinfo?(emilio)

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.

Flags: needinfo?(paolo.mozmail)

Comment 11

2 months ago

(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.

Flags: needinfo?(paolo.mozmail)

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

2 months ago

(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.

(Assignee)

Comment 14

20 days ago

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

Flags: needinfo?(emilio)
Attachment #9048698 - Attachment is obsolete: true
Attachment #9026781 - Attachment description: Bug 1450652 - Remove platform support for display="" in XBL bindings. → Bug 1450652 - Remove platform support for display="" in XBL bindings. r=mats

Comment 16

19 days ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5edf9199407c
Remove platform support for display="" in XBL bindings. r=mats
(Assignee)

Updated

18 days ago
Assignee: nobody → emilio

Comment 17

18 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1533085

Comment 18

15 days ago

(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

15 days ago

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

(Assignee)

Comment 20

15 days ago

(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

13 days ago

What about <binding name=""> ?

(Assignee)

Comment 22

13 days ago

That has nothing to do with this.

You need to log in before you can comment on or make changes to this bug.