128428 - Implement getComputedStyle for directional, list, and cursor properties

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

This covers "direction", "unicode-bidi", "list-style-position",
"list-style-type", and "cursor".

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed patch v1.0

Implement the 5 properties and fix a few other issues:	We weren't displaying
proper values for display: -moz-* styles since we were using a predefined value
list as opposed to looking up the property's string from its keyword table in
nsCSSProps.  Also switches to CallQueryInterface over QueryInterface.

the GetDirection code was done by bz, and that has r=caillon.  The rest still
needs review so bz, would you do the honors?  Not sure if this applies cleanly
to CVS as I've got other outstanding patches which aren't checked in yet, but
if you need a diff that will apply, email me and I'll send one out to you.

Comment 2

[Boris Zbarsky [:bzbarsky]]

+  if (list && list->mListStyleType != NS_STYLE_LIST_STYLE_NONE) {
+  }
+  else {
+    val->SetString(NS_LITERAL_STRING("none"));
+  }

The default value of list-style-type is "disc".  So you want to handle !list and
list->mListStyleType == NS_STYLE_LIST_STYLE_NONE separately here.

The code in GetUnicodeBidi is nice and clever but I think I would prefer that al
that stuff be in the #ifdef with a lone
|val->SetString(NS_LITERAL_STRING("normal"));| in an #else.  Also, please use:

#ifdef IBMBIDI
 stuff
#else // IBMBIDI
 stuff
#endif // IBMBIDI

so it's clear what the #else and #endif refer to.

> +    if (!ui->mCursorImage.IsEmpty()) {
> +      val->SetString(ui->mCursorImage);

We actually have this problem elsewhere... We do this wrong for
background-image, eg.  What we really want here is:

val->SetString(NS_LITERAL_STRING("url(")+ui->mCursorImage+NS_LITERAL_STRING(")"));

Should we just add a SetURL to nsROCCSSPrimitiveValue so that callers don't have
to bother with that stuff?  Any other places where we screw this up?

The rest looks good...

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Changes to nsROCSSPrimitiveValue

Implement SetURI(), and make necessary modifications to GetCssText() and
GetStringValue()

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.1

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 72029 [details] [diff] [review]
Proposed patch v1.0

The latest patch fixes the silly mistake with the list-style-type property:
Special-cases for NS_STYLE_LIST_STYLE_BASIC (whatever that is).  Also uses
val->SetURL() where needed for the three properties we have implemented I could
think of which use URIs (see attachment 72211 [details] [diff] [review] for the SetURI() method).  And
fix the #ifdef issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 72211 [details] [diff] [review]
Changes to nsROCSSPrimitiveValue

>+        tmpStr.Append(NS_LITERAL_STRING("url("));
>+        tmpStr.Append(mString);
>+        tmpStr.Append(NS_LITERAL_STRING(")"));

tmpStr.Assign(NS_LITERAL_STRING("url(") + mString + NS_LITERAL_STRING(")"));

>+      aReturn.Append(NS_LITERAL_STRING("url("));
>+      aReturn.Append(mString);
>+      aReturn.Append(NS_LITERAL_STRING(")"));

same thing here.  Here you _definitely_ want to be using Assign, 
not Append, in case the string passed in is not empty.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 72214 [details] [diff] [review]
Patch v1.1

r=bzbarsky

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 72211 [details] [diff] [review]
Changes to nsROCSSPrimitiveValue

>>+        tmpStr.Append(NS_LITERAL_STRING("url("));
>>+        tmpStr.Append(mString);
>>+        tmpStr.Append(NS_LITERAL_STRING(")"));

>tmpStr.Assign(NS_LITERAL_STRING("url(") + mString + NS_LITERAL_STRING(")"));

I'm going to leave this alone because tmpStr is a local nsAutoString which gets
Truncated before we do anything with it.  Every other case in the switch uses
Append() here so this is fine.

>+      aReturn.Append(NS_LITERAL_STRING("url("));
>+      aReturn.Append(mString);
>+      aReturn.Append(NS_LITERAL_STRING(")"));

>same thing here.  Here you _definitely_ want to be using Assign, 
>not Append, in case the string passed in is not empty.

Yep you're right, though I don't really like having it all in one statement. 
It looks confusing.  Can I just change the first part of that to Assign and
leave the two other Append()s alone?  It would look a lot cleaner, IMO.  Or I
could |aReturn.Truncate();| as the first statement in the function and leave
the Appends alone...

Comment 9

[Boris Zbarsky [:bzbarsky]]

The point here is that a single call to Assign with a concatenation is much
faster than three separate calls to Append, especially if the URL is longer than
76 characters long....

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Changes v1.1 to nsROCSSPrimitiveValue

Well since you put it that way...  :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 72263 [details] [diff] [review]
Changes v1.1 to nsROCSSPrimitiveValue

r=bzbarsky.  ask marc or jst for sr.  :)

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.2

Okay, since 126319 was checked in I can finally diff against CVS completely now
and just use one patch file.

Changes from attachments 72214:
 - Renamed GetBehavior() --> GetBinding() as the property is called
-moz-binding
 - Changed SetString() --> SetURI() in GetBinding()
 - Made GetBinding() return "none" instead of "".

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

excellent.  r=bzbarsky

Comment 14

[Johnny Stenback (:jst)]

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

sr=jst

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.2.1

This patch is the same as v1.2 (which got bit-rotted by 129469) except I fixed
a val->QI which I missed (changed it to CallQI), and updated the stuff in
nsROCSSPrimitiveValue to work with the new mValue.mString

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.2.2

Also free up mValue.mString if mType is CSS_URI.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 73376 [details] [diff] [review]
Patch v1.2.2

Since these changes are made only to match changes to nsROCSSPrimitiveValue,
transferring over r=bzbarsky and sr=jst.

Comment 18

[Hixie (not reading bugmail)]

Don't forget to back-slash escape any '(' or ')' characters in URIs when you
generate url() values...

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

> Don't forget to back-slash escape any '(' or ')' characters in URIs when you
> generate url() values...

Or '"', "'", ',', or ' ' as well, per the spec.

I looked at nsCSSValue and nsCSSDeclaration to see how they went about this, and
it appears they don't back-slash escape either...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Er... if you do that, make sure to teach the context menu code about it.

Comment 21

[Hixie (not reading bugmail)]

Context menu code?

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

The context menu uses computed values to see if there is a background-image or
not to determine whether to put up the 'View Background Image' menu, and to pull
out the URI to go to upon clicking that menu.

I'm spinning off escaping URI values into bug 129960.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 73376 [details] [diff] [review]
Patch v1.2.2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 24

[Hixie (not reading bugmail)]

And the CSSOM has no way to return the actual URI from a URI value? Jeez, that
sucks. Couldn't we add a mozURI property or something? We really don't want to
have to decode it each time.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fixed

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Hixie, You must have gotten that comment in right before I loaded this bug to
resolve it :)

Anyway, I responded to your latest comment on bug 129960.  Moving further
discussion on the issue of escaping/unescaping URI values over there.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Temporary patch to prevent assertions with XBL using -moz-binding: none;

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Oops, this is the real patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 73811 [details] [diff] [review]
Oops, this is the real patch.

r=bzbarsky (and yes, I built and tested this time)

Comment 30

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73811 [details] [diff] [review]
Oops, this is the real patch.

sr=shaver for 1.0 trunk.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 73811 [details] [diff] [review]
Oops, this is the real patch.

a=[e-mail], hope this isn't redundant in a closed bug.

/be

Comment 32

[Boris Zbarsky [:bzbarsky]]

XBL fix checked in