xul:toolbarbutton should have a type that wraps its label

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8][Australis:P2])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Created attachment 760044 [details] [diff] [review]
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>.
Attachment #760044 - Flags: feedback?(dao)
Whiteboard: [Australis:M7]
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.
Attachment #760044 - Flags: feedback?(dao) → feedback-
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
Talked about this with Jared, going to have a look at this, probably tomorrow.
Assignee: jaws → gijskruitbosch+bugs
Created attachment 777078 [details] [diff] [review]
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.
Attachment #777078 - Flags: review?(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.
Attachment #777078 - Flags: review?(dao) → review-
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.
(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.
Created attachment 777700 [details] [diff] [review]
add a toolbarbutton sub-binding that wraps labels

This should be better.
Attachment #777700 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #777078 - Attachment is obsolete: true
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.
Attachment #777700 - Flags: review?(enndeakin)
Attachment #777700 - Flags: review?(dao)
Attachment #777700 - Flags: review+

Updated

5 years ago
Attachment #760044 - Attachment is obsolete: true
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"
Attachment #777700 - Flags: review?(enndeakin) → review+
(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?
https://hg.mozilla.org/projects/ux/rev/0b6c881ba74c
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M8][Australis:P2][fixed-in-ux]
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.
Created attachment 778467 [details] [diff] [review]
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...
Attachment #778467 - Flags: review?(enndeakin)
(Assignee)

Updated

5 years ago
Whiteboard: [Australis:M8][Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2]
(Assignee)

Updated

5 years ago
Attachment #777700 - Flags: checkin+
Attachment #778467 - Flags: review?(enndeakin) → review+
Thanks!

Pushed: https://hg.mozilla.org/projects/ux/rev/2dbad15d775e
Summary: xul:label should have a textwrap attribute to opt-in to text wrapping of the label value → xul:toolbarbutton should have a type that wraps its label
Whiteboard: [Australis:M8][Australis:P2] → [Australis:M8][Australis:P2][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/0b6c881ba74c
https://hg.mozilla.org/mozilla-central/rev/2dbad15d775e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P2][fixed-in-ux] → [Australis:M8][Australis:P2]
Target Milestone: --- → mozilla28
(Assignee)

Updated

5 years ago
Depends on: 942454
You need to log in before you can comment on or make changes to this bug.