Bug 90017

Summary:

[Meta] [Shadow] contenteditable attribute for distributed nodes.

Product:

WebKit

Reporter:

Hayato Ito <hayato>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, darin, dglazkov, dominicc, enrica, macpherson, menard, morrita, rniwa, shinyak, tasak, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

90197, 90434, 90661

Bug Blocks:

82697

Attachments:

Description	Flags
WIP	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (5.45 KB, patch) 2012-08-30 19:54 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (5.44 KB, patch) 2012-10-14 23:50 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

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

Created attachment 161419 [details] WIP

Takashi Sakamoto

Comment 3 2012-08-30 19:54:25 PDT

Created attachment 161609 [details] Patch

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

Created attachment 168633 [details] Patch

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.