Switch to new PseudoElement based :before and :after
Created attachment 178390 [details] Patch
This is going to fail fast/css-generated-content/table-row-group-to-inline.html, I didn't add the new baseline to the patch yet. I did manage to fix the selection bugs though! :)
Comment on attachment 178390 [details] Patch Attachment 178390 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15210804 New failing tests: fast/css-generated-content/before-content-continuation-chain.html fast/css-generated-content/table-row-group-to-inline.html
Comment on attachment 178390 [details] Patch Attachment 178390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15218512 New failing tests: fast/forms/week/week-appearance-pseudo-elements.html fast/forms/time/time-appearance-pseudo-elements.html fast/forms/month/month-appearance-pseudo-elements.html fast/css-generated-content/before-content-continuation-chain.html fast/css-generated-content/table-row-group-to-inline.html fast/forms/date/date-appearance-pseudo-elements.html
Comment on attachment 178390 [details] Patch Attachment 178390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15228102 New failing tests: fast/forms/week/week-appearance-pseudo-elements.html fast/forms/time/time-appearance-pseudo-elements.html fast/forms/month/month-appearance-pseudo-elements.html fast/css-generated-content/before-content-continuation-chain.html fast/css-generated-content/table-row-group-to-inline.html fast/forms/date/date-appearance-pseudo-elements.html
Please make sure Antti has a chance to look at this before committing anything.
Here's a bug this also fixes, I'll add a test: https://code.google.com/p/chromium/issues/detail?id=164820 This is also fixes our support for display block generated content on display inline things: <style> span:before { display: block; content: "should be on it's own line"; } </style> <span>and this too</span> I'll add a test for that as well.
Created attachment 178616 [details] Patch
Created attachment 178631 [details] Patch
Created attachment 178634 [details] Patch
Once this is landed I can create a tiny patch that removes the if statement from AnimationController.cpp and adds some tests and then we'll have animations on :before and :after as well. The new :before/:after implementation supports animations, but I wanted to turn them on separately.
Comment on attachment 178634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178634&action=review This looks great. Thanks. > Source/WebCore/rendering/RenderObject.h:629 > + // FIXME: Why does RenderWidget need this? > + void clearNode() { m_node = 0; } It shouldn't. RenderWidget outlives the Rendering Tree (and Document) sometimes, as I believe plugins can hold onto it during their teardown?
Comment on attachment 178634 [details] Patch Doesn't this directly contradict what is being done in bug 103339 by making more of the internals depend on Shadow DOM? r- until this is clarified.
Currently ComposedShadowTreeWalker can be compiled completely out when SHADOW_DOM is not enabled (except for <details> element). It looks like this makes it unavoidable.
(In reply to comment #14) > Currently ComposedShadowTreeWalker can be compiled completely out when SHADOW_DOM is not enabled (except for <details> element). It looks like this makes it unavoidable. I see. SHADOW_DOM is in a confusing state it seems. Fighting to be most elegantly integrated, and yet still desired to be compiled completely out. :(
This shows why getting SHADOW_DOM conditionals in place is so important. We need to have clear understanding where the boundary between core WebKit code and the experimental feature exactly is. The standard for former are different than for the latter.
Comment on attachment 178634 [details] Patch Talked with Elliott and Morrita and it sounds like my concerns are being addressed. Switching back to r+.
(In reply to comment #12) > (From update of attachment 178634 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178634&action=review > > This looks great. Thanks. > > > Source/WebCore/rendering/RenderObject.h:629 > > + // FIXME: Why does RenderWidget need this? > > + void clearNode() { m_node = 0; } > > It shouldn't. RenderWidget outlives the Rendering Tree (and Document) sometimes, as I believe plugins can hold onto it during their teardown? Hmm okay, I'll try to remove this in a separate patch.
(In reply to comment #17) > (From update of attachment 178634 [details]) > Talked with Elliott and Morrita and it sounds like my concerns are being addressed. Switching back to r+. So to reiterate the reason the patch looks the way it does: input:before {} or input:after {} needs ComposedShadowTreeWalker behavior so that the pseudo elements end up down inside the input shadow tree visually and placed before their "siblings" in the composed shadow tree sense. I had tried to disable generated content for shadows, but it seems some sites depend on this for now, and we should do that separately from getting the new generated content turned on which is a big win. If we didn't add methods to CSTW in this patch we'd need two separate almost identical traversals, one with CSTW and one that manually walks down into userAgentShadowRoot()'s to make inputs work. Once this is landed morrita will refactor the code so that we have a reusable traversal that can turn on and off the insertion point related things so you can compile out CSTW if you need to.
Comment on attachment 178634 [details] Patch Rejecting attachment 178634 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: __ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] Total errors found: 2 in 2 files Full output: http://queues.webkit.org/results/15238879
(In reply to comment #20) > (From update of attachment 178634 [details]) > Rejecting attachment 178634 [details] from commit-queue. > > ... > Updating webkit projects from gyp files... > LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] > LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] > Total errors found: 2 in 2 files Um, are the bots broken? These files definitely exist in svn: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/TestExpectations http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/TestExpectations
Comment on attachment 178634 [details] Patch Rejecting attachment 178634 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: __ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] Total errors found: 2 in 2 files Full output: http://queues.webkit.org/results/15259318
Dirk, any guesses what's going wrong here on the commit queue? Looks like someone broke check-webkit-style?
(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 178634 [details] [details]) > > Rejecting attachment 178634 [details] [details] from commit-queue. > > > > ... > > Updating webkit projects from gyp files... > > LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] > > LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] > > Total errors found: 2 in 2 files > > Um, are the bots broken? These files definitely exist in svn: > > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/TestExpectations > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/TestExpectations The messages aren't saying that the expectations files themselves are missing, they're saying the tests on lines 143/398 are missing. Unfortunately, the test name isn't being printed out by the style checker, but it's animations/animation-api-1.html (from what rwt --lint-test-files tells me). I'm not sure why the style checker is complaining about this, though, since it seems to be a pre-existing error.
(In reply to comment #24) > (In reply to comment #21) > > (In reply to comment #20) > > > (From update of attachment 178634 [details] [details] [details]) > > > Rejecting attachment 178634 [details] [details] [details] from commit-queue. > > > > > > ... > > > Updating webkit projects from gyp files... > > > LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] > > > LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] > > > Total errors found: 2 in 2 files > > > > Um, are the bots broken? These files definitely exist in svn: > > > > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/TestExpectations > > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/TestExpectations > > The messages aren't saying that the expectations files themselves are missing, they're saying the tests on lines 143/398 are missing. Unfortunately, the test name isn't being printed out by the style checker, but it's animations/animation-api-1.html (from what rwt --lint-test-files tells me). > > I'm not sure why the style checker is complaining about this, though, since it seems to be a pre-existing error. Heh, so what should I do to get this landed? That line isn't even in my patch.
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > (From update of attachment 178634 [details] [details] [details] [details]) > > > > Rejecting attachment 178634 [details] [details] [details] [details] from commit-queue. > > > > > > > > ... > > > > Updating webkit projects from gyp files... > > > > LayoutTests/platform/chromium/TestExpectations:143: Path does not exist. [test/expectations] [5] > > > > LayoutTests/platform/mac/TestExpectations:398: Path does not exist. [test/expectations] [5] > > > > Total errors found: 2 in 2 files > > > > > > Um, are the bots broken? These files definitely exist in svn: > > > > > > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/TestExpectations > > > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/TestExpectations > > > > The messages aren't saying that the expectations files themselves are missing, they're saying the tests on lines 143/398 are missing. Unfortunately, the test name isn't being printed out by the style checker, but it's animations/animation-api-1.html (from what rwt --lint-test-files tells me). > > > > I'm not sure why the style checker is complaining about this, though, since it seems to be a pre-existing error. > > Heh, so what should I do to get this landed? That line isn't even in my patch. I'll land the patch manually for you.
Eric, do you have any ideas off the top of your head what might be going on w/ the style bot?
Committed r137336: <http://trac.webkit.org/changeset/137336>
Filed https://bugs.webkit.org/show_bug.cgi?id=104687 to track the CQ problem ... I landed this patch by hand.