Bug 13097 – Unsupported content:inherit value

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[David Latapie]

WebKit ToT does not recognise content:inherit, at least on hreflang. See attached test case.

[David Latapie]

Created attachment 13677 [details]
Reduced test case.

[Dave Hyatt]

Not a regression.

[Dave Hyatt]

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

[David Kilzer (:ddkilzer)]

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

[Vincent Ricard]

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.

[Darin Adler]

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.

See also bug 20032.

[Nicholas Wilson]

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.

[Nicholas Wilson]

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.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Brent Fulgham]

It doesn't seem like 'content' supports 'inherit' in the current spec.