514763 - Implement "cropped" attribute for XUL labels

Status: NEW

(Core :: XUL, enhancement)

Description

[Gabriele Best [:Gabri]]

I think we should implement an attribute (e.g.: cropped) which allows us to determine if the value of a label was cropped or not.

Comment 1

[Gabriele Best [:Gabri]]

Attachment: Trivial patch

Comment 2

[Gabriele Best [:Gabri]]

Attachment: Patch

"cropped" attribute have to be assigned to the main element if the label is an anonymous node.

Comment 3

[Neil Deakin]

Comment on attachment 398865 [details] [diff] [review]
Patch

Unfortunately, attributes can't be changed during painting or layout directly, because mutation listeners could be called which could delete the elements and frames, which could cause a crash.

This would either need to pass false for the last argument(that may cause other problems though) or be done asynchronously. There are a number of other places in the code that does the latter, such as menus, if we wanted to go that route.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FYI, there are helpers classes for safe set/unset attr.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h#1107

Comment 5

[Gabriele Best [:Gabri]]

Attachment: I hope now it's ok

Thanks to Smaug who helped me to make this patch.
I hope that now it works properly :)

Comment 6

[Gabriele Best [:Gabri]]

Attachment: Ops, now it's ok :)

Comment 7

[Gabriele Best [:Gabri]]

Tommoeeow I'll make some tests

Comment 8

[Gabriele Best [:Gabri]]

Err, tomorrow

Comment 9

[Gabriele Best [:Gabri]]

Attachment: Patch + reftests

I added a reftest

Comment 10

[Gabriele Best [:Gabri]]

Attachment: Patch with a better reftest

Comment 11

[Neil Deakin]

Comment on attachment 399457 [details] [diff] [review]
Patch with a better reftest

>+    nsIContent* croppedElement = mContent;
>+    if (mContent->GetBindingParent())
>+        croppedElement = mContent->GetBindingParent();

What is this part for? To set cropped on buttons? This will also end up setting cropped on a parent bindings when a label is used in any binding.

What would happen in a case like the following:

label { max-width: 20px; }
label[cropped=true] { max-width: 500px; }

where having the cropped attribute causes the size to grow such that it no longer needs the cropping, thus reseting the width to 20px, and repeat.

Comment 12

[Gabriele Best [:Gabri]]

One month ago I spoke with Smaug about that and he told me that we could use an asyncronous function to resolve this problem.

I'll post a new patch soon, maybe... :)

Comment 13

[Gabriele Best [:Gabri]]

Attachment: testcase v1

Comment 14

[Gabriele Best [:Gabri]]

If you open this testcase you should not see nothing without the patch.
If you include the patch on your firefox source you should see e green label.

Now, with the problem reported by neil, you'll see a blinking label.

Comment 15

[Gabriele Best [:Gabri]]

Attachment: Patch, v2.0

(In reply to comment #11)
> (From update of attachment 399457 [details] [diff] [review])
> >+    nsIContent* croppedElement = mContent;
> >+    if (mContent->GetBindingParent())
> >+        croppedElement = mContent->GetBindingParent();
> 
> What is this part for? To set cropped on buttons? This will also end up setting
> cropped on a parent bindings when a label is used in any binding.

We could check the tag of the bindingParent.
Have you an idea about that?

> What would happen in a case like the following:
> 
> label { max-width: 20px; }
> label[cropped=true] { max-width: 500px; }
> 
> where having the cropped attribute causes the size to grow such that it no
> longer needs the cropping, thus reseting the width to 20px, and repeat.

I fixed that in this new patch adding a control that checks if the label cropped for more than 3 times per seconds, if yes it doesn't set the attribute for the label.

Comment 16

[Neil Deakin]

Comment on attachment 413903 [details] [diff] [review]
Patch, v2.0

This needs to be updated, although I'm not sure about this idea.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.