Bug 103711 - [Shadow DOM]: reset-style-inheritance doesn't work for insertion point
Summary: [Shadow DOM]: reset-style-inheritance doesn't work for insertion point
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on:
Blocks: 63606 103230 103709
  Show dependency treegraph
 
Reported: 2012-11-29 22:11 PST by Sergey G. Grekhov
Modified: 2013-01-17 22:30 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.14 KB, patch)
2012-12-05 17:26 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.25 KB, patch)
2012-12-06 19:05 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.27 KB, patch)
2012-12-09 21:42 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey G. Grekhov 2012-11-29 22:11:20 PST
According the Shadow DOM spec (https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles) reset-style-inheritance flag should reset all inheritable CSS properties to their initial values at the lower boundary of the tree. This doesn't work in Chrome 23.0.1271.91 m. Example:

<html>
<head>
<script type="text/javascript">

function test() {
    var SR = window.ShadowRoot || window.WebKitShadowRoot;
    var d = document;

    d.body.innerHTML = 
        '<ul id="shHost">' +
            '<li id="li1" class="shadow">1</li>' +
            '<li id="li2" class="shadow2">2</li>' +
            '<li id="li3" class="shadow">3</li>' +
            '<li id="li4">4</li>' +
            '<li id="li5" class="shadow">5</li>' +
            '<li id="li6" class="shadow2">6</li>' +
        '</ul>';

    var host = d.querySelector('#shHost');
    
    d.body.setAttribute('style', 'color:red');
        
    //Shadow root to play with
    var s = new SR(host);

    var div = d.createElement('div');   
    div.innerHTML ='<ul><content select=".shadow" reset-style-inheritance=true></content></ul>'; 
    s.appendChild(div);    
}
</script>
</head>
<body onload="test()">

</body>
</html>

Run the example above and observe the browser window. Red color style inherited from document body is not reset in spite of reset-style-inheritance=true attribute. If to modify example above to set resetStyleInheritance property this won't work anyway. In other words:

    var div = d.createElement('div');   
    div.innerHTML ='<ul><content select=".shadow" id="shInsPoint"></content></ul>';
    s.appendChild(div);
    
    s.querySelector('#shInsPoint').resetStyleInheritance = true;

The modified script doesn't work anyway
Comment 1 Sergey G. Grekhov 2012-11-29 22:14:03 PST
See also https://bugs.webkit.org/show_bug.cgi?id=103709
Comment 2 Takashi Sakamoto 2012-12-05 17:26:45 PST
Created attachment 177881 [details]
Patch
Comment 3 Takashi Sakamoto 2012-12-05 17:28:24 PST
This patch will also fix bug 103709, [Shadow DOM]: reset-style-inheritance flag doesn't work for shadow insertion point.
Comment 4 Hajime Morrita 2012-12-05 20:48:20 PST
Comment on attachment 177881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177881&action=review

> Source/WebCore/html/shadow/InsertionPoint.cpp:168
> +void InsertionPoint::parseAttribute(const QualifiedName& name, const AtomicString& value)

This isn't enough. We need to reflect reflect change on resetStyleInheritance property to the attribute value. 
In general, we no longer need a boolean flag on the class. That can be represented as an attribute value.
See how HTMLButtonElement#disabled is implemented.
Comment 5 Takashi Sakamoto 2012-12-06 19:05:08 PST
Created attachment 178135 [details]
Patch
Comment 6 Takashi Sakamoto 2012-12-06 19:34:22 PST
Comment on attachment 177881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177881&action=review

Thank you for reviewing.

>> Source/WebCore/html/shadow/InsertionPoint.cpp:168
>> +void InsertionPoint::parseAttribute(const QualifiedName& name, const AtomicString& value)
> 
> This isn't enough. We need to reflect reflect change on resetStyleInheritance property to the attribute value. 
> In general, we no longer need a boolean flag on the class. That can be represented as an attribute value.
> See how HTMLButtonElement#disabled is implemented.

I see. I removed m_shouldResetStyleInheritance and modified to look up at reset-style-inheritance attribute value.
Comment 7 WebKit Review Bot 2012-12-07 00:05:20 PST
Comment on attachment 178135 [details]
Patch

Rejecting attachment 178135 [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:
dow/InsertionPoint.cpp.rej
patching file Source/WebCore/html/shadow/InsertionPoint.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/shadow/insertion-point-resetStyleInheritance-expected.txt
patching file LayoutTests/fast/dom/shadow/insertion-point-resetStyleInheritance.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15210033
Comment 8 Takashi Sakamoto 2012-12-09 21:42:35 PST
Created attachment 178469 [details]
Patch
Comment 9 WebKit Review Bot 2012-12-09 23:44:41 PST
Comment on attachment 178469 [details]
Patch

Clearing flags on attachment: 178469

Committed r137112: <http://trac.webkit.org/changeset/137112>
Comment 10 WebKit Review Bot 2012-12-09 23:44:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Hajime Morrita 2013-01-17 22:30:39 PST
(In reply to comment #10)
> All reviewed patches have been landed.  Closing bug.

It looks test on shadow dom test suite continues failing even after this change. Filed 107229.