|Summary:||CSS 2.1 failure: content-*|
|Product:||WebKit||Reporter:||Simon Fraser (smfr) <simon.fraser>|
|Severity:||Normal||CC:||carol, commit-queue, esprehn, hyatt, mitz, peter, simon.fraser|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:||6503, 53024, 53037|
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: firstname.lastname@example.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
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.