RESOLVED FIXED 90017
[Meta] [Shadow] contenteditable attribute for distributed nodes.
https://bugs.webkit.org/show_bug.cgi?id=90017
Summary [Meta] [Shadow] contenteditable attribute for distributed nodes.
Hayato Ito
Reported 2012-06-26 14:57:05 PDT
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.
Attachments
WIP (5.41 KB, patch)
2012-08-30 00:53 PDT, Takashi Sakamoto
no flags
Patch (5.45 KB, patch)
2012-08-30 19:54 PDT, Takashi Sakamoto
no flags
Patch (5.44 KB, patch)
2012-10-14 23:50 PDT, Takashi Sakamoto
no flags
Hayato Ito
Comment 1 2012-07-02 00:19:04 PDT
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
Comment 2 2012-08-30 00:53:02 PDT
Takashi Sakamoto
Comment 3 2012-08-30 19:54:25 PDT
Dimitri Glazkov (Google)
Comment 4 2012-09-07 11:10:45 PDT
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 5 2012-09-13 15:34:10 PDT
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 6 2012-09-25 02:13:42 PDT
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 7 2012-09-25 09:02:50 PDT
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 8 2012-09-27 02:21:14 PDT
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 9 2012-09-27 11:04:58 PDT
Comment on attachment 161609 [details] Patch I understand now.
Takashi Sakamoto
Comment 10 2012-10-14 23:50:45 PDT
WebKit Review Bot
Comment 11 2012-10-16 09:23:48 PDT
Comment on attachment 168633 [details] Patch Clearing flags on attachment: 168633 Committed r131464: <http://trac.webkit.org/changeset/131464>
WebKit Review Bot
Comment 12 2012-10-16 09:23:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.