Summary: | Unsupported content:inherit value | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Latapie <david> | ||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED INVALID | ||||||||||
Severity: | Normal | CC: | bfulgham, emacemac7, mitz, nicholas, nickshanks, robert, syoichi, webkit, webkit.review.bot | ||||||||
Priority: | P3 | ||||||||||
Version: | 523.x (Safari 3) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
URL: | http://test.csswg.org/harness/test/CSS21_DEV/single/content-inherit-001/ | ||||||||||
Attachments: |
|
Description
David Latapie
2007-03-16 18:19:27 PDT
Created attachment 13677 [details]
Reduced test case.
Not a regression. *** Bug 13191 has been marked as a duplicate of this bug. *** *** Bug 13200 has been marked as a duplicate of this bug. *** Created attachment 20372 [details]
Implements 'inherit' value for 'content'
This patch implements the 'inherit' value for 'content'.
I let the note about CSS3... if this patch is correct, we could remove it.
I commented the switch case about the CONTENT_QUOTE, waiting my patch for #6503 is landed.
Comment on attachment 20372 [details]
Implements 'inherit' value for 'content'
Thanks for taking this on. Looks like a good first cut.
A few things need fixing.
If you're going to implement inherit, then you need to remove the following now-inaccurate comment:
// FIXME: In CSS3, it will be possible to inherit content. In CSS2 it is not. This
// note is a reminder that eventually "inherit" needs to be supported.
And replace it with a new one.
+/* case CONTENT_QUOTE:
+ m_style->setContent(content->m_content.m_quote);
+ break;*/
We don't check in commented-out code like this in the WebKit project.
The test case is inadequate. For example, it doesn't exercise the CONTENT_OBJECT or CONTENT_COUNTER cases in the newly-added switch statement. The only type of inheritance it checks is inheriting nothing.
Also, are we guaranteed that m_parentStyle is non-0? I think you could probably create a test case where content: inherit is in a style without a parent, and then we'd dereference null in the inherit code.
review- because of these issues.
I am afraid to say that this behaviour is actually wrong. It seems sensible that inherit should inherit, but there are a lot of non-obvious decisions about how to treat replaced content. In CSS3, the current public Gen-Con draft is very incomplete, but does not want does not want to allow inheritance (please comment if you know personally of any move to change this). On the other hand, in CSS2.1, 'inherit' is basically just there as a placeholder value that nearly any property can have set. It it is not intended to have any function for "content" and the spec requires none, normal, and inherit to all work the same way. Patch on its way. Created attachment 53180 [details] Rounds off support for CSS2.1 content This should go a way towards the various bugs around this (fixes bug 20032, bug 18587). 2.1 spec a little vague, but I have thought the behaviour through and am willing to explain further why I think this is the correct interpretation. Test case and code has some explanation to check first. Have worked on various projects, but this is my first time tinkering with any browser, so any problems are due to sheer ignorance. Attachment 53180 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:633: One space before end of line comments [whitespace/comments] [5]
WebCore/css/CSSParser.cpp:634: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
WebCore/css/CSSParser.cpp:639: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 3 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 53180 [details] Rounds off support for CSS2.1 content Looks good. > + if (id == CSSValueNormal || > + id == CSSValueNone) > + validPrimitive = true; > + else { > + return parseContent(propId, important); > + } No braces around a single-line else. No need to break that up into multiple lines. I suggest this formatting: if (id == CSSValueNormal || id == CSSValueNone) validPrimitive = true; else return parseContent(propId, important) break; review- because this patch includes a new regression test but does not include the expected result for the test It would also be best if the test could be changed so that it can be dumpAsText instead, as we have done for many style tests in the past. But what's mandatory is including the expected results. It doesn't seem like 'content' supports 'inherit' in the current spec. |