In my advanced edit code, I allow the user to set his or her own inline style properties. One might enter border: 1px solid blue; as a property. What's Expected: My Advanced Edit dialog code appends this to the inline STYLE string, and uses setAttribute() on the selected editor element to set the "style" attribute of that element to the stylestring. This stylestring should include "border: 1px solid blue;" What Actually Happens: Sure enough, my Advanced Edit code sets the right string. However, closing the Advanced Edit dialog and doing a Debug -> Output HTML shows that "style" on that element has actually been set to: border-left-color: blue; border-left-style: solid; border-left-width: 1px; border-right-color: blue; border-right-style: solid; border-right-width: 1px;... and so on.. in other words, if the CSS property is a "compound" one, the style string that is set is actually broken down into the constituent chunks. This isn't very useful from a user's point of view, and unnecessarily complicates the generated HTML. (I also tried this with outline: 1px solid blue and the same thing happened) I'll mail you my Advanced Edit code later today when I get it into a working state so you can see this yourself.

Since the editor is just setting an attribute on a node, this problem would be caused by how those attributes are converted to HTML style attributes during file saving. Akkana: Do you know anything about this? Troy or Pierre: Does layout participate in that?

What is in the content tree? Debug->Dump Content Tree.

I'm afraid it is a problem with the style system that can't be fixed anytime soon. How important is it to you, Editor folks?

It's obviously just a matter of outputing the most efficient HTML. We can live with it now (as long as the style is correct), but it should be fixed before releasing 5.0, don't you think? Beth: What do you think? Assigning the bug to Pierre.

Pushing my M15 bugs to M16

What you call a "compound" property is called a "short-cut" in the style system. It really is a syntactical short-cut: once it is parsed, nowhere we keep track of which property was set by a short-cut and which were set by a simple declaration, and which were set by a short-cut and then overriden by another declaration, etc... It's a weakness of our storage model. It won't be fixed for this version. Marked Later.

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

Reopening my LATER'd bugs.

Reassigned to Daniel like bug 58638. It is blocking some features in the Editor.

Where is what the DOM Level 2 CSS spec says: ---- If an implementation does implement this interface, it is expected to understand the specific syntax of the shorthand properties, and apply their semantics; when the margin property is set, for example, the marginTop, marginRight, marginBottom and marginLeft properties are actually being set by the underlying implementation. When dealing with CSS "shorthand" properties, the shorthand properties should be decomposed into their component longhand properties as appropriate, and when querying for their value, the form returned should be the shortest form exactly equivalent to the declarations made in the ruleset. However, if there is no shorthand declaration that could be added to the ruleset without changing in any way the rules already declared in the ruleset (i.e., by adding longhand rules that were previously not declared in the ruleset), then the empty string should be returned for the shorthand property. For example, querying for the font property should not return "normal normal normal 14pt/normal Arial, sans-serif", when "14pt Arial, sans-serif" suffices. (The normals are initial values, and are implied by use of the longhand property.) ----

Pierre, I don't think that this text has anything to do with the root of our problem here : given an element and the contents of his style attribute, we generate a CSSDeclaration; and for that purpose, we decompose a shorthand property into its "longhand" components. We all agree on that algo. But there is **no*** reason why we need to update the contents of the STYLE attribute with the string representation of the parsed CSSDeclaration. In fact, we *must not* change the contents of the STYLE attribute because the contents of this attribute can be in another language than CSS. Even if **our** style engine understands only CSS, a script in the document or a third-party plug-in might want to use the contents of the STYLE attribute. For the moment, we throw away everything if we don't understand it. I guess that we have to keep the original string contents of the attribute instead of generating on the fly by a call to ToString() when it is queried. That would solve our problem, even I see an impact on memory. In my opinion, we are buggy here and we need to solve this problem anyway. I have to add that this bug could be VERY annoying for third-party vendors wanting to embed our technology. Excerpts of HTML 4.01 spec; section 14.2.2: The syntax of the value of the style attribute is determined by the default style sheet language. Section 14.2.1 : User agents should determine the default style sheet language for a document according to the following steps (highest to lowest priority): 1. If any META declarations specify the "Content-Style-Type", the last one in the character stream determines the default style sheet language. 2. Otherwise, if any HTTP headers specify the "Content-Style-Type", the last one in the character stream determines the default style sheet language. 3. Otherwise, the default style sheet language is "text/css".

Ok, I think I can see how we can do the half way, ie provide a way to answer with shorthands if we query the value of the style attribute. Working on it now. I have important questions : how are we going to control this output ? Do we always want to answer with shorthands, ie get the most concise style attribute ? What's the level of support for shorthands in other browsers ? Here are the existing shorthands in CSS 2: border-top, border-bottom, border-left, border-right border margin padding background font list-style outline pause cue

Fix in hand for the 'border' shorthand property ; works just fine :-) Trying now 'margin' and 'padding'; I guess the others are of much lower priority.

I'd like to see Daniel's fix. I've been thinking a bit about this and thought it might be a good idea to "always want to answer with shorthands". It seems we could look at the values and combine to a shorthand replacedment, e.g.: if all border values are the same (2), use "border : 2" instead of: "border-top : 2; border-right : 2; border-bottom : 2; border-left : 2; "It seems we would have to know what the default values are in order to know what properties we can ommit. Daniel: is that what you did?

Always answer with the simplest form seems to be a better strategy than the one we have now (which is "always the longest form"). It will improve things in many cases even though round-tripping will still not be guaranteed. The text I copied does not point out the root of the problem, it explains what we should do, or under what form we should return a shorthand property. 'background' and 'font' are as important as 'border' and 'margin', I think. Will you fix bug 58638 at the same time?

> Will you fix bug 58638 at the same time? Hmmmm, not sure I can; the problems look similar, the solutions look similar and will share some code, but will also widely differ in other pieces of code I'll finish the bug first and work on 58638 later, even if I have to move some code in order to share it.

Pierre, outputing the 'font' shorthand could result in user's disapointment. Here is why : let's suppose we have an element with the following specified declarations : font-family: sans-serif; font-size: 2em; font-style: italic; We would like to use the 'font' shorthand, right ? But 'font' resets all the font-* properties and those properties are ALL inherited. It implies that the only way to reduce the set above is the following one : font: italic 2em sans-serif; font-variant: inherit; font-weight: inherit; line-height: inherit; font-stretch: inherit; font-size-adjust: inherit; Do we really want to run into that ? Is that considered being more concise than the original set of declarations with langhands ? I doubt it. I think this issue needs to be raised to the CSS WG. Shorthanding inherited properties makes the implem of what DOM 2 Style says about outputing shorthands quite hard, or worse, not desirable from user's point of view.

The patch attached above handles correctly the following shorthands border, border-top, border-bottom, border-left, border-right, margin, padding, background Pierre and Marc, can you please r= and sr= ?

Comment on attachment 52719 [details] [diff] [review] patch v1.0 ready for reviews General comments. If you're adding functions that are not actually interface methods, just use |nsresult| instead of |NS_IMETHOD| and |NS_IMETHODIMP|. Try to observe the 80-char limit. And is there a reason to make all of these public, beyond the fact that all the other methods of this class are? >+ PRBool AppendPropertyAndValueToString(nsCSSProperty aProperty, nsAWritableString& aResult); >+ NS_IMETHOD TryBorderShorthand(nsAWritableString & aString, >+ PRInt32 & borderTopWidth, PRInt32 & borderTopStyle, PRInt32 & borderTopColor, weird indentation here... >+ NS_IMETHOD TryBorderSideShorthand(nsAWritableString & aString, nsCSSProperty aShorthand, >+ PRInt32 & borderWidth, PRInt32 & borderStyle, PRInt32 & borderColor); And here. >+ NS_IMETHOD TryMarginOrPaddingShorthand(nsAWritableString & aString, nsCSSProperty aShorthand, >+ PRInt32 & aTop, PRInt32 & aBottom, PRInt32 & aLeft, PRInt32 & aRight); And here. >+ NS_IMETHOD TryBackgroundShorthand(nsAWritableString & aString, >+ PRInt32 & bgColor, PRInt32 & bgImage, And here. In CSSDeclarationImpl::AllPropertiesSameValue, I would reverse the sense of all the tests, drop the "result" variable, and have early returns of PR_FALSE if the quantities are not equal. Then just return PR_TRUE at the end if you get that far. I think that would make the code less indented and more readable. >+PRBool >+CSSDeclarationImpl::AppendPropertyAndValueToString(nsCSSProperty aProperty, nsAWritableString& aString) >+{ >+ if (0 <= aProperty) { This test seems redundant. You should never be calling this if aProperty < 0. I would add an assertion on aProperty and not do this test. >+ aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(aProperty))); >+ aString.Append(NS_LITERAL_STRING(": ")); Please use + here. So: aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(aProperty)) + NS_LITERAL_STRING(": ")); >+ PRBool res = AppendValueToString(aProperty, aString); >+ aString.Append(NS_LITERAL_STRING("; ")); >+ return res; Should you not check |res| before appending the "; " ? >+ } >+ return NS_OK; That's not a PRBool. This function should not be returning it. >+CSSDeclarationImpl::TryBorderShorthand(nsAWritableString & aString, >+ PRInt32 & aBorderTopWidth, PRInt32 & aBorderTopStyle, PRInt32 & aBorderTopColor, Again, weird indentation. >+ if (aBorderTopWidth && aBorderBottomWidth && aBorderLeftWidth && aBorderRightWidth && >+ AllPropertiesSameValue(aBorderTopWidth-1, aBorderBottomWidth-1, aBorderLeftWidth-1, aBorderRightWidth-1)) I'm not sure about +1 and -1 business. It would be much clearer, imo, if you would define something like kNoValue to be -1, initialize all those to kNoValue instead of 0, then when you go through the property list just set them to the correct index, not |index+1|. Then here you can avoid the bizarre subtraction of 1 and make your test much more clear about what it's actually testing. >+ aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(eCSSProperty_border))); >+ aString.Append(NS_LITERAL_STRING(": ")); Again, use + as much as possible for string concatenation. >+ aBorderTopStyle = 0; aBorderBottomStyle = 0; aBorderLeftStyle = 0; aBorderRightStyle = 0; This would go over better as 2 lines of code. Or better yet, 4. >+ aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(aShorthand))); >+ aString.Append(NS_LITERAL_STRING(":")); Use + to concatenate >+ AppendValueToString(topProp, aString); This would be better as |AppendValueToString(topProp, topValue, aString);| and same elsewhere in this function. You already have the value, why not use it? >+CSSDeclarationImpl::TryBackgroundShorthand(nsAWritableString & aString, >+ PRInt32 & aBgColor, PRInt32 & aBgImage, indentation? >+ aString.Append(NS_ConvertASCIItoUCS2(nsCSSProps::GetStringValue(eCSSProperty_background))); >+ aString.Append(NS_LITERAL_STRING(":")); Use + >+ PRInt32 border = 0; You never use this do you? >+ PRUint32 length = aString.Length(); >+ if (length) { >+ // if the string is not empty, we have a trailing whitespace we should remove >+ aString.SetLength(length - 1); May be better as: if (! aString.IsEmpty()) { aString.Truncate(aString.Length() - 1); }

Oh, and the CSSDeclaration actually has no value for background-position. It has -x-background-x-position and -x-background-y-position. Those need to be combined into a single background-position both in the "background" shorthand and in the longhand "background-position"

Boris: thanks for your useful comments. About the +1/-1 thing, I disagree with you. I think that adding everywhere tests (kNoValue != blabla) makes the code less clear than this very simple index offset thing. I'll keep the code as it is.

Comment on attachment 54668 [details] [diff] [review] patch v2.0 >+ void TryBorderShorthand(nsAWritableString & aString, >+ PRInt32 & borderTopWidth, Could you name all the parameters aParameter instead of parameter? >+ if (topValue != rightValue || topValue != leftValue || topValue != rightValue) { You have topValue != rightValue twice. >+ PRInt8 numberPropertiesSpecified = (aBgColor ? 1 : 0) + (aBgImage? 1 : 0) >+ + (aBgRepeat? 1 : 0) + (aBgAttachment? 1 : 0) >+ + (aBgPositionX*aBgPositionY? 1 : 0); Inconsistent whitespace (aBgColor ? vs aBgImage?). >+ if (2 <= numberPropertiesSpecified ) { No space before the closing parens. I find (2 <= numberPropertiesSpecified) harder to read then (numberPropertiesSpecified >= 2), but that's a style (no pun intended) issue i guess. >+ if (backgroundXValue != backgroundYValue) { You told me this doesn't always work :). The new code I saw fixed this. r=peterv with those fixed.

> You told me this doesn't always work Right, because background-position-x and -y can have enumerated values top|center|bottom and left|center|right. The enum index can be the same and that does not mean the values are the same. Only 'center' can match for both properties.

Comment on attachment 54673 [details] [diff] [review] patch v2.1 in answer to peterv sr=attinasi - NOTE: ToString looks like it will be considerably slower now, but I'm guessing that this is a seldom-called method. Is that assumption correct?

Marc: right, this code is call only when you query the value of the STYLE attribute, or of course, the cssText member of a DOMCSSStyleDeclaration. checked in (trunk).