872317 - Download indicator toolbar button depends on an odd XBL quirk

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch

One of the odd quirks of the current XBL code is that if a binding is attached to a node that has an explicit child that doesn't match any of the binding's <children> elements, we still apply the binding, but ignoring its anonymous content. As a concrete example, given a binding:
<binding id="b">
  <content>Foo</content>
  <implementation><property name="prop" onget="return 42" /></implementation>
</binding>

applying "b" to an element:
<element id="e" style="-moz-binding: url(#b)">Bar</element>

Results in the rendering: "Bar", even though document.getElementById('e').prop === 42.

The patch in bug 653881 is getting rid of this behavior in favor of always applying the anonymous content for the binding. The download indicator toolbar button relies on this behavior by applying "toolbarbutton" (<children includes="observes|template|menupopup|panel|tooltip"/>) to a <toolbarbutton ...><stack>.

I'm curious to know whether this quirk was used explicitly here or if this was a case of "oh, it happens to work" without understanding the deeper mysteries of why. The patch (as you can see) is trivial, just creating an explicit binding that slurps up all of the children and extends the original binding to preserve its behavior and styles.

Also note: this patch works equally well on the current XBL code as it does with the patch in bug 653881 applied.

Comment 1

[Marco Bonardo [:mak]]

So, me, paolo and matteo discussed this briefly in person since we are at a conference.

First, we have a question: with you xbl change, would our content be ignored/trashed since it doesn't match includes, or both our content and anonymous content would be added. Off-hand looks like adding both makes includes lose some of its usefulness.

Second, toolbarbutton has already various types, like checkbox, menu, menu-button... adding a custom type would disallow mixing up these, so you could not have a type menu or type checkbox with custom defined content. Unless you also add menu-custom, checkbox-custom... that may end up being a mess.

A possibility may be to allow a way for the existing toolbarbutton to get custom contents, for example adding some generic container to includes and an attribute to hide anonymous contents. This would allow to keep the existing types (and their styling/implementation) while redefining contents completely. Though it's a bit onerous for the implementer to define both the attribute and the very special container of his contents...
Another alternative would require to have a "-customcontents" version of each type (either by having a menu-customcontents type or a customcontents attribute.
Or, if we feel like it's not worth to spend time in figuring a global toolkit fix, we may just add a special binding just for the downloads button, that would not have any unexpected consequences on other consumers.

These are just ideas, we'd like to bring on the discussion to figure together the right fix to take here.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Marco Bonardo [:mak] from comment #1)
> First, we have a question: with you xbl change, would our content be
> ignored/trashed since it doesn't match includes, or both our content and
> anonymous content would be added. Off-hand looks like adding both makes
> includes lose some of its usefulness.

With the changes, the content that doesn't match will not appear (it will still be in the document, it simply won't be rendered). Any content that *does* match will still be rendered alongside the anonymous content in the binding. I don't understand why this reduces the usefulness. You can still imitate the old behavior with script (or maybe even just a well-placed stack?).

> Second, toolbarbutton has already various types, like checkbox, menu,
> menu-button... adding a custom type would disallow mixing up these, so you
> could not have a type menu or type checkbox with custom defined content.
> Unless you also add menu-custom, checkbox-custom... that may end up being a
> mess.

Sure, if that happens, then clearly there'll need to be another solution here. However, in our testing with the patch in bug 653881 shows that, at least in Firefox's UI, this is pretty rare. So far, we've found exactly two cases of this (wchen has been investigating the other case) in Firefox itself and two cases in tests (one of the tests was a fuzz bug and for the other, it appeared that using this behavior was a mistake on the part of the author).

> A possibility may be to allow a way for the existing toolbarbutton to get
> custom contents, for example adding some generic container to includes and
> an attribute to hide anonymous contents. This would allow to keep the

I'm sketching this out right now and it looks really doable. I don't know if it's worth using that approach here.

> existing types (and their styling/implementation) while redefining contents
> completely. Though it's a bit onerous for the implementer to define both the
> attribute and the very special container of his contents...

My impression is that for anything outside of really simple bindings, the implementor is going to have to deal with this anyway -- if there's a method that relies on e.g. some anonymous element having a specific ID, somebody is going to have to make sure the code deals with that ID not existing (the ID might not even make sense with the new rendering).

> Another alternative would require to have a "-customcontents" version of
> each type (either by having a menu-customcontents type or a customcontents
> attribute.

This sounds really bad, but I don't think it is in practice. I just don't see this pattern being used very much.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proof of concept

This patch mostly works, although the width of the download indicator is slightly smaller than a normal button. My CSS-fu isn't quite strong enough to debug why, but an approach like this is certainly doable if the need is there.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> With the changes, the content that doesn't match will not appear (it will
> still be in the document, it simply won't be rendered). Any content that
> *does* match will still be rendered alongside the anonymous content in the
> binding. I don't understand why this reduces the usefulness.

I meant that having an "includes" and then ignoring it by showing any contents, would have reduced the usefulness.  But that's not the case, so it's fine.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> Created attachment 750158 [details] [diff] [review]
> Proof of concept
> 
> This patch mostly works, although the width of the download indicator is
> slightly smaller than a normal button. My CSS-fu isn't quite strong enough
> to debug why, but an approach like this is certainly doable if the need is
> there.

I think that many css rules directly refer to .toolbarbutton-icon and .toolbarbutton-text using the child selector, that may explain why some rules don't apply with this solution. It's indeed quite compatibility breaking, so I don't think we can do that.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 750158 [details] [diff] [review]
Proof of concept

per comment 5, I don't think changing the hierarchy of .toolbarbutton-icon and .toolbarbutton-text is a good idea...
It could be modified to add a special "default-content" class to those elements and queryselector those, though sounds like more pain than the benefit, as you said this is a rare setup.

Comment 7

[Marco Bonardo [:mak]]

So, what if we avoid trying to find a generic solution until it's actually needed by more than one consumer, and here we just add a special binding for the downloads button in browser/ ?
that should be simple and won't have consequences on other consumers.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch v1.1

So, something like this?

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 755040 [details] [diff] [review]
Proposed patch v1.1

Review of attachment 755040 [details] [diff] [review]:
-----------------------------------------------------------------

yes, that's the right idea, I'd be even more specific though, so a specific binding just for #downloads-indicator

::: browser/components/downloads/content/download.xml
@@ +106,5 @@
>  
>      </content>
>    </binding>
> +
> +  <binding id="download-toolbarbutton-custom"

I think the -custom part is not really useful, download-toolbarbutton is specific enough.

::: browser/components/downloads/content/indicatorOverlay.xul
@@ +36,5 @@
>                     ondragover="DownloadsIndicatorView.onDragOver(event);"
>                     ondragenter="DownloadsIndicatorView.onDragOver(event);"
>                     ondragleave="DownloadsIndicatorView.onDragLeave(event);"
> +                   skipintoolbarset="true"
> +                   type="custom-button">

I don't think custom-button is needed, since this has an id we can just use a #downloads-indicator {} rule and I'd put such rule in browser/base/content/browser.css

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.2

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 755596 [details] [diff] [review]
Patch v1.2

Review of attachment 755596 [details] [diff] [review]:
-----------------------------------------------------------------

very tiny fix!

::: browser/components/downloads/content/download.xml
@@ +106,5 @@
>  
>      </content>
>    </binding>
> +
> +  <binding id="download-toolbarbutton"

very-nit: I'd prefer downloads-toolbarbutton (plural)

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdea148283a3

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cdea148283a3

Comment 14

[[e-mail]]

Comment on attachment 750158 [details] [diff] [review]
Proof of concept

>     <content>
>       <children includes="observes|template|menupopup|panel|tooltip"/>
>-      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
>-      <xul:label class="toolbarbutton-text" crop="right" flex="1"
>-                 xbl:inherits="value=label,accesskey,crop"/>
>+      <xul:box anonid="default-content">
>+        <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
>+        <xul:label class="toolbarbutton-text" crop="right" flex="1"
>+                   xbl:inherits="value=label,accesskey,crop"/>
>+      </xul:box>
>+      <children />
>     </content>
I stumbled over this while looking at another "regression" from bug 653881.
Under XBL 1, the correct technique for this would have been something like this:
<content>
  <children includes="observes|template|menupopup|panel|tooltip"/>
  <children>
      <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label"/>
      <xul:label class="toolbarbutton-text" crop="right" flex="1"
                 xbl:inherits="value=label,accesskey,crop"/>
   </children>
</content>
I don't know whether this would work the same in XBL 2.
P.S. You don't need a space before /> in XML. It was only invented to make XHTML parse as HTML.