Bug 93922

Created attachment 160684 [details] WIP

Created attachment 167277 [details] Patch

Comment on attachment 167277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167277&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Oops :) > Source/WebCore/css/StyleResolver.cpp:1232 > + for ( ; insertionPoint; ) { > + InsertionPoint* youngerInsertionPoint = shadow->insertionPointFor(insertionPoint); > + if (!youngerInsertionPoint) > + break; > + insertionPoint = youngerInsertionPoint; > + } This seems forced. Can you help me understand why the styles of the insertion point wouldn't implicitly have this information? Presumably, their styles have already been computed and they already have the correctly reset inheritance. > LayoutTests/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Oops :) > LayoutTests/fast/dom/shadow/insertion-point-resetStyleInheritance-expected.txt:10 > +FAIL window.getComputedStyle(document.getElementById("reset-style-inheritance-dynamic").firstChild).color should be rgb(0, 0, 0). Was rgb(255, 238, 238). Why'd we fail this?

Created attachment 167694 [details] Patch

Comment on attachment 167277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167277&action=review Thank you for reviewing. >> Source/WebCore/ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > Oops :) I'm sorry. I forgot to update ChangeLogs. Done. >> Source/WebCore/css/StyleResolver.cpp:1232 >> + } > > This seems forced. Can you help me understand why the styles of the insertion point wouldn't implicitly have this information? Presumably, their styles have already been computed and they already have the correctly reset inheritance. I think, as insertion points don't have any renderers, insertion points don't have any render styles. I know, it is possible for some element to have render styles without any renderers, i.e. implementing its own nonRendererRenderStyle method. However currently insertion points uses default nonRenderRenderStyle which just returns 0. Talking about render styles, currently no information about reset-style-inheritance is added to render styles. >> LayoutTests/ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > Oops :) Sorry. Done. >> LayoutTests/fast/dom/shadow/insertion-point-resetStyleInheritance-expected.txt:10 >> +FAIL window.getComputedStyle(document.getElementById("reset-style-inheritance-dynamic").firstChild).color should be rgb(0, 0, 0). Was rgb(255, 238, 238). > > Why'd we fail this? I see. This is a bug of insertion-point-resetStyleInheritance.html. I fixed.

Comment on attachment 167694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167694&action=review > Source/WebCore/css/StyleResolver.cpp:1066 > +inline bool isResetStyleInheritance(NodeRenderingContext& context) It makes no sense to say that a context “is reset style inheritance”. Instead we should ask if a context “should reset style inheritance” or “resets style inheritance”. > Source/WebCore/html/shadow/InsertionPoint.h:81 > + bool m_resetStyleInheritance : 1; It’s unfortunate that the shadow specification gives a property a verb phrase name. The function resetStyleInheritance sounds like something that would perform a reset operation, not a getter that returns a boolean. We don’t have to repeat that mistake in our data member. I would suggest the name “m_shouldResetStyleInheritance” or at least “m_resetsStyleInheritance”, and also suggest discussing changing the name in the spec.

(In reply to comment #6) > (From update of attachment 167694 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167694&action=review > > > Source/WebCore/css/StyleResolver.cpp:1066 > > +inline bool isResetStyleInheritance(NodeRenderingContext& context) > > It makes no sense to say that a context “is reset style inheritance”. Instead we should ask if a context “should reset style inheritance” or “resets style inheritance”. > > > Source/WebCore/html/shadow/InsertionPoint.h:81 > > + bool m_resetStyleInheritance : 1; > > It’s unfortunate that the shadow specification gives a property a verb phrase name. The function resetStyleInheritance sounds like something that would perform a reset operation, not a getter that returns a boolean. Hey, you have the spec editor's ears right here :) It's a flag that, if set, will reset style inheritance. What would be a better name? > > We don’t have to repeat that mistake in our data member. I would suggest the name “m_shouldResetStyleInheritance” or at least “m_resetsStyleInheritance”, and also suggest discussing changing the name in the spec.

(In reply to comment #7) > It's a flag that, if set, will reset style inheritance. What would be a better name? Here's the spec definition: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#dfn-reset-style-inheritance

Comment on attachment 167694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167694&action=review This is getting close. We need to get CSS WG to go ahead and spec display: contents, so that we can redefine <content>/<shadow> in terms of that primitive, instead of the hacking this into StyleResolver. > Source/WebCore/html/shadow/HTMLShadowElement.idl:37 > + attribute boolean resetStyleInheritance; What about adding the attribute to HTML? >> Source/WebCore/html/shadow/InsertionPoint.h:81 >> + bool m_resetStyleInheritance : 1; > > It’s unfortunate that the shadow specification gives a property a verb phrase name. The function resetStyleInheritance sounds like something that would perform a reset operation, not a getter that returns a boolean. > > We don’t have to repeat that mistake in our data member. I would suggest the name “m_shouldResetStyleInheritance” or at least “m_resetsStyleInheritance”, and also suggest discussing changing the name in the spec. For what it's worth, since it reflects the "reset-style-inheritance" attribute on HTMLContentElement/HTMLShadowElement, I think the name falls well into the current naming convention for attributes, such as "autoplay", "autofocus", "defer", etc. http://www.whatwg.org/specs/web-apps/current-work/multipage/section-index.html#attributes-1

Created attachment 167917 [details] Patch

Comment on attachment 167694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167694&action=review Thank you for reviewing. >>> Source/WebCore/css/StyleResolver.cpp:1066 >>> +inline bool isResetStyleInheritance(NodeRenderingContext& context) >> >> It makes no sense to say that a context “is reset style inheritance”. Instead we should ask if a context “should reset style inheritance” or “resets style inheritance”. > > Hey, you have the spec editor's ears right here :) > > It's a flag that, if set, will reset style inheritance. What would be a better name? As resetStyleInheritance in HTMLShadowElement.idl and HTMLContentElement.idl is defined in Shadow DOM spec, we cannot change the code. Firstly we have to change the spec. However talking about this inline function and m_resetStyleInhertiance in class InsertionPoint, we can change. So I renamed. >> Source/WebCore/html/shadow/HTMLShadowElement.idl:37 >> + attribute boolean resetStyleInheritance; > > What about adding the attribute to HTML? Would you mean moving resetStyleInheritance from HTMLShadow/ContentElement.idl to HTMLElement.idl? If so, I talked with morrita@ and haraken@ about this and heard that IDL files allow such duplication and it is bad to add resetStyleInheritance to HTMLElement because users can see HTMLElement has resetStyleInheritance.

(In reply to comment #11) > Would you mean moving resetStyleInheritance from HTMLShadow/ContentElement.idl to HTMLElement.idl? > If so, I talked with morrita@ and haraken@ about this and heard that IDL files allow such duplication and it is bad to add resetStyleInheritance to HTMLElement because users can see HTMLElement has resetStyleInheritance. No, no -- none of that. Why would we need to do something like this? :) I was just talking about adding the HTML attribute "reset-style-inheritance" to HTMLAttributeNames.in and the corresponding plumbing to set/unset it. You can do this in the follow-up patch.

Created attachment 168339 [details] Patch for landing

Comment on attachment 168339 [details] Patch for landing Clearing flags on attachment: 168339 Committed r131136: <http://trac.webkit.org/changeset/131136>

All reviewed patches have been landed. Closing bug.