Bug 52126

Summary: CSS 2.1 failure: content-*
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: ahmad.saleem792, bfulgham, carol, commit-queue, esprehn, hyatt, mitz, peter, rniwa, simon.fraser, zalan
Priority: P2 Keywords: WPTImpact
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 6503, 53024, 53037    
Bug Blocks: 47141    
Attachments:
Description Flags
Proposed Patch
hyatt: review-, hyatt: commit-queue-
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.
none
Patch. Made the change suggested by David Dyatt
commit-queue: commit-queue-
Patch. Fixed MacWarnings. none

Description Simon Fraser (smfr) 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 Carol Szabo 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 Simon Fraser (smfr) 2011-01-14 13:37:57 PST
Prefix them with http://test.csswg.org/suites/css2.1/20110111/ and append ".htm"
Comment 3 Carol Szabo 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 Carol Szabo 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 Carol Szabo 2011-02-02 14:22:09 PST
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.
Comment 6 Dave Hyatt 2011-02-04 09:19:19 PST
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.
Comment 7 Carol Szabo 2011-02-04 10:14:23 PST
Created attachment 81241 [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.
Comment 8 Carol Szabo 2011-02-04 10:36:01 PST
(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.
Comment 9 Dave Hyatt 2011-02-04 14:09:34 PST
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.
Comment 10 Dave Hyatt 2011-02-04 14:10:21 PST
(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.
Comment 11 Carol Szabo 2011-02-04 14:50:52 PST
Created attachment 81291 [details]
Patch. Made the change suggested by David Dyatt
Comment 12 WebKit Commit Bot 2011-02-04 15:08:36 PST
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
Comment 13 Carol Szabo 2011-02-04 15:29:19 PST
Created attachment 81305 [details]
Patch. Fixed MacWarnings.
Comment 14 WebKit Commit Bot 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 WebKit Commit Bot 2011-02-04 16:29:12 PST
Comment on attachment 81305 [details]
Patch. Fixed MacWarnings.

Clearing flags on attachment: 81305

Committed r77702: <http://trac.webkit.org/changeset/77702>
Comment 16 WebKit Commit Bot 2011-02-04 16:29:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Carol Szabo 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 Peter Beverloo 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 Simon Fraser (smfr) 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 Eric Seidel (no email) 2011-04-06 10:47:19 PDT
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.
Comment 21 Ahmad Saleem 2022-07-24 07:04:25 PDT
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!
Comment 22 zalan 2022-07-24 07:09:17 PDT
(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!