604505 - inheriting 'content' from an element should always inherit none/normal since the computed value on elements is always 'normal'

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Inheriting 'content' from an element should always inherit none/normal since the computed value on elements is always 'normal'.

http://test.csswg.org/suites/css2.1/20101001/html4/content-160.htm
http://test.csswg.org/suites/css2.1/20101001/xhtml1/content-160.xht

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Was this a spec change at some point?

But yeah, we should fix.  And fix the tests we have that will fail as a result... and maybe the ones that will stop testing what they're supposed to be testing as a result.  :(

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also:
http://test.csswg.org/suites/css2.1/20110111/html4/content-computed-value-001.htm
(a test introduced after RC1 of the test suite)

Comment 3

[Gérard Talbot]

http://test.csswg.org/suites/css2.1/latest/html4/content-160.htm

http://test.csswg.org/suites/css2.1/latest/xhtml1/content-160.xht

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9c949b922918

It seems that we have a lot of tests to fix for this bug. In addition, the test for bug 569006 uses this wrong behavior. I'm not sure whether we should get rid of that test or do something different.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8525062 [details] [diff] [review]
patch

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>@@ -7977,18 +7977,22 @@ nsRuleNode::ComputeContentData(void* aSt
>                                const bool aCanStoreInRuleTree)
> {
>   uint32_t count;
>   nsAutoString buffer;
> 
>   COMPUTE_START_RESET(Content, (), content, parentContent)
> 
>   // content: [string, url, counter, attr, enum]+, normal, none, inherit
>-  const nsCSSValue* contentValue = aRuleData->ValueForContent();
>-  switch (contentValue->GetUnit()) {
>+  nsCSSValue contentValue(*aRuleData->ValueForContent());
>+  if (!aContext->GetPseudo()) {
>+    // On elements, content is always computed to normal.
>+    contentValue.SetNormalValue();
>+  }
>+  switch (contentValue.GetUnit()) {


This isn't safe, since an element and a pseudo-element might match the same list of rules and thus share the same rule node.

It's also bad because it breaks all partial computation from a start struct (i.e., when ComputeContentData has a non-null aStartStruct, aRuleData only contains the declarations that should override what is already in aStartStruct, and here you're overriding eCSSUnit_Null (which says there's nothing specified and thus you shouldn't override what's already there).

Both of these could cause test failures.

I need to think a little more about the best way around the first problem, though.  (Fixing the second is easy.)

Comment 7

[Cameron McCormack (:heycam)]

I think this should be WONTFIX (or INVALID), given that bug 215083 needs us to have a computed value other than normal on an element.  Does that sounds right Xidorn?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Yeah, INVALID I guess. The spec is still very unclear about many cases, but given that values no longer need to be computed to normal for elements, the description of this bug no longer applies.