Bug 100812

Summary: [Shadow] Implement custom pseudo elements styling
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: CSSAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarcelo, dglazkov, gustavo, ojan, peter+ews, philn, webcomponents-bugzilla, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 100831    
Bug Blocks: 101170, 101533    
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch
none
Patch none

Shinya Kawanaka
Reported 2012-10-30 21:56:51 PDT
Let's consider the following tree. div#host -- SR |-span (pseudo='x-foo') When we have a CSS selector #host:x-foo, it should match the span.
Attachments
WIP (12.34 KB, patch)
2012-10-31 02:06 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (4.27 KB, patch)
2012-10-31 22:36 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (4.27 KB, patch)
2012-11-02 02:10 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (4.94 KB, patch)
2012-11-02 23:13 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (4.96 KB, patch)
2012-11-04 17:54 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-10-30 23:51:25 PDT
I would like to implement this like shadowPseudoId, since this also breaks shadow boundary.
Shinya Kawanaka
Comment 2 2012-10-31 02:06:33 PDT
Created attachment 171601 [details] WIP
Shinya Kawanaka
Comment 3 2012-10-31 02:08:02 PDT
This contains a lot of debug-printf... But it succeeds in styling custom pseudo elements.
Dimitri Glazkov (Google)
Comment 4 2012-10-31 10:22:04 PDT
I don't understand why you're implementing this again? UnknownPseudoElements and shadowPseudoIds are exactly what you need already.
Dimitri Glazkov (Google)
Comment 5 2012-10-31 10:28:16 PDT
Comment on attachment 171601 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=171601&action=review > Source/WebCore/css/CSSSelector.cpp:395 > + if (name.startsWith("x-")) I could be wrong, but it should be easier to make this work at the attribute-setting time, right?
Shinya Kawanaka
Comment 6 2012-10-31 18:33:25 PDT
First I thought I would like to use the shadowPseudoId, but I was afraid that it causes some confusion. But maybe it's OK to use it anyway...
Shinya Kawanaka
Comment 7 2012-10-31 22:36:51 PDT
Created attachment 171769 [details] Patch
Shinya Kawanaka
Comment 8 2012-10-31 22:37:32 PDT
This patch depends on https://bugs.webkit.org/show_bug.cgi?id=100831, so it won't run until it's landed.
Build Bot
Comment 9 2012-10-31 22:42:21 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14692355
WebKit Review Bot
Comment 10 2012-10-31 22:43:24 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14677391
Early Warning System Bot
Comment 11 2012-10-31 22:45:27 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14697180
Peter Beverloo (cr-android ews)
Comment 12 2012-10-31 22:47:10 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14678390
Early Warning System Bot
Comment 13 2012-10-31 22:47:34 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14684376
EFL EWS Bot
Comment 14 2012-10-31 22:51:52 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14687379
Build Bot
Comment 15 2012-10-31 23:05:08 PDT
Comment on attachment 171769 [details] Patch Attachment 171769 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14561501
Shinya Kawanaka
Comment 16 2012-11-02 02:10:52 PDT
Created attachment 172009 [details] Patch
Hajime Morrita
Comment 17 2012-11-02 02:25:12 PDT
Comment on attachment 172009 [details] Patch The change looks good. In my understanding, we currently allows arbitrary name for pseudos. What is our plan to reject invalid ones?
Early Warning System Bot
Comment 18 2012-11-02 02:26:07 PDT
Comment on attachment 172009 [details] Patch Attachment 172009 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14686775
Hajime Morrita
Comment 19 2012-11-02 02:26:51 PDT
Comment on attachment 172009 [details] Patch (cq- for now just in case.)
Hajime Morrita
Comment 20 2012-11-02 02:27:27 PDT
Comment on attachment 172009 [details] Patch hmm. let's fix build breakage...
Shinya Kawanaka
Comment 21 2012-11-02 22:42:40 PDT
(In reply to comment #17) > (From update of attachment 172009 [details]) > The change looks good. > In my understanding, we currently allows arbitrary name for pseudos. What is our plan to reject invalid ones? Yes. I would like to do some refactoring (https://bugs.webkit.org/show_bug.cgi?id=100826) first. Then it should become easy to reject invalid ones. I'll work it in https://bugs.webkit.org/show_bug.cgi?id=100919
Shinya Kawanaka
Comment 22 2012-11-02 23:13:06 PDT
Created attachment 172210 [details] Patch
Shinya Kawanaka
Comment 23 2012-11-04 17:54:45 PST
Created attachment 172251 [details] Patch
WebKit Review Bot
Comment 24 2012-11-04 19:05:22 PST
Comment on attachment 172251 [details] Patch Clearing flags on attachment: 172251 Committed r133428: <http://trac.webkit.org/changeset/133428>
WebKit Review Bot
Comment 25 2012-11-04 19:05:28 PST
All reviewed patches have been landed. Closing bug.
Ojan Vafai
Comment 26 2012-11-06 10:19:09 PST
I'm not sure how, but I think this patch may have caused a 2% increase in memory usage on the Chromium page cyclers. http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?rev=165925&graph=ws_single_peak_r&history=100 Regression range: http://trac.webkit.org/log/?verbose=on&rev=133430&stop_rev=133428
Dimitri Glazkov (Google)
Comment 29 2012-11-07 09:55:04 PST
(In reply to comment #28) > Another regressions that are either from this patch or http://trac.webkit.org/changeset/133429/: > > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=166198&graph=CreateNodes&history=100 Time for some speculative rollouts? :)
Ojan Vafai
Comment 30 2012-11-07 10:28:42 PST
I tried to roll this out, but couldn't due to conflicts with http://trac.webkit.org/changeset/133749. Could someone more familiar with these patches give it a shot?
Shinya Kawanaka
Comment 31 2012-11-07 17:55:17 PST
Hmm... It's hard for me to imagine this causes such memory regression...
Shinya Kawanaka
Comment 32 2012-11-07 18:00:42 PST
(In reply to comment #31) > Hmm... > It's hard for me to imagine this causes such memory regression... Let me try to roll this out anyway. I hope this is innocent, though.
Ojan Vafai
Comment 33 2012-11-07 18:27:46 PST
Thanks! If it turns out to be Dimitri's patch, r=me on relanding this. We'll know once the rollout goes in.
Shinya Kawanaka
Comment 34 2012-11-07 20:05:49 PST
Reverted. https://bugs.webkit.org/show_bug.cgi?id=101533 Let's see the page cycler for a while.
Ojan Vafai
Comment 36 2012-11-07 22:10:48 PST
(In reply to comment #35) > This patch seems innocent? Yes. Sorry for the misplaced blame. Thanks for trying the rollout. It must be http://trac.webkit.org/changeset/133429/.
Note You need to log in before you can comment on or make changes to this bug.