Bug 52126 - CSS 2.1 failure: content-*
: CSS 2.1 failure: content-*
Status: REOPENED
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 6503 53024 53037
: 47141
  Show dependency treegraph
 
Reported: 2011-01-08 21:04 PST by
Modified: 2013-01-16 18:46 PST (History)


Attachments
Proposed Patch (20.61 KB, patch)
2011-02-02 14:22 PST, Carol Szabo
hyatt: review-
hyatt: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters. (20.81 KB, patch)
2011-02-04 10:14 PST, Carol Szabo
no flags Review Patch | Details | Formatted Diff | Diff
Patch. Made the change suggested by David Dyatt (20.81 KB, patch)
2011-02-04 14:50 PST, Carol Szabo
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch. Fixed MacWarnings. (21.65 KB, patch)
2011-02-04 15:29 PST, Carol Szabo
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-08 21:04:01 PST
The following CSS 2.1 tests fail:

html4/content-021
html4/content-022
html4/content-023
html4/content-024
html4/content-025
html4/content-026
html4/content-027
html4/content-028
html4/content-029
html4/content-030
html4/content-031
html4/content-032
html4/content-033
html4/content-034
html4/content-035
html4/content-056
html4/content-072
html4/content-151
html4/content-156
html4/content-157
html4/content-158
html4/content-159

Many are related to counters in content.
------- Comment #1 From 2011-01-14 12:58:23 PST -------
(In reply to comment #0)
> The following CSS 2.1 tests fail:
> 
> html4/content-021
> html4/content-022
> html4/content-023
> html4/content-024
> html4/content-025
> html4/content-026
> html4/content-027
> html4/content-028
> html4/content-029
> html4/content-030
> html4/content-031
> html4/content-032
> html4/content-033
> html4/content-034
> html4/content-035
> html4/content-056
> html4/content-072
> html4/content-151
> html4/content-156
> html4/content-157
> html4/content-158
> html4/content-159
> 
> Many are related to counters in content.

Can you provide URL for these?
------- Comment #2 From 2011-01-14 13:37:57 PST -------
Prefix them with http://test.csswg.org/suites/css2.1/20110111/ and append ".htm"
------- Comment #3 From 2011-01-19 15:29:31 PST -------
I have looked at the first failed test and found the root cause:
Currently the counters are based on the renderer hierarchy and not the DOM node hierarchy as the CSS spec requires. These hierarchies are pretty close, so most of the time we get the right result, but when they are not we don't.
In the case of content-021 what happens is that an anonymous block is created inside the renderer of the outer div and the renderer for the before pseudo element of this block is placed inside that anonymous block. This way the counter attached to that node (pseudo node) is out of scope for the before pseudo element of the inner div.
Shall think about whether to change counter behavior to use the DOM tree instead of the render tree or whether to just handle anonymous blocks.
------- Comment #4 From 2011-01-20 09:54:45 PST -------
I have reviewed the documentation and I feel that the right thing to do is to walk the DOM tree instead of the render tree. It does not appear to be too difficult either. I just need to be careful to check for pseudo elements and for display: none and visibility: hidden styles.
------- Comment #5 From 2011-02-02 14:22:09 PST -------
Created an attachment (id=80970) [details]
Proposed Patch

This patch fixes counter problems other problems are related to WebKit not supporting quotes. There is a separate patch to fix that.
------- Comment #6 From 2011-02-04 09:19:19 PST -------
(From update of attachment 80970 [details])
This looks fine... let's just remove the asserts when generatingNode() points you to a non-before/after pseudo, since that's something that we expect will happen as we patch generatingNode() to handle more cases besides before/after.
------- Comment #7 From 2011-02-04 10:14:23 PST -------
Created an attachment (id=81241) [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.
------- Comment #8 From 2011-02-04 10:36:01 PST -------
(In reply to comment #6)
> (From update of attachment 80970 [details] [details])
> This looks fine... let's just remove the asserts when generatingNode() points you to a non-before/after pseudo, since that's something that we expect will happen as we patch generatingNode() to handle more cases besides before/after.

Dave,
The fact that in the general case generatingNode() can return non-null values for renderers attached to other types of pseudoElements than :before and :after is irrelevant here (at least if I understand your comment correctly).

Counters (which this code is all about - notice that the special tree navigation functions are marked static so they are available only withing the RenderCounter.cpp code) are valid only on elements and :before and :after pseudo elements. This is enforced in the planCounter function (in my latest patch version).
Because of the above stated facts, the default branch of the switch statements should never be reached regardless of the fact that generatingNode() can be called on any node and could return a non-null value for all types of real DOM nodes and pseudoElements. The reason is that we start navigation always from a renderer containing a counter (which is either the renderer of an element or of a before or after pseudo element - see planCounter) and we navigate only elements and before and after pseudoElements.
This is why I believe that the ASSERTS should stay (to force errors if suddenly counters become valid on other types of elements and planCounter is changed to allow that without the proper changes to the navigation algorithm).
On the other hand RenderObject::generatingNode() is not a function tied to counters or content in any obvious way. It allows retrieval of a DOM node generating a renderer potentially for all pseudoElements as needed.
------- Comment #9 From 2011-02-04 14:09:34 PST -------
(From update of attachment 81241 [details])
r=me

Maybe rename the debugging function to showCounterRendererTree instead of just showRendererTree.
------- Comment #10 From 2011-02-04 14:10:21 PST -------
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 80970 [details] [details] [details])
> > This looks fine... let's just remove the asserts when generatingNode() points you to a non-before/after pseudo, since that's something that we expect will happen as we patch generatingNode() to handle more cases besides before/after.
> 
> Dave,
> The fact that in the general case generatingNode() can return non-null values for renderers attached to other types of pseudoElements than :before and :after is irrelevant here (at least if I understand your comment correctly).
> 

And good, that's all I was checking for.
------- Comment #11 From 2011-02-04 14:50:52 PST -------
Created an attachment (id=81291) [details]
Patch. Made the change suggested by David Dyatt
------- Comment #12 From 2011-02-04 15:08:36 PST -------
(From update of attachment 81291 [details])
Rejecting attachment 81291 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build'..." exit_code: 2

Last 500 characters of output:
ebCore:
    CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/CounterNode.o /mnt/git/webkit-commit-queue/Source/WebCore/rendering/CounterNode.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
    CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/RenderCounter.o /mnt/git/webkit-commit-queue/Source/WebCore/rendering/RenderCounter.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(2 failures)


Full output: http://queues.webkit.org/results/7704005
------- Comment #13 From 2011-02-04 15:29:19 PST -------
Created an attachment (id=81305) [details]
Patch. Fixed MacWarnings.
------- Comment #14 From 2011-02-04 16:26:25 PST -------
The commit-queue encountered the following flaky tests while processing attachment 81305 [details]:

media/invalid-media-url-crash.html bug 51138 (author: inferno@chromium.org)
The commit-queue is continuing to process your patch.
------- Comment #15 From 2011-02-04 16:29:12 PST -------
(From update of attachment 81305 [details])
Clearing flags on attachment: 81305

Committed r77702: <http://trac.webkit.org/changeset/77702>
------- Comment #16 From 2011-02-04 16:29:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #17 From 2011-02-07 12:39:18 PST -------
Not all tests were fixed, only the counters related ones. Patch to follow for the quotes related issues.
------- Comment #18 From 2011-02-07 12:51:32 PST -------
Bug 3234 and specifically bug 6503 specifically talk about the support for quotes in CSS. It may be better to use bug 6503 for follow-up patches, as this one addressed the counter issue.
------- Comment #19 From 2011-02-07 13:23:22 PST -------
(In reply to comment #18)
> Bug 3234 and specifically bug 6503 specifically talk about the support for quotes in CSS. It may be better to use bug 6503 for follow-up patches, as this one addressed the counter issue.

I agree. When I filed this bug I didn't rigorously check the offending tests; feel free to spin off different bugs for remaining issues.
------- Comment #20 From 2011-04-06 10:47:19 PST -------
(From update of attachment 81241 [details])
Cleared David Hyatt's review+ from obsolete attachment 81241 [details] so that this bug does not appear in http://webkit.org/pending-commit.