Bug 104462 – Switch to new PseudoElement based :before and :after

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Switch to new PseudoElement based :before and :after

[Elliott Sprehn]

Created attachment 178390 [details]
Patch

[Elliott Sprehn]

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! :)

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Sam Weinig]

Please make sure Antti has a chance to look at this before committing anything.

[Elliott Sprehn]

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.

[Elliott Sprehn]

Created attachment 178616 [details]
Patch

[Elliott Sprehn]

Created attachment 178631 [details]
Patch

[Elliott Sprehn]

Created attachment 178634 [details]
Patch

[Elliott Sprehn]

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.

[Eric Seidel (no email)]

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?

[Antti Koivisto]

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.

[Antti Koivisto]

Currently ComposedShadowTreeWalker can be compiled completely out when SHADOW_DOM is not enabled (except for <details> element). It looks like this makes it unavoidable.

[Eric Seidel (no email)]

(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. :(

[Antti Koivisto]

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.

[Antti Koivisto]

Comment on attachment 178634 [details]
Patch

Talked with Elliott and Morrita and it sounds like my concerns are being addressed. Switching back to r+.

[Elliott Sprehn]

(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.

[Elliott Sprehn]

(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.

[WebKit Review Bot]

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

[Elliott Sprehn]

(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

[WebKit Review Bot]

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

[Ojan Vafai]

Dirk, any guesses what's going wrong here on the commit queue? Looks like someone broke check-webkit-style?

[Dirk Pranke]

(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.

[Elliott Sprehn]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Eric, do you have any ideas off the top of your head what might be going on w/ the style bot?

[Dirk Pranke]

Committed r137336: <http://trac.webkit.org/changeset/137336>

[Dirk Pranke]

Filed https://bugs.webkit.org/show_bug.cgi?id=104687 to track the CQ problem ... I landed this patch by hand.