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

RESOLVED FIXED in mozilla1.2beta

Status

()

Core
DOM: CSS Object Model
P1
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.2beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

"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
glazou says he does not have time for this...
Assignee: glazman → bzbarsky
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla0.9.9
The shorthand system needs a major overhaul

Component: Style System → DOM Style
Depends on: 108923
Priority: P3 → P5
Target Milestone: mozilla0.9.9 → Future
No longer blocks: 123276
*** Bug 123276 has been marked as a duplicate of this bug. ***
Daniel, would you review please?
No longer depends on: 108923
Priority: P5 → P1
Summary: cssText property for a DOMCSSDeclaration is broken for clip → [FIX]cssText property for a DOMCSSDeclaration is broken for clip
Target Milestone: Future → mozilla1.2beta
Created attachment 89058 [details]
The real silly testcase
Attachment #89055 - Attachment is obsolete: true
Created attachment 89987 [details] [diff] [review]
slightly better assertion wording.
Attachment #89057 - Attachment is obsolete: true
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".
Attachment #89987 - Flags: needs-work+
> 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 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.
Attachment #89987 - Flags: needs-work+ → review+
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(' '));
Attachment #89987 - Flags: superreview+
fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Blocks: 183189
You need to log in before you can comment on or make changes to this bug.