Bug 52126 – CSS 2.1 failure: content-*

• Component: CSS
• Status: REOPENED

[Simon Fraser (smfr)]

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.

[Carol Szabo]

(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?

[Simon Fraser (smfr)]

Prefix them with http://test.csswg.org/suites/css2.1/20110111/ and append ".htm"

[Carol Szabo]

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.

[Carol Szabo]

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.

[Carol Szabo]

Created attachment 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.

[Dave Hyatt]

Comment on attachment 80970 [details]
Proposed Patch

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.

[Carol Szabo]

Created attachment 81241 [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.

[Carol Szabo]

(In reply to comment #6)
> (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.

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.

[Dave Hyatt]

Comment on attachment 81241 [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.

r=me

Maybe rename the debugging function to showCounterRendererTree instead of just showRendererTree.

[Dave Hyatt]

(In reply to comment #8)
> (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).
> 

And good, that's all I was checking for.

[Carol Szabo]

Created attachment 81291 [details]
Patch. Made the change suggested by David Dyatt

[WebKit Commit Bot]

Comment on attachment 81291 [details]
Patch. Made the change suggested by David Dyatt

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

[Carol Szabo]

Created attachment 81305 [details]
Patch. Fixed MacWarnings.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 81305 [details]:

media/invalid-media-url-crash.html bug 51138 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 81305 [details]
Patch. Fixed MacWarnings.

Clearing flags on attachment: 81305

Committed r77702: <http://trac.webkit.org/changeset/77702>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carol Szabo]

Not all tests were fixed, only the counters related ones. Patch to follow for the quotes related issues.

[Peter Beverloo]

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.

[Simon Fraser (smfr)]

(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.

[Eric Seidel (no email)]

Comment on attachment 81241 [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.

Cleared David Hyatt's review+ from obsolete attachment 81241 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ahmad Saleem]

These tests are also now on WPT - https://wpt.fyi/results/css/CSS2/generated-content?label=master&label=experimental&aligned&q=content-

I noticed that Safari 15.6 do fail on few content-* tests but still doing better than other browsers. Just wanted to share updated status. Thanks!

[alan]

(In reply to Ahmad Saleem from comment #21)
> These tests are also now on WPT -
> https://wpt.fyi/results/css/CSS2/generated-
> content?label=master&label=experimental&aligned&q=content-
> 
> I noticed that Safari 15.6 do fail on few content-* tests but still doing
> better than other browsers. Just wanted to share updated status. Thanks!
Thank you!