880918 - xul:toolbarbutton should have a type that wraps its label

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

In order to get a xul:label to wrap it's text content, you have to set the textContent of the <label>. Setting the value attribute will not allow the text to wrap.

This normally isn't troublesome, but in the case of <xul:toolbarbutton> there is an anonymous child of a <xul:label> that inherits the toolbarbutton's label attribute and uses that for the value of the <xul:label> element.

This gets messy when dealing with toolbarbuttons that aren't appended to a document, as the anonymous content doesn't exist yet.

If we can create an attribute for <label>, such as textwrap="true", then we can have the <label> inherit the textwrap attribute from <toolbarbutton>, and allow the toolbarbutton to specify if its label should be wrapped.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Dao, what do you think about something like this?

As is, it doesn't work for setting/removing the attribute after the constructor has run, nor for subsequent changes to the label attribute of the <toolbarbutton>.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 760044 [details] [diff] [review]
WIP Patch

It's probably best to add a new binding for the wrapping behavior, one that extends toolbarbutton.xml#toolbarbutton with an adjusted anonymous content tree.

Comment 3

[:Gijs (he/him)]

Talked about this with Jared, going to have a look at this, probably tomorrow.

Comment 4

[:Gijs (he/him)]

Attachment: add a toolbarbutton sub-binding that wraps labels

Dao, I'm not 100% sure this is what you meant, but I think so. I've added a mutation observer to make the label apply when it's changed, or when the anonymous content is added/removed. IME, this seems to work well.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 777078 [details] [diff] [review]
add a toolbarbutton sub-binding that wraps labels

>+      <xul:label class="toolbarbutton-text" crop="right" flex="1"
>+                 xbl:inherits="accesskey,crop"/>

Use xbl:inherits="xbl:text=label" here and get rid of the mutation observer.

Inheriting crop doesn't seem to make sense, as it's not going to work. By the way, is there a plan for handling overly long labels other than having them create labels with an unbound number of lines?

As for inheriting accesskey: Would it actually work and be displayed when the attribute is present?

>+toolbarbutton[textwrap="true"] {
>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-wrapping-label");
>+}

You probably want a type attribute here, otherwise it's ambiguous whether type or textwrap would win.

Comment 6

[:Gijs (he/him)]

Thanks for the quick review! I'll try and iterate on this either today or tomorrow.

(In reply to Dão Gottwald [:dao] from comment #5)
> By the way, is there a plan for handling overly long labels other than having
> them create labels with an unbound number of lines?

I think Jared suggested fading anything past the first two labels using an SVG mask for our own usage in the panel. I don't think we should be doing that in/with the binding though. If you like, I could file a followup bug about restricting the number of lines, but I'm not sure that'd easily be possible.

> As for inheriting accesskey: Would it actually work and be displayed when
> the attribute is present?

Not sure. I'll try to test for the next iteration.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> As for inheriting accesskey: Would it actually work and be displayed when the attribute is present?

No, apparently. The accesskey still works, but it does so even if it the label doesn't inherit it, so I'll remove it.

Comment 8

[:Gijs (he/him)]

Attachment: add a toolbarbutton sub-binding that wraps labels

This should be better.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 777700 [details] [diff] [review]
add a toolbarbutton sub-binding that wraps labels

I'd like Enn to sign off on this as well.

Comment 10

[Neil Deakin]

Comment on attachment 777700 [details] [diff] [review]
add a toolbarbutton sub-binding that wraps labels


> No, apparently. The accesskey still works, but it does so even if it the label doesn't inherit it, so I'll remove it.

While the accesskey will work, it won't be drawn properly unless the label inherits it.


>+toolbarbutton[type="wrapping-label"] {
>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-wrapping-label");
>+}
>+

Just use a shorter value: type="wrap"

Comment 11

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #10)
> Comment on attachment 777700 [details] [diff] [review]
> add a toolbarbutton sub-binding that wraps labels
> 
> 
> > No, apparently. The accesskey still works, but it does so even if it the label doesn't inherit it, so I'll remove it.
> 
> While the accesskey will work, it won't be drawn properly unless the label
> inherits it.

Right, but even inheriting it, in this case, didn't get it to draw properly - presumably because we're using textContent rather than the actual value... I can file a separate bug on this if you like?

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/0b6c881ba74c

Comment 13

[Neil Deakin]

OK, I realized that you also need to have the label use the 'text.xml#label-control' bindings. You can do this by adding a class to the label, in the same way/place that the checkbox uses checkbox-label.

Comment 14

[:Gijs (he/him)]

Attachment: Allow accesskeys to work for wrapped labels.

Like this, I presume? I tested it locally, and it seems to work although I need to still call formatAccessKey on the inner label if the element has been constructed already and I change the accesskey attribute afterwards...

Comment 15

[:Gijs (he/him)]

Thanks!

Pushed: https://hg.mozilla.org/projects/ux/rev/2dbad15d775e

Comment 16

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/0b6c881ba74c
https://hg.mozilla.org/mozilla-central/rev/2dbad15d775e