Bug 9284 – Quirksmode (CSS1): Removing inline border styles is impossible

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Adele Peterson]

http://www.quirksmode.org/bugreports/archives/safari/index.html

"Setting an inline border style (element.style.border) works in all browsers. Removing it to allow the normal border styles to return, however, is tricky in Explorer Windows and impossible in Safari.

Test page. http://www.quirksmode.org/js/tests/bordertest.html
Workaround is included.
Reported by ppk."

[Joost de Valk (AlthA)]

CHanging component.

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

Priority and CSS1 copied over from bug 9927.

Created attachment 12285 [details]
Expand shorthands for removal

Please let me know if this is going in the right direction. Not sure about using a HashMap - perhaps a simple array indexed by property ID would be better.

[Dave Hyatt]

Yeah, this is great.  What about "border" though?  I see expansions for border-top/left/right/bottom but not for border.  I assume it will be recursive and will expand border into border-top/left/right/bottom and then from there into top-width/color/style, etc.

(In reply to comment #5)
> Yeah, this is great.  What about "border" though?  I see expansions for
> border-top/left/right/bottom but not for border.

Hiding here (I wanted to reuse the first 4 arrays):

+    shorthandMap->set(CSS_PROP_BORDER, new PropertyLonghand(borderTopProperties, (sizeof(borderTopProperties) + sizeof(borderRightProperties) + sizeof(borderBottomProperties) + sizeof(borderLeftProperties)) / sizeof(borderTopProperties[0])));


>  I assume it will be recursive
> and will expand border into border-top/left/right/bottom and then from there
> into top-width/color/style, etc.

Given that border is the only 2-level (and 2-way) "hierarchy", I figured it would be an overkill to implement it with recursion.

> 

[Darin Adler]

Comment on attachment 12285 [details]
Expand shorthands for removal

+struct PropertyLonghand
+{

We should put the brace on the line iwth the struct name.

+static HashMap<int, PropertyLonghand* >* shorthandMap;

I don't think the space before the > is needed here. I think the values should be PropertyLonghand, no need for pointers.

The shortHandMap could be a static inside the CSSMutableStyleDeclaration::removeProperty function -- I think I like that slightly better.

It would be nice if the function that turns a propertyID into a pointer to a list of property IDs and a length was put with the other shorthand handling rather than right here in CSSMutableStyleDeclaration -- CSSMutableStyleDeclaration could just call a function defined there. That might make it easier to keep the list of shorthands in sync with the other lists of shorthands.

Should I mark this review- now, or do you want other people to look at it?

[Darin Adler]

Comment on attachment 12285 [details]
Expand shorthands for removal

> Hiding here (I wanted to reuse the first 4 arrays):

> +    shorthandMap->set(CSS_PROP_BORDER, new
PropertyLonghand(borderTopProperties, (sizeof(borderTopProperties) +
sizeof(borderRightProperties) + sizeof(borderBottomProperties) > +
sizeof(borderLeftProperties)) / sizeof(borderTopProperties[0])));

That technique is not reliable. It relies on the four arrays being consecutive in global data, something that is not guaranteed by the C++ language.

Created attachment 12303 [details]
Expand shorthands for removal

(In reply to comment #7)
> +struct PropertyLonghand
> +{
> 
> We should put the brace on the line iwth the struct name.

Done.

> +static HashMap<int, PropertyLonghand* >* shorthandMap;
> 
> I don't think the space before the > is needed here. I think the values should
> be PropertyLonghand, no need for pointers.

Done.

> The shortHandMap could be a static inside the
> CSSMutableStyleDeclaration::removeProperty function -- I think I like that
> slightly better.

Done.

> It would be nice if the function that turns a propertyID into a pointer to a
> list of property IDs and a length was put with the other shorthand handling
> rather than right here in CSSMutableStyleDeclaration --
> CSSMutableStyleDeclaration could just call a function defined there. That might
> make it easier to keep the list of shorthands in sync with the other lists of
> shorthands.

Agreed, but I think that can (and should) be a part of bug 12159.

> That technique is not reliable. It relies on the four arrays being consecutive
> in global data, something that is not guaranteed by the C++ language.

Fixed.

I also updated the set of properties in this patch to be exactly those properties
that are parsed as shorthands. Interestingly enough (for me) the list doesn't include
'font', '-webkit-border-radius' and 'overflow'.

Added change log and a test.

[Darin Adler]

Comment on attachment 12303 [details]
Expand shorthands for removal

r=me

[Mark Rowe (bdash)]

Landed in r18753.