869086 - Make textbox.xml resilient against the changes in bug 653881

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

One of the changes coming in bug 653881 is that the XBL code will no longer remove <children> elements from the XBL binding's DOM tree. For nested bindings that touch the binding's DOM, this causes elements to move around. Currently textbox.xml simply uses this.parentNode.firstChild and breaks with the change, but is also fragile in the face of any manual changes.

This patch makes things work in both the new and old worlds.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #0)
> One of the changes coming in bug 653881 is that the XBL code will no longer
> remove <children> elements from the XBL binding's DOM tree.

That seems pretty counter-intuitive, from a user-of-XBL perspective. Is this really the only thing that change breaks? Is it easy to avoid? I guess we can discuss that in that bug.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> That seems pretty counter-intuitive, from a user-of-XBL perspective. Is this
> really the only thing that change breaks? Is it easy to avoid? I guess we
> can discuss that in that bug.

I'm curious why you find this counter-intuitive. Why would you expect <children> elements to disappear (note: they're not replaced with anything -- there's just a hole there where it used to be)? There are probably more bindings that will need to be fixed. We really want to move to this model though because it's saner and closer to what Web Components specifies. Also, this is a big first step in the ability to dynamically add and remove <children> elements in XBL bindings.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 745965 [details] [diff] [review]
Proposed fix

Can you instead use:

document.getAnonymousElementByAttribute(this.parentNode, "anonid", "input")?
or:
this.parentNode.getElementsByAttribute("class", "textbox-input-box")?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> I'm curious why you find this counter-intuitive. Why would you expect
> <children> elements to disappear (note: they're not replaced with anything
> -- there's just a hole there where it used to be)?

I would expect them to be replaced with children of the bound node, or nothing if there are no such children. <children> on its own is not a useful element, its only use is in specifying the insertion point for the child elements.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> I would expect them to be replaced with children of the bound node, or
> nothing if there are no such children. <children> on its own is not a useful

I assumed that's what happened as well, but then this code would have been broken anyway because inserting one or more elements through the <children> in question would have changed the layout of the element. I could definitely imagine us adding an API: getDistributedNodes to the children element.

> element, its only use is in specifying the insertion point for the child
> elements.

No argument here. I still think that it's more awkward to have nodes that appear in the source disappear from the DOM.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> I assumed that's what happened as well, but then this code would have been
> broken anyway because inserting one or more elements through the <children>
> in question would have changed the layout of the element.

This was a reasonably safe assumption for this code to make. this.parentNode refers to the bound <xul:hbox class="textbox-input-box">, and it's quite unlikely that anyone would have given that element (hardcoded in the textbox binding's anonymous content) any other children (apart from the <html:input>). This is an "internal" binding that's not meant to be used independently from the textbox binding.

> No argument here. I still think that it's more awkward to have nodes that
> appear in the source disappear from the DOM.

I disagree; <children> nodes are known to be "magical", and so making them behave like normal DOM nodes is actually more confusing, IMO.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> I disagree; <children> nodes are known to be "magical", and so making them
> behave like normal DOM nodes is actually more confusing, IMO.

It is not a good thing that XBL developers have to know that <children> are these magical entities that disappear from the DOM without notice. In addition, this magic limits the features of XBL (in particular allowing dynamic changes to the binding), adds additional complexity (and quite a lot of it) to the implementation, for no benefit to the end developer.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> It is not a good thing that XBL developers have to know that <children> are
> these magical entities that disappear from the DOM without notice.

The whole purpose of XBL is to be "magical", by allowing you to create a template for what's actually in the DOM. It doesn't "disappear from the DOM without notice", it's just never in the DOM of the bound node. There's a difference between the binding definition DOM and the bound node's DOM, and in practice that's not been a real source of author confusion. So I really don't buy any of the "it's confusing" arguments, but I guess that leaves the "it's complicated to implement" arguments, which I guess I'll have to take your word for.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #0)
> Currently textbox.xml simply uses this.parentNode.firstChild and breaks with
> the change, but is also fragile in the face of any manual changes.

How did you notice the breakage? Do we have test coverage for this?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 745986 [details] [diff] [review]
Proposed fix v2

r=me assuming this works without your XBL patches (getElementsByAttribute with "anonid" is unusual, typically you need to use getAnonymousElementByAttribute for anonymous nodes. but maybe something is different about this particular case?)

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> How did you notice the breakage? Do we have test coverage for this?

In my build with the patches in bug 653881 right clicking in the search box throws a JS exception. I don't know how to test for this given that the old and new code are correct in current builds and I don't know how to exercise the testcase with only the new code.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> r=me assuming this works without your XBL patches (getElementsByAttribute
> with "anonid" is unusual, typically you need to use
> getAnonymousElementByAttribute for anonymous nodes. but maybe something is
> different about this particular case?)

I'm replacing a parentNode.firstChild, so I am actually looking for a direct child of the parentNode, not anonymous content.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/775ab8588cec

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/775ab8588cec

Comment 15

[[e-mail]]

(In reply to Gavin Sharp from comment #9)
> (In reply to Blake Kaplan from comment #8)
> > It is not a good thing that XBL developers have to know that <children> are
> > these magical entities that disappear from the DOM without notice.
> 
> The whole purpose of XBL is to be "magical", by allowing you to create a
> template for what's actually in the DOM. It doesn't "disappear from the DOM
> without notice", it's just never in the DOM of the bound node. There's a
> difference between the binding definition DOM and the bound node's DOM, and
> in practice that's not been a real source of author confusion. So I really
> don't buy any of the "it's confusing" arguments, but I guess that leaves the
> "it's complicated to implement" arguments, which I guess I'll have to take
> your word for.

For posterity, here's an example of the magic that it looks like that I will have to remove in bug 902312 because of bug 653881.

<tabmail> has some explicit children. Its XBL binding expects one to be a <tabpanels> element while the rest get added as children of a <tabs> element.
The regular <tabs> element doesn't expect these children so there's a derived binding with new content that includes an insertion point for them.
With the old setup, the <tabs> element doesn't even know that the children are there - its child nodes are all <tab> elements. The explicit children of the <tabmail> magically end up in the derived binding's insertion point.
But with the new setup, the <tabs> element gets to see the insertion point from <tabmail>'s binding, which confuses the hell out of it.
This probably only worked before because the two sets of elements were completely separate and did not interact with each other in any way.