Closed Bug 18894 Opened 20 years ago Closed 18 years ago

STYLE attribute expands CSS shorthand property assignment to equivalent longhand assignments

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: bugs, Assigned: glazou)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Target Milestone: M12
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?
Assignee: cmanske → pierre
Component: Editor → Style System
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.
Status: NEW → ASSIGNED
Target Milestone: M12 → M15
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.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → LATER
*** Bug 44043 has been marked as a duplicate of this bug. ***
Reopening my LATER'd bugs.
Status: RESOLVED → REOPENED
Resolution: LATER → ---
Target Milestone: M16 → mozilla1.0
Reassigned to Daniel like bug 58638.  It is blocking some features in the Editor.
Assignee: pierre → glazman
Status: REOPENED → NEW
Depends on: 58638
OS: Windows 95 → All
Hardware: PC → All
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

Status: NEW → ASSIGNED
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.
Blocks: 77705
Summary: Setting style property of selected object to compound object does not work properly → STYLE attribute expands CSS shorthand property assignment to equivalent longhand assignments
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.
Target Milestone: mozilla1.0 → mozilla0.9.6
Attached patch patch v1.0 ready for reviews (obsolete) — Splinter Review
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);
}
Attachment #52719 - Flags: needs-work+
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.
Attachment #52719 - Attachment is obsolete: true
Attached patch patch v2.0Splinter Review
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.
Attachment #54668 - Flags: review+
> 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?
Attachment #54673 - Flags: superreview+
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).
Status: ASSIGNED → RESOLVED
Closed: 20 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.