104321 - [FIX]cssText property for a DOMCSSDeclaration is broken for clip

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

"clip: rect(a, b, c, d)" is listed as:

"-x-clip-top: a; -x-clip-right: b; -x-clip-bottom: c; -x-clip-left: d"

in the cssText property of a DOMCSSDeclaration

Comment 1

[Boris Zbarsky [:bzbarsky]]

glazou says he does not have time for this...

Comment 2

[Boris Zbarsky [:bzbarsky]]

The shorthand system needs a major overhaul

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: silly testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

*** Bug 123276 has been marked as a duplicate of this bug. ***

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch v 1.0

Comment 6

[Boris Zbarsky [:bzbarsky]]

Daniel, would you review please?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: The real silly testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: slightly better assertion wording.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 89987 [details] [diff] [review]
slightly better assertion wording.


>@@ -5880,10 +5920,16 @@
>         NS_CASE_OUTPUT_PROPERTY_VALUE(eCSSProperty_background_color, bgColor)
>         NS_CASE_OUTPUT_PROPERTY_VALUE(eCSSProperty_background_image, bgImage)
>         NS_CASE_OUTPUT_PROPERTY_VALUE(eCSSProperty_background_repeat, bgRepeat)
>         NS_CASE_OUTPUT_PROPERTY_VALUE(eCSSProperty_background_attachment, bgAttachment)
> 
>+        case eCSSProperty_clip_top:
>+        case eCSSProperty_clip_right:
>+        case eCSSProperty_clip_bottom:
>+        case eCSSProperty_clip_left:
>+          // these are always handled by the shorthand
>+          break;
>         case eCSSProperty_background_x_position:

Hmmm... This approach is ok only if it is impossible to set for instance
-x-clip-top individually...
If it is possible to do it - and in fact in all cases - I prefer four
NS_CASE_OUTPUT_...
like above with a reset of aTop ... aBottom in DoClipShorthand() if the
properties
are "consumed".

Comment 10

[Boris Zbarsky [:bzbarsky]]

> if it is impossible to set for instance -x-clip-top individually...

It is impossible to set it individually via the parser or any DOM methods.  Any
internal Mozilla code that does set it individually is very very confused and
should be triggering an assertion somewhere.

The point is, clip is not a real shorthand.  It's a single property.  If we
output clip, we MUST always consume all four values.  If that has not happened,
something is very deeply wrong.

Now I could add those "consumed" flags and always set them to 0 in the shorthand
and always assert that they are zero here.  But that seems like lots of extra
work during execution for a debug-only check that can only fail if someone who
utterly misunderstands how the clip property works edits the "shorthand" function.

Perhaps the confusion is caused by the use of the word "shorthand"?  I would be
happy to rename things appropriately to make it clear that this is in fact _not_
a shorthand but a single property with a single value.

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 89987 [details] [diff] [review]
slightly better assertion wording.

r=glazman
providing the fact that you explain in a comment why you don't need to
"consume" clipTop and others and use the macro.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 89987 [details] [diff] [review]
slightly better assertion wording.

sr=dbaron if you put the ' ' into the ":" and drop the extra Append:

>+  aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(eCSSProperty_clip))
>+                 + NS_LITERAL_STRING(":"));
>+  aString.Append(PRUnichar(' '));

Comment 13

[Boris Zbarsky [:bzbarsky]]

fixed on trunk.