[Shadow DOM] Insertion points need resetStyleInheritance

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

http://dvcs.w3.org/hg/webcomponents/rev/dc65ffd87564
http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#api-html-content-element-reset-style-inheritance etc.

[Takashi Sakamoto]

Created attachment 160684 [details]
WIP

[Takashi Sakamoto]

Created attachment 167277 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Takashi Sakamoto]

Created attachment 167694 [details]
Patch

[Takashi Sakamoto]

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.

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

(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

[Dimitri Glazkov (Google)]

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

[Takashi Sakamoto]

Created attachment 167917 [details]
Patch

[Takashi Sakamoto]

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.

[Dimitri Glazkov (Google)]

(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.

[Takashi Sakamoto]

Created attachment 168339 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168339 [details]
Patch for landing

Clearing flags on attachment: 168339

Committed r131136: <http://trac.webkit.org/changeset/131136>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.