Bug 7118 - Property values with extra items do not get treated as invalid (they should)
Summary: Property values with extra items do not get treated as invalid (they should)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL: http://mingw.org/
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2006-02-06 18:36 PST by Dave Hyatt
Modified: 2006-04-04 12:00 PDT (History)
4 users (show)

See Also:


Attachments
Testcase (451 bytes, text/html)
2006-02-07 00:01 PST, Joost de Valk (AlthA)
no flags Details
Even further reduced testcase (382 bytes, text/html)
2006-02-07 00:05 PST, Joost de Valk (AlthA)
no flags Details
testcase to show bug (1.10 KB, text/html)
2006-02-07 01:39 PST, Joost de Valk (AlthA)
no flags Details
Newer testcase (1.73 KB, text/html)
2006-02-07 02:20 PST, Joost de Valk (AlthA)
no flags Details
"final" version (2.61 KB, text/html)
2006-02-07 03:50 PST, Joost de Valk (AlthA)
no flags Details
Easier to understand version (2.70 KB, text/html)
2006-02-07 09:29 PST, Joost de Valk (AlthA)
no flags Details
proposed fix (30.47 KB, patch)
2006-04-01 13:37 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
revised fix (20.69 KB, patch)
2006-04-02 06:59 PDT, Alexey Proskuryakov
darin: review-
Details | Formatted Diff | Diff
revised fix (27.88 KB, patch)
2006-04-03 13:06 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2006-02-06 18:36:30 PST
www.mingw.org looks very bad in Safari.  The middle column is way too narrow.  It looks fine in Konq, FFX and WinIE.
Comment 1 Dave Hyatt 2006-02-06 18:42:31 PST
This may be a problem of margin + float left width instead of MAX(float left width, margin).
Comment 2 Dave Hyatt 2006-02-06 18:44:28 PST
This bug scares me, since I think it might be a regression from 1.2.
Comment 3 Alexey Proskuryakov 2006-02-06 21:56:27 PST
I see the same problem with Safari 1.2.4.
Comment 4 Joost de Valk (AlthA) 2006-02-07 00:01:09 PST
Created attachment 6318 [details]
Testcase

We do something wrong here....
Comment 5 Joost de Valk (AlthA) 2006-02-07 00:04:00 PST
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.
Comment 6 Joost de Valk (AlthA) 2006-02-07 00:05:24 PST
Created attachment 6320 [details]
Even further reduced testcase
Comment 7 Dave Hyatt 2006-02-07 01:20:07 PST
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?
Comment 8 Dave Hyatt 2006-02-07 01:23:29 PST
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.
Comment 9 Joost de Valk (AlthA) 2006-02-07 01:39:50 PST
Created attachment 6321 [details]
testcase to show bug

Ok the bug is simple, we do not invalidate wrong css values correctly.
Comment 10 Joost de Valk (AlthA) 2006-02-07 02:20:24 PST
Created attachment 6323 [details]
Newer testcase

This shows that some values get properly invalidated and others do not...
Comment 11 Joost de Valk (AlthA) 2006-02-07 03:50:22 PST
Created attachment 6326 [details]
"final" version
Comment 12 Joost de Valk (AlthA) 2006-02-07 09:29:05 PST
Created attachment 6330 [details]
Easier to understand version

As ap commented "is auto garbage here?" now it's garbage all over ;)
Comment 13 Alexey Proskuryakov 2006-04-01 13:37:06 PST
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.
Comment 14 Darin Adler 2006-04-01 15:19:57 PST
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).
Comment 15 Alexey Proskuryakov 2006-04-02 06:59:07 PDT
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.
Comment 16 Darin Adler 2006-04-02 16:28:43 PDT
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.
Comment 17 Alexey Proskuryakov 2006-04-03 13:06:02 PDT
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.
Comment 18 Darin Adler 2006-04-03 14:59:25 PDT
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.
Comment 19 Alexey Proskuryakov 2006-04-03 22:30:14 PDT
(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.
Comment 20 Darin Adler 2006-04-04 12:00:27 PDT
(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.