| Summary: | CSS 2.1 failure: content-* | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | REOPENED --- | ||||||||||||
| Severity: | Normal | CC: | carol, commit-queue, esprehn, hyatt, mitz, peter, simon.fraser | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Bug Depends on: | 6503, 53024, 53037 | ||||||||||||
| Bug Blocks: | 47141 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Simon Fraser (smfr)
2011-01-08 21:04:01 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? Prefix them with http://test.csswg.org/suites/css2.1/20110111/ and append ".htm" 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. 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. 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 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.
Created attachment 81241 [details]
Proposed Patch. Ensured only the legal elements and pseudoElements (according to CSS2.1) get counters.
(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 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.
(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. Created attachment 81291 [details]
Patch. Made the change suggested by David Dyatt
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 Created attachment 81305 [details]
Patch. Fixed MacWarnings.
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 on attachment 81305 [details] Patch. Fixed MacWarnings. Clearing flags on attachment: 81305 Committed r77702: <http://trac.webkit.org/changeset/77702> All reviewed patches have been landed. Closing bug. Not all tests were fixed, only the counters related ones. Patch to follow for the quotes related issues. 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. (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 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. |