Bug 104462 - Switch to new PseudoElement based :before and :after
Summary: Switch to new PseudoElement based :before and :after
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 98687 92591 95117 104595
  Show dependency treegraph
 
Reported: 2012-12-08 19:37 PST by Elliott Sprehn
Modified: 2012-12-13 23:08 PST (History)
13 users (show)

See Also:


Attachments
Patch (41.90 KB, patch)
2012-12-08 19:50 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (62.10 KB, patch)
2012-12-10 13:15 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (61.65 KB, patch)
2012-12-10 13:56 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (61.76 KB, patch)
2012-12-10 14:34 PST, Elliott Sprehn
koivisto: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-12-08 19:37:22 PST
Switch to new PseudoElement based :before and :after
Comment 1 Elliott Sprehn 2012-12-08 19:50:04 PST
Created attachment 178390 [details]
Patch
Comment 2 Elliott Sprehn 2012-12-08 19:52:15 PST
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 3 Build Bot 2012-12-09 00:39:48 PST
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 4 WebKit Review Bot 2012-12-09 01:02:37 PST
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 5 WebKit Review Bot 2012-12-09 02:07:39 PST
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
Comment 6 Sam Weinig 2012-12-09 17:34:30 PST
Please make sure Antti has a chance to look at this before committing anything.
Comment 7 Elliott Sprehn 2012-12-09 21:06:10 PST
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.
Comment 8 Elliott Sprehn 2012-12-10 13:15:33 PST
Created attachment 178616 [details]
Patch
Comment 9 Elliott Sprehn 2012-12-10 13:56:44 PST
Created attachment 178631 [details]
Patch
Comment 10 Elliott Sprehn 2012-12-10 14:34:48 PST
Created attachment 178634 [details]
Patch
Comment 11 Elliott Sprehn 2012-12-10 14:47:38 PST
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 12 Eric Seidel (no email) 2012-12-10 17:48:11 PST
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 13 Antti Koivisto 2012-12-10 17:58:21 PST
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.
Comment 14 Antti Koivisto 2012-12-10 18:03:31 PST
Currently ComposedShadowTreeWalker can be compiled completely out when SHADOW_DOM is not enabled (except for <details> element). It looks like this makes it unavoidable.
Comment 15 Eric Seidel (no email) 2012-12-10 18:38:50 PST
(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. :(
Comment 16 Antti Koivisto 2012-12-10 18:48:36 PST
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 17 Antti Koivisto 2012-12-10 18:59:45 PST
Comment on attachment 178634 [details]
Patch

Talked with Elliott and Morrita and it sounds like my concerns are being addressed. Switching back to r+.
Comment 18 Elliott Sprehn 2012-12-10 20:06:00 PST
(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.
Comment 19 Elliott Sprehn 2012-12-10 20:10:18 PST
(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 20 WebKit Review Bot 2012-12-10 21:16:52 PST
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
Comment 21 Elliott Sprehn 2012-12-10 21:21:23 PST
(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 22 WebKit Review Bot 2012-12-10 21:59:05 PST
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
Comment 23 Ojan Vafai 2012-12-10 22:05:33 PST
Dirk, any guesses what's going wrong here on the commit queue? Looks like someone broke check-webkit-style?
Comment 24 Dirk Pranke 2012-12-11 10:42:17 PST
(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.
Comment 25 Elliott Sprehn 2012-12-11 10:47:04 PST
(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.
Comment 26 Dirk Pranke 2012-12-11 10:49:51 PST
(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.
Comment 27 Dirk Pranke 2012-12-11 10:51:48 PST
Eric, do you have any ideas off the top of your head what might be going on w/ the style bot?
Comment 28 Dirk Pranke 2012-12-11 10:54:25 PST
Committed r137336: <http://trac.webkit.org/changeset/137336>
Comment 29 Dirk Pranke 2012-12-11 10:56:05 PST
Filed https://bugs.webkit.org/show_bug.cgi?id=104687 to track the CQ problem ... I landed this patch by hand.