1479908 - Either expose <xul:broadcaster> / [observes] behavior to chrome HTML documents, or stop using it in browser.xul

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

We removed single-consumer broadcasters in Bug 1475304, but there are still cases where we use <broadcaster> and [observes] in browser.xul:

- https://searchfox.org/mozilla-central/search?q=%3Cbroadcaster&case=false&regexp=false&path=
- https://searchfox.org/mozilla-central/search?q=observes%3D&case=false&regexp=false&path=.xul

This behavior seems to be controlled by XULDocument (see XULDocument::CheckBroadcasterHookup and friends). We have a few options for supporting this in browser.xhtml:

1) Port this behavior so it will work in HTML documents. This could be done either by moving it onto a base document class, or by moving it onto XULElement. I'm not sure the challenges to doing the latter (perf or otherwise), but it does sort of feel like it 'belongs' at the element level.
2) Implement the same thing in JS via Custom Elements and Mutation Observers
3) Stop relying on this behavior. The basic idea here would be to store state inside of a JS object instead of a DOM node, and have the observing nodes get mutated when the state object changes.

Some thoughts:
* If (1) is easy, that would be a nice starting point.
* (2) seems like it'd be inefficient. We have to look at every DOM node that gets created and every attr change to see if we need to copy initial values and set up listening.
* Lots to figure out with (3), but it's worth investigating since teasing the state apart from the DOM could make things easier to understand, and let us rely on less chrome magic.

Comment 1

[Brian Grinstead [:bgrins]]

The link for `observes=` should be https://searchfox.org/mozilla-central/search?q=observes%3D&path=. The restriction on XUL files only in Comment 0 misses results in browser-*.inc

Comment 2

[Brian Grinstead [:bgrins]]

Also we have `<observes>` elements that let you target a single attribute and copy them up to the parent node: https://searchfox.org/mozilla-central/search?q=%3Cobserves&path=

Comment 3

[Dave Townsend [:mossop]]

If we converted every observes attribute to a child <observes> we could re-implement all of this with custom elements and no need for mutation observers.

Comment 4

[:Gijs (he/him)]

Off-hand (3) seems best to me long-term, because:

- it avoids special magical things new people have to learn
- unlike other parts of XUL, this isn't something with magical platform integration, so we should be able to figure out how to remove it in relatively straightforward fashion.
- the implementation is surprising in some ways (e.g. bug 309953) in that it can't sync the absence of attributes


The other elephant in the room here is that command attributes (ie on buttons, that point to command elements) are implicit broadcaster/observer patterns, at least for "disabled" but maybe for other attributes, I forget exactly how it works. In a way that's a nice semantic thing, ie if I have 5 bits of UI that point to 1 command, I can just set the disabled attribute/property on the command instead of the UI. But that does seem like the kind of thing that you could also implement with JS + custom elements, in that the buttons could essentially subscribe to the command for updates to its state.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #4)
> The other elephant in the room here is that command attributes (ie on
> buttons, that point to command elements) are implicit broadcaster/observer
> patterns, at least for "disabled" but maybe for other attributes, I forget
> exactly how it works. In a way that's a nice semantic thing, ie if I have 5
> bits of UI that point to 1 command, I can just set the disabled
> attribute/property on the command instead of the UI. But that does seem like
> the kind of thing that you could also implement with JS + custom elements,
> in that the buttons could essentially subscribe to the command for updates
> to its state.

I believe (I could be wrong - I'd have to double check) that the [command] magic already "just works" for XUL elements in HTML docs.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Dave Townsend [:mossop] from comment #3)
> If we converted every observes attribute to a child <observes> we could
> re-implement all of this with custom elements and no need for mutation
> observers.

Talked with smaug about this, and unfortunately there's not a way with CE to get attributeChangedCallback on every attribute. You need a statically defined array of attribute names via observedAttributes. So we would need a MutationObserver for the <broadcaster> element in this case.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #4)
> Off-hand (3) seems best to me long-term, because:
> 
> - it avoids special magical things new people have to learn
> - unlike other parts of XUL, this isn't something with magical platform
> integration, so we should be able to figure out how to remove it in
> relatively straightforward fashion.
> - the implementation is surprising in some ways (e.g. bug 309953) in that it
> can't sync the absence of attributes

I agree. We should at least investigate the options for migrating the frontend to not use them, so we can avoid porting the behavior over if possible.

Comment 8

[Dave Townsend [:mossop]]

I'm willing to bet that as MutationObservers are async we'd be breaking some assumptions by trying to use them to emulate broadcasters so I think we should do something like 3. Here's a strawman:

Implement an API importable into a window scope:

    Broadcasters.getBroadcaster(id)
    Broadcasters.registerObserver(id, element, attributes)
    Broadcasters.unregisterObserver(id, element)

getBroadcaster return something that looks a little like an element with setAttribute, removeAttribute, etc. available for calling. First call registers a new broadcaster. This should make it straightforward to rewrite document.getElementById(id) calls to Broadcasters.getBroadcaster(id) calls.

Then we create a custom element for <observer> which on attachment registers for the broadcaster. Then we migrate existing observes attributes to <observes> elements with attributes="*".

The final trick is setting the initial attributes for a broadcaster. The easiest way is probably another custom element for <broadcaster> that simply sets its attributes on attachment.

Thoughts?

Comment 9

[Dave Townsend [:mossop]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > If we converted every observes attribute to a child <observes> we could
> > re-implement all of this with custom elements and no need for mutation
> > observers.
> 
> Talked with smaug about this, and unfortunately there's not a way with CE to
> get attributeChangedCallback on every attribute. You need a statically
> defined array of attribute names via observedAttributes. So we would need a
> MutationObserver for the <broadcaster> element in this case.

It just occurred to me that a custom element can just override the setAttribute, getAttribute and removeAttribute methods and then do whatever it likes, right? That would mean we can make <broadcaster> do everything as a custom element.

Comment 10

[:Paolo Amadini]

Where conversions away from <broadcaster> are not as easy as bug 1480082, I suspect that's because of unneeded code complexity.

Also, I'm not sure if Fluent attributes needs special APIs to be changed, but if they do, just not using broadcasters would avoid the problem in the first place.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #10)
> Also, I'm not sure if Fluent attributes needs special APIs to be changed,
> but if they do, just not using broadcasters would avoid the problem in the
> first place.

Generally, setting the `data-l10n-id` attribute via setAttribute should be enough for Fluent. There are special APIs to get the resolved string value if needed but doing that should be an exception, not the rule.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #10)
> Where conversions away from <broadcaster> are not as easy as bug 1480082, I
> suspect that's because of unneeded code complexity.

Thanks for grabbing that. Switching from `broadcaster.setAttribute("foo")` to `[...observers].forEach(o => o.setAttribute("foo"))` does seem pretty simple in this case.

It seems like a good idea to audit existing [observes] consumers and see if others are that easy. I wonder how far that would get us. The cases where I think it may not be as easy are:

* Dynamically added nodes with [observes], or changed [observes] attributes. Do we ever do this? The current system will automatically sync them up with the broadcaster when it's added. The custom element approach Mossop has mentioned would also be automatic once the <observes> was added. If we have a new JS API it would probably require manual wiring up but there would at least be a centralized place to do that.
* Cases where we use <observes> to select a single attribute for one of the consumers could make the loop more complex (i.e. don't set "foo" if o.id is "bar"). Duplicating that logic around the tree wouldn't be great - but I suppose we could have a shared function to set the state on all DOM nodes. If we do that it starts to look a bit like option 3 :)
* For tests, we currently can check that the broadcaster attr are correct and just assume that means observing nodes are correct. If we expect all callers to properly iterate nodes and set attrs we probably want to expand test coverage to also qSA all nodes.

Comment 13

[Sam Foster [:sfoster] (he/him)]

triage: assigning this p1 as its under active development.

Comment 14

[Dão Gottwald [:dao]]

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #4)
> Off-hand (3) seems best to me long-term, because:
> 
> - it avoids special magical things new people have to learn
> - unlike other parts of XUL, this isn't something with magical platform
> integration, so we should be able to figure out how to remove it in
> relatively straightforward fashion.
> - the implementation is surprising in some ways (e.g. bug 309953) in that it
> can't sync the absence of attributes

All these pros apply to (2) as well. Custom Elements and Mutation Observers are standard DOM, we'd just implement a helper on top of that; nothing really magical about it.

(In reply to (Unavailable until Aug 11) Brian Grinstead [:bgrins] from comment #5)
> I believe (I could be wrong - I'd have to double check) that the [command]
> magic already "just works" for XUL elements in HTML docs.

AFAIK command is basically just an alias for observes...

(In reply to Dave Townsend [:mossop] from comment #8)
> I'm willing to bet that as MutationObservers are async we'd be breaking some
> assumptions by trying to use them to emulate broadcasters

I'm not sure sure about that (updating those attribute asynchronously breaking assumptions).

Comment 15

[:Paolo Amadini]

Yeah, it looks like the "command" attribute uses the same mechanism as the "observes" attribute for propagating attribute changes from other elements. The only difference seems to be that "menuitem" elements handle the "command" attribute in a special way, only propagating some attributes. There is also some magic code for "command" event handling with "command" attributes on non-"command" elements... if that sounds confusing, it totally is :-)

Also, the "command" and "broadcaster" elements are not special - apparently any element ID can be used as the source for the attributes, with the limitation that the element should already exist in the document when the element with the "command" or "observes" attribute is added. The mechanism can also be used recursively, and we have examples of "command" elements which contain an "observes" attribute. If that sounds more confusing, consider that it's probably also buggy.

It doesn't end here! Even more confusingly, we have at least one case where we have _both_ the "command" and "observes" attributes on a single element, and this works, even though by looking at the code it seems only "observes" should be considered:

https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/dom/xul/XULDocument.cpp#2562-2567

I would be more than happy to replace all of this with a simple JavaScript API or just simple loops. I don't think we need Custom Elements, because while they are standard, any time we can avoid a round-trip through the DOM is a performance win, and the simplest thing that works is also the simplest to learn for new developers.

We have far more "command" attributes and "command" elements than broadcasters, and we also plan to get rid of XUL command dispatching. The latter is a distinct concept from "command" events, "command" attributes, and "command" elements, although in practice they are all just a very convoluted way of doing a very simple thing.

Now that we don't have overlays and traditional add-ons anymore, all of this indirection and making things generic is probably unnecessary, and we should start thinking about a simpler way to do things.

In the meantime, I've found what looks like dead code or single-instance broadcasters, and I'll file bugs to simplify some of those.

Comment 16

[:Paolo Amadini]

All dependencies landed. Brian, do you want to morph this for <xul:command> / [command], or use a different bug?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #16)
> All dependencies landed. Brian, do you want to morph this for <xul:command>
> / [command], or use a different bug?

Thanks for taking care of these! I'll file a new bug for commands.