[CSS] bad canonicalization of border shorthand properties

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: glazou, Assigned: glazou)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
The following declarations :

  border-top-style: solid;
  border-left-style: solid;
  border-right-style: solid;
  border-bottom-style: solid;
  border-top-width: 9pt;
  border-top-color: red;

are incorrectly canonicalized by nsCSSDeclaration::ToString() into

  border: solid;
  border-top: 9pt red;

The first set of declarations give the following set of values (suppose that
the computed value of the 'color' property is black) :

  top side     :    9pt     solid     red
  left side    :    medium  solid     black
  right side   :    medium  solid     black
  bottom side  :    medium  solid     black

The second set gives

  top side     :    9pt     none      red    <<<<<<<<< DIFFERENCE
  left side    :    medium  solid     black
  right side   :    medium  solid     black
  bottom side  :    medium  solid     black
(Assignee)

Comment 1

16 years ago
Created attachment 82205 [details] [diff] [review]
patch v 1.0


Jonas, care to review ?
It seems wrong that we canonicalized

  border-top-style: solid;
  border-left-style: solid;
  border-right-style: solid;
  border-bottom-style: solid;

to

  border: solid;

Consider the following stylesheet:

foo {
  border-top-style: solid;
  border-left-style: solid;
  border-right-style: solid;
  border-bottom-style: solid;
}

* {
  border: 10px dotted green;
}

gives <foo>s a border of "10px solid green". whereas

foo {
  border: solid;
}

* {
  border: 10px dotted green;
}

gives <foo>s a border of "medium solid <some color>".

Not sure if this should be handled in a different bug though?
If we convert

   border-top-style: solid;
   border-left-style: solid;
   border-right-style: solid;
   border-bottom-style: solid;

...to

   border: solid;

...then that is definitely a bug. It should be, at most, converted to

   border-style: solid;
(Assignee)

Comment 4

16 years ago
> If we convert
>
> border-top-style: solid;  border-left-style: solid;  border-right-style: solid;
> border-bottom-style: solid;
>
>...to
>
>   border: solid;
>
>...then that is definitely a bug

No we don't. 
Status: NEW → ASSIGNED
(To clarify, glazou told me he means we don't with his patch. I believe Jonas
was saying that we do at the moment, i.e., without glazou's patch.)
This pseudo-code is a pretty optimal way of determining which properties need to
be used to accurately and minimally canonicalise the border-*-* properties:

   let propertiesSet be values of the following that are set, ored together:

      border-top-style     0x001
      border-left-style    0x002
      border-right-style   0x004
      border-bottom-style  0x008
      border-top-color     0x010
      border-left-color    0x020
      border-right-color   0x040
      border-bottom-color  0x080
      border-top-width     0x100
      border-left-width    0x200
      border-right-width   0x400
      border-bottom-width  0x800

   if (propertiesSet & 0xFFF and
        all styles are the same and
        all colors are the same and
        all widths are the same): use 'border', else {
      let propertiesToSet = 0;
      if (propertiesSet & 0x00F): use 'border-style',
         else let propertiesToSet |= 0x00F;
      if (propertiesSet & 0x0F0): use 'border-color',
         else let propertiesToSet |= 0x0F0;
      if (propertiesSet & 0xF00): use 'border-width',
         else let propertiesToSet |= 0xF00;
      let propertiesToSet &= propertiesSet;
      if (propertiesToSet) {
         let finalPropertiesToSet = 0;
         if (propertiesToSet & 0x111): use 'border-top',
         else let finalPropertiesToSet |= 0x111;
         if (propertiesToSet & 0x222): use 'border-left',
         else let finalPropertiesToSet |= 0x222;
         if (propertiesToSet & 0x444): use 'border-right',
         else let finalPropertiesToSet |= 0x444;
         if (propertiesToSet & 0x888): use 'border-bottom',
         else let finalPropertiesToSet |= 0x888;
         let finalPropertiesToSet &= propertiesSet;
         if (finalPropertiesToSet) {
             if (finalPropertiesToSet & 0x001): use 'border-top-style'
             if (finalPropertiesToSet & 0x002): use 'border-left-style'
             if (finalPropertiesToSet & 0x004): use 'border-right-style'
             if (finalPropertiesToSet & 0x008): use 'border-bottom-style'
             if (finalPropertiesToSet & 0x010): use 'border-top-color'
             if (finalPropertiesToSet & 0x020): use 'border-left-color'
             if (finalPropertiesToSet & 0x040): use 'border-right-color'
             if (finalPropertiesToSet & 0x080): use 'border-bottom-color'
             if (finalPropertiesToSet & 0x100): use 'border-top-width'
             if (finalPropertiesToSet & 0x200): use 'border-left-width'
             if (finalPropertiesToSet & 0x400): use 'border-right-width'
             if (finalPropertiesToSet & 0x800): use 'border-bottom-width'
         }
      }
   }

I think it works correctly... if it doesn't, please tell me why! :-)
(Assignee)

Comment 7

16 years ago
That's what I am doing right now. But that's not minimal. Test case :

   border-*-style: solid    for the four sides
   border-XXX-color: red    for left and right sides
   border-XXX-width: thin   for left and right sides
Here's a better version. It increases the likelyhood of redundancy but decreases
the overall number of properties used.

   let propertiesSet be values of the following that are set, ored together:

      border-top-style     0x001
      border-left-style    0x002
      border-right-style   0x004
      border-bottom-style  0x008
      border-top-color     0x010
      border-left-color    0x020
      border-right-color   0x040
      border-bottom-color  0x080
      border-top-width     0x100
      border-left-width    0x200
      border-right-width   0x400
      border-bottom-width  0x800

   if (propertiesSet & 0xFFF and
        all styles are the same and
        all colors are the same and
        all widths are the same): use 'border', else {
      let propertiesToSet = 0;
      if (propertiesSet & 0x00F): use 'border-style',
         else let propertiesToSet |= 0x00F;
      if (propertiesSet & 0x0F0): use 'border-color',
         else let propertiesToSet |= 0x0F0;
      if (propertiesSet & 0xF00): use 'border-width',
         else let propertiesToSet |= 0xF00;
      let propertiesToSet &= propertiesSet;
      if (propertiesToSet) {
         let finalPropertiesToSet = 0;
         if (propertiesSet & 0x111): use 'border-top',
            else let finalPropertiesToSet |= 0x111;
         if (propertiesSet & 0x222): use 'border-left',
            else let finalPropertiesToSet |= 0x222;
         if (propertiesSet & 0x444): use 'border-right',
            else let finalPropertiesToSet |= 0x444;
         if (propertiesSet & 0x888): use 'border-bottom',
            else let finalPropertiesToSet |= 0x888;
         let finalPropertiesToSet &= propertiesToSet;
         if (finalPropertiesToSet) {
             if (finalPropertiesToSet & 0x001): use 'border-top-style'
             if (finalPropertiesToSet & 0x002): use 'border-left-style'
             if (finalPropertiesToSet & 0x004): use 'border-right-style'
             if (finalPropertiesToSet & 0x008): use 'border-bottom-style'
             if (finalPropertiesToSet & 0x010): use 'border-top-color'
             if (finalPropertiesToSet & 0x020): use 'border-left-color'
             if (finalPropertiesToSet & 0x040): use 'border-right-color'
             if (finalPropertiesToSet & 0x080): use 'border-bottom-color'
             if (finalPropertiesToSet & 0x100): use 'border-top-width'
             if (finalPropertiesToSet & 0x200): use 'border-left-width'
             if (finalPropertiesToSet & 0x400): use 'border-right-width'
             if (finalPropertiesToSet & 0x800): use 'border-bottom-width'
         }
      }
   }
(Assignee)

Comment 9

16 years ago
ooooh, I like this one ; my current implem is the answer to your comment #6 but
the light modifs you made to obtain algo in comment #8 make it much nicer.
Trying...
(Assignee)

Comment 10

16 years ago
Created attachment 83534 [details] [diff] [review]
patch v2.0


This new patch merges fixes for bugs 142019 (the current one) and bug 107923
(!important property values missing or not tagged important). This patch is
ready
for reviews.
Attachment #82205 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
No longer blocks: 107923
Comment on attachment 83534 [details] [diff] [review]
patch v2.0

>+  PRBool  isImportant = GetValueIsImportant(aProperty);
>+  if (PR_TRUE == isImportant) {

Why not just |if (isImportant)| ?

> +nsCSSDeclaration::TryBorderShorthand(nsAString & aString, PRInt32 aPropertiesSet,

PRUint32 aPropertiesSet

> +  if (0xfff == aPropertiesSet

Could we possibly have some recognizable names for these magic values?	(0xfff,
0x00f, 0x0f0,
0xf00, 0x111, 0x222, 0x444, 0x888).  It's not absolutely necessary, but would
make this code
more readable.

Failing that, please have some comments explaining what the heck is going on.

>+    if (!valueString.Equals(NS_LITERAL_STRING("-moz-use-text-color"))) {

Could we have a comment explaining why this is needed?

> nsCSSDeclaration::TryBorderSideShorthand(nsAString & aString,
>                                          nsCSSProperty aShorthand,
>                                          PRInt32 & aBorderWidth,
>                                          PRInt32 & aBorderStyle,
>                                          PRInt32 & aBorderColor)

Do you ever set aBorder* in this method?  If not, take out those "&"

>+  if (aBgColor && aBgImage && aBgRepeat && aBgAttachment && (aBgPositionX*aBgPositionY) &&
>+      AllPropertiesSameImportance(aBgColor, aBgImage, aBgRepeat, aBgAttachment,
>+                                  (aBgPositionX*aBgPositionY), isImportant)) {

What's with (aBgPositionX*aBgPositionY)?  That's almost certainly wrong in the 
AllPropertiesSameImportance call and it would be clearer to list them
separately in
the boolean test, no?

>+#define NS_CASE_CONDITIONAL_OUTPUT_PROPERTY_VALUE(_condition, _prop, _index) \
>+case _prop: \
>+          if (_condition && _index) { \

((_condition) && _index) just in case someone writes a condition like "foo ||
bar"

>+void nsCSSDeclaration::PropertyIsSet(PRInt32 & aPropertyIndex, PRInt32 aIndex, PRInt32 & aSet, PRInt32 aValue)
>+{
>+  aPropertyIndex = aIndex + 1;
>+  aSet |= aValue;

PRUint32 & aPropertySet, maybe?  Note the "U".

>+    PRInt32 borderPropertiesSet = 0, finalBorderPropertiesToSet = 0;

PRUint32

>+      PRInt32 borderPropertiesToSet = 0;

PRUint32


>+      if ((borderPropertiesSet & 0x00f) != 0x00f ||

More magic constants that needs names or comments....

>+          TryFourSidesShorthand(aString, eCSSProperty_border_style,
>+                                borderTopStyle, borderBottomStyle,
>+                                borderLeftStyle, borderRightStyle,
>+                                PR_FALSE)) {
>+        borderPropertiesToSet |= 0x00f;

Should that not be "! TryFourSidesShorthand(....)" ?

>+      }
>+      if ((borderPropertiesSet & 0x0f0) != 0x0f0 ||
>+          TryFourSidesShorthand(aString, eCSSProperty_border_color,
>+                                borderTopColor, borderBottomColor,
>+                                borderLeftColor, borderRightColor,
>+                                PR_FALSE)) {
>+        borderPropertiesToSet |= 0x0f0;
>+      }
>+      if ((borderPropertiesSet & 0xf00) != 0xf00 ||
>+          TryFourSidesShorthand(aString, eCSSProperty_border_width,
>+                                borderTopWidth, borderBottomWidth,
>+                                borderLeftWidth, borderRightWidth,
>+                                PR_FALSE)) {
>+        borderPropertiesToSet |= 0xf00;

Same for those two blocks.

>+      if (borderPropertiesToSet) {
>+        if ((borderPropertiesSet & 0x111) != 0x111 ||
>+            TryBorderSideShorthand(aString, eCSSProperty_border_top,
>+                                   borderTopWidth, borderTopStyle, borderTopColor)) {
>+          finalBorderPropertiesToSet |= 0x111;
>+        }
>+        if ((borderPropertiesSet & 0x222) != 0x222 ||
>+            TryBorderSideShorthand(aString, eCSSProperty_border_left,
>+                                   borderLeftWidth, borderLeftStyle, borderLeftColor)) {
>+          finalBorderPropertiesToSet |= 0x222;
>+        }
>+        if ((borderPropertiesSet & 0x444) != 0x444 ||
>+            TryBorderSideShorthand(aString, eCSSProperty_border_right,
>+                                   borderRightWidth, borderRightStyle, borderRightColor)) {
>+          finalBorderPropertiesToSet |= 0x444;
>+        }
>+        if ((borderPropertiesSet & 0x888) != 0x888 ||
>+            TryBorderSideShorthand(aString, eCSSProperty_border_bottom,
>+                                   borderBottomWidth, borderBottomStyle, borderBottomColor)) {
>+          finalBorderPropertiesToSet |= 0x888;
>+        }

And the same for all of these (add the "!" before the
"TryBorderSideShorthand").

Could you please attach whatever testcases you have been using to test this?
Attachment #83534 - Flags: needs-work+
(Assignee)

Comment 12

16 years ago
Created attachment 83709 [details] [diff] [review]
patch v2.1 in answer to Boris's comments
Attachment #83534 - Attachment is obsolete: true
So.. I think our handling of #foo5 in that testcase is (while correct)
sub-optimal.  In particular, we repeat all the width and color information twice
instead of just realizing that we have already specified border-width and
border-color and specifying the four border-styles.

I personally find this acceptable (and better by far than being incorrect), so
r=bzbarsky, but feel free to modify how that's handled...
(Assignee)

Comment 15

16 years ago
Boris : right, that's why Hixie said "It increases the likelyhood of redundancy".
I can live with that. I think that we should have a hidden pref allowing to select
between "I want shorthand canonicalization" and "don't output shorthands", but
that can live in another bug.
Comment on attachment 83709 [details] [diff] [review]
patch v2.1 in answer to Boris's comments

r=bzbarsky
Attachment #83709 - Flags: review+
please, no more prefs (hidden or not). We don't need the bloat, and it makes QA 
and debugging very, very painful as hidden prefs get basically no QA and end up 
bitrotting and becoming useless.
Comment on attachment 83709 [details] [diff] [review]
patch v2.1 in answer to Boris's comments

sr=jst
Attachment #83709 - Flags: superreview+
(Assignee)

Comment 19

16 years ago
checked in into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 20

16 years ago
This commit have added a warning on brad TBox:

+content/html/style/src/nsCSSDeclaration.cpp:5461
+ `PRBool isImportant' might be used uninitialized in this function

See also
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsCSSDeclaration.cpp&mark=5461,5489,5512#5446

(Assignee)

Comment 21

16 years ago
Good catch Aleksey, thanks a lot.
You need to log in before you can comment on or make changes to this bug.