Bug 7118 – Property values with extra items do not get treated as invalid (they should)

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

[Dave Hyatt]

www.mingw.org looks very bad in Safari.  The middle column is way too narrow.  It looks fine in Konq, FFX and WinIE.

[Dave Hyatt]

This may be a problem of margin + float left width instead of MAX(float left width, margin).

[Dave Hyatt]

This bug scares me, since I think it might be a regression from 1.2.

[Alexey Proskuryakov]

I see the same problem with Safari 1.2.4.

[Joost de Valk (AlthA)]

Created attachment 6318 [details]
Testcase

We do something wrong here....

[Joost de Valk (AlthA)]

The testcase i just attached shows that the 14em auto; on #Content is the faulty one here. We seem to simply ignore the "auto" part.

[Joost de Valk (AlthA)]

Created attachment 6320 [details]
Even further reduced testcase

[Dave Hyatt]

So this is technically illegal but I guess we should match FFX and WinIE and use the last value.  AlthA, can you verify that if 3, 4, 5, etc. values are given that FFX and WinIE always pick the last one?

[Dave Hyatt]

Never mind, I get it now.  We're incorrectly accepting this invalid property.  If we rejected it, then we'd use auto for margin-left.  This is our bug.

[Joost de Valk (AlthA)]

Created attachment 6321 [details]
testcase to show bug

Ok the bug is simple, we do not invalidate wrong css values correctly.

[Joost de Valk (AlthA)]

Created attachment 6323 [details]
Newer testcase

This shows that some values get properly invalidated and others do not...

[Joost de Valk (AlthA)]

Created attachment 6326 [details]
"final" version

[Joost de Valk (AlthA)]

Created attachment 6330 [details]
Easier to understand version

As ap commented "is auto garbage here?" now it's garbage all over ;)

[Alexey Proskuryakov]

Created attachment 7446 [details]
proposed fix

Couldn't come up with any smart fix, so just checking the number of values in each case. As for layout tests, I think that both '"final" version' and 'Easier to understand version' should be landed, as "garbage" and "auto" are quite different values.

[Darin Adler]

Comment on attachment 7446 [details]
proposed fix

Since most properties take one value, we could make this nicer by creating table of the properties that accept a number of values other than 1. Then we could do the check against 1 at the top of the function rather than sprinkling the entire thing with if (num != 1).

[Alexey Proskuryakov]

Created attachment 7459 [details]
revised fix

For cases that only set valid_primitive, moved the num==1 check to the end of the function, where parsedValue is created. This has made the patch much nicer.

[Darin Adler]

Comment on attachment 7459 [details]
revised fix

I think this patch is great; headed in the right direction, but not ready to land yet.

Why is there any need to check "num" in functions that call parseShorthand? It seems to me that parseShorthand is already guaranteed to do the right thing; it can't possibly return true if there are more values than properties.

Could we perhaps fix the whole bug another way? Instead of looking at valueList->size(), could we look at valueList->current() at the end of the function to see if it's 0? That seems to go more with the flow of the code. Seems like that would work for everything except for the very few places that explicitly call return true.

What about the CSS_VAL_INHERIT and CSS_VAL_INITIAL cases? Don't those need one of these checks?

What about the SVG properties? Those should have the same issue, but they're in a different source file.

[Alexey Proskuryakov]

Created attachment 7490 [details]
revised fix

> Why is there any need to check "num" in functions that call parseShorthand? It
> seems to me that parseShorthand is already guaranteed to do the right thing; it
> can't possibly return true if there are more values than properties.

  Indeed, but turns out that it has a slightly different bug - it returns false, but the properties stay applied. Thanks for noticing! Added a fix (in CSSGrammar.y) and a test.

  Also added a class to automatically match enterShortcut/exitShortcut pairs; hopefully there's no hidden drawback in doing so.

> Could we perhaps fix the whole bug another way? Instead of looking at
> valueList->size(), could we look at valueList->current() at the end of the
> function to see if it's 0? That seems to go more with the flow of the code.

  Ok, done.

> What about the CSS_VAL_INHERIT and CSS_VAL_INITIAL cases? Don't those need one
> of these checks?

  Fixed (a num == 1 check seemed more appropriate here).

> What about the SVG properties? Those should have the same issue, but they're in
> a different source file.

  The fix would be similar, but there's no test case to validate it, so I was going to open a new bug for SVG.

[Darin Adler]

Comment on attachment 7490 [details]
revised fix

Looks great.

+    int num = inShorthand() ? 1 : valueList->size();

It's a little strange here -- people never really want to know the size, they just want to know if it's greater than 1, 2, or greater. But if the value list happened to be huge we'd walk the entire list. On the one hand, I don't think this is the only n^2 algorithm here, but on the other it would be slightly better not to do this (here or the other places in the file where we already do this). It's faster to check 1 vs. not-1 than it is to get num, etc.

ShorthandScope is a great idea. I'd like ShorthandScope to be private to the .cpp file if possible. The only reason to have it in the .h file would be if we need to share with some other source file.

+                if (!(m_parser->m_inParseShorthand++))
+                    m_parser->m_currentShorthand = propId;

I think the way this works is strange. Should it really just leave m_currentShorthand alone if we're already in a shorthand? It seems to me that either we need to change this to save and restore m_currentShorthand (which is easy now that you have the ShorthandScope object) or maybe just change m_inParseShorthand to a boolean or something. This is sloppy the way it is right now, although it's not your fault. You made things better, not worse, with your patch.

I think I'll r+ even though I have these comments.

[Alexey Proskuryakov]

(In reply to comment #18)
> But if the value list happened to be huge we'd walk the entire list.

  ValueList is implemented with a Vector, does it really have this problem?

> ShorthandScope is a great idea. I'd like ShorthandScope to be private to the
> .cpp file if possible.

  Ok, putting it in an anonymous namespace.

> +                if (!(m_parser->m_inParseShorthand++))
> +                    m_parser->m_currentShorthand = propId;
> 
> I think the way this works is strange. 

  I have convinced myself that this is actually correct behavior :). For example, when parsing CSS_PROP_BORDER, we descend into CSS_PROP_BORDER_WIDTH, which is also a shorthand - but the actual shorthand that needs to be passed to CSSProperty constructor is still the former.

  Filed bug 8170 for SVG.

[Darin Adler]

(In reply to comment #19)
> (In reply to comment #18)
> > But if the value list happened to be huge we'd walk the entire list.
> 
>   ValueList is implemented with a Vector, does it really have this problem?

No. It doesn't. My mistake. ValueList is a terrible name.

> > ShorthandScope is a great idea. I'd like ShorthandScope to be private to the
> > .cpp file if possible.
> 
>   Ok, putting it in an anonymous namespace.

I don't think that's a good idea. It makes debugging harder and has little benefit since we're already inside the WebCore namespace.

> > +                if (!(m_parser->m_inParseShorthand++))
> > +                    m_parser->m_currentShorthand = propId;
> > 
> > I think the way this works is strange. 
> 
>   I have convinced myself that this is actually correct behavior :). For
> example, when parsing CSS_PROP_BORDER, we descend into CSS_PROP_BORDER_WIDTH,
> which is also a shorthand - but the actual shorthand that needs to be passed to
> CSSProperty constructor is still the former.

Sounds good.