Bug 90017 – [Meta] [Shadow] contenteditable attribute for distributed nodes.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hayato Ito]

This meta bug tracks all issues related to 'contenteditable' attribute for distributed nodes in Shadow DOM, including but not limited to:

- Should a contenteditable attribute of shadow host propagate to distributed nodes?
  - See: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17608
- How to implement an inheritance of 'contenteditable' for distributed ndoes.
  - We might need to modify how to inherit a '-user-modify' CSS property.
  - We need to figure out how style is resolved for distributed nodes. We might need to tweak Style::inheritFrom()

Let me add details later.

[Hayato Ito]

I guess style resolving does not work for distributed nodes. Maybe a cache is wrongly used for distributed nodes in resolving style. We might need to file another bug to fix style issues for distributed nodes.

[Takashi Sakamoto]

Created attachment 161419 [details]
WIP

[Takashi Sakamoto]

Created attachment 161609 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 161609 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1783
> +    if (m_distributedToInsertionPoint) {

This seems like an odd place for this code. Should this not be in StyleResolver::applyProperty?

[Ryosuke Niwa]

Comment on attachment 161609 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1786
> +        m_style->setUserModify(element->parentElement()->renderStyle()->userModify());

This looks fishy to me. So distributed nodes don't inherit any style but -webkit-user-modify?

[Takashi Sakamoto]

Comment on attachment 161609 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/StyleResolver.cpp:1783
>> +    if (m_distributedToInsertionPoint) {
> 
> This seems like an odd place for this code. Should this not be in StyleResolver::applyProperty?

In StyleResolver::applyProperty, I found that "case CSSPropertyWebkitUserModify:" is "ASSERT_NOT_REACHED();"
So did you mean StyleBuilder.cpp? i.e. adding a new class, e.g. ApplyPropertyUserModify and implementing an original applyInheritValue?

>> Source/WebCore/css/StyleResolver.cpp:1786
>> +        m_style->setUserModify(element->parentElement()->renderStyle()->userModify());
> 
> This looks fishy to me. So distributed nodes don't inherit any style but -webkit-user-modify?

Distributed nodes inherit styles except -webkit-user-modify from their rendering and style parents, because m_style->inheritFrom(m_parentStyle) has been already executed.
The code is for overriding -webkit-user-modify values by using shadow hosts' values.

[Dimitri Glazkov (Google)]

Comment on attachment 161609 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:1783
>>> +    if (m_distributedToInsertionPoint) {
>> 
>> This seems like an odd place for this code. Should this not be in StyleResolver::applyProperty?
> 
> In StyleResolver::applyProperty, I found that "case CSSPropertyWebkitUserModify:" is "ASSERT_NOT_REACHED();"
> So did you mean StyleBuilder.cpp? i.e. adding a new class, e.g. ApplyPropertyUserModify and implementing an original applyInheritValue?

Yep, that seems like the more logical approach.

[Takashi Sakamoto]

Comment on attachment 161609 [details]
Patch

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

>>>> Source/WebCore/css/StyleResolver.cpp:1783
>>>> +    if (m_distributedToInsertionPoint) {
>>> 
>>> This seems like an odd place for this code. Should this not be in StyleResolver::applyProperty?
>> 
>> In StyleResolver::applyProperty, I found that "case CSSPropertyWebkitUserModify:" is "ASSERT_NOT_REACHED();"
>> So did you mean StyleBuilder.cpp? i.e. adding a new class, e.g. ApplyPropertyUserModify and implementing an original applyInheritValue?
> 
> Yep, that seems like the more logical approach.

I think, applyProperty will be invoked when an element has some matched properties, i.e. suppose that A is a distributed node and there exists "<style>A { -webkit-user-modify: inherit }</style>" or something. I think, this is another issue to be discussed.
The issue I would like to fix in this bug is, for example,

    <host> --------- (SR)
         |           |
         +---- <A>    +---<div>
                           |
                           +---<content> (or <shadow>)

(<host content-editable>
    <#shadow-root><div><content /></div></#shadow-root>
    <A></A>
 </host>) 

A has no matched styles related to -webkit-user-modify.
The host is "content-editable". <div> is not "content-editable".
A's style is inherited from <div>'s style, because A's parentNodeForStyle is <div>.

A should be content-editable or not?

So this patch makes A content-editable.

[Dimitri Glazkov (Google)]

Comment on attachment 161609 [details]
Patch

I understand now.

[Takashi Sakamoto]

Created attachment 168633 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168633 [details]
Patch

Clearing flags on attachment: 168633

Committed r131464: <http://trac.webkit.org/changeset/131464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.