"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
17 years ago
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla0.9.9
17 years ago
Priority: P4 → P3
The shorthand system needs a major overhaul
Component: Style System → DOM Style
Depends on: 108923
Priority: P3 → P5
Target Milestone: mozilla0.9.9 → Future
16 years ago
16 years ago
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
You need to log in before you can comment on or make changes to this bug.