Bug 104462 - Switch to new PseudoElement based :before and :after
: Switch to new PseudoElement based :before and :after
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 92591 95117 98687 104595
  Show dependency treegraph
 
Reported: 2012-12-08 19:37 PST by
Modified: 2012-12-13 23:08 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-08 19:37:22 PST
Switch to new PseudoElement based :before and :after
------- Comment #1 From 2012-12-08 19:50:04 PST -------
Created an attachment (id=178390) [details]
Patch
------- Comment #2 From 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 From 2012-12-09 00:39:48 PST -------
(From update of attachment 178390 [details])
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 From 2012-12-09 01:02:37 PST -------
(From update of attachment 178390 [details])
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 From 2012-12-09 02:07:39 PST -------
(From update of attachment 178390 [details])
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 From 2012-12-09 17:34:30 PST -------
Please make sure Antti has a chance to look at this before committing anything.
------- Comment #7 From 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 From 2012-12-10 13:15:33 PST -------
Created an attachment (id=178616) [details]
Patch
------- Comment #9 From 2012-12-10 13:56:44 PST -------
Created an attachment (id=178631) [details]
Patch
------- Comment #10 From 2012-12-10 14:34:48 PST -------
Created an attachment (id=178634) [details]
Patch
------- Comment #11 From 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 From 2012-12-10 17:48:11 PST -------
(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?
------- Comment #13 From 2012-12-10 17:58:21 PST -------
(From update of attachment 178634 [details])
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 From 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 From 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 From 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 From 2012-12-10 18:59:45 PST -------
(From update of attachment 178634 [details])
Talked with Elliott and Morrita and it sounds like my concerns are being addressed. Switching back to r+.
------- Comment #18 From 2012-12-10 20:06:00 PST -------
(In reply to comment #12)
> (From update of attachment 178634 [details] [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 From 2012-12-10 20:10:18 PST -------
(In reply to comment #17)
> (From update of attachment 178634 [details] [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 From 2012-12-10 21:16:52 PST -------
(From update of attachment 178634 [details])
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 From 2012-12-10 21:21:23 PST -------
(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
------- Comment #22 From 2012-12-10 21:59:05 PST -------
(From update of attachment 178634 [details])
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 From 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 From 2012-12-11 10:42:17 PST -------
(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.
------- Comment #25 From 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] [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.
------- Comment #26 From 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] [details])
> > > > Rejecting attachment 178634 [details] [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 From 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 From 2012-12-11 10:54:25 PST -------
Committed r137336: <http://trac.webkit.org/changeset/137336>
------- Comment #29 From 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.