|
|
Created:
4 years, 1 month ago by hayato Modified:
4 years, 1 month ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a UA style, "display: contents", for <slot>
The spec is: https://html.spec.whatwg.org/#flow-content-3
This rule is guarded by the same runtime flag for "display: contents".
Unless the flag is enabled, the rule can not be effective because the parser does
not consider it a valid rule.
This change only affects slots in non-shadow trees since Blink excludes slots in
shadow trees from a flat tree.
Since "display: contents" is not well supported yet, the children of a slot in
non-shadow trees would not be displayed.
BUG=657748, 660265
Committed: https://crrev.com/d582fd6159457a8a9ff4f2064ce45d8bc4626c72
Cr-Commit-Position: refs/heads/master@{#429198}
Patch Set 1 #
Total comments: 2
Patch Set 2 : updatae #
Total comments: 3
Patch Set 3 : Use html.css #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add an UA-style for <slot> BUG= ========== to ========== Add UA-style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since the current implementation of "display: contents" is limited, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ==========
Description was changed from ========== Add UA-style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since the current implementation of "display: contents" is limited, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ========== to ========== Add an UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since the current implementation of "display: contents" is limited, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ==========
Description was changed from ========== Add an UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since the current implementation of "display: contents" is limited, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ========== to ========== Add an UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ==========
hayato@chromium.org changed reviewers: + kochi@chromium.org
PTAL
I'm confused by the last line of the description: > the children of a slot in non-shadow trees > would not be displayed. When the experimental flag is turned on, any fallback slots in non-shadow tree will be displayed as if they are specified "display: none" as "display: contents" is not implemented? https://codereview.chromium.org/2466243002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/html.css (right): https://codereview.chromium.org/2466243002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/html.css:1103: display: contents; Why this is defined in both html.css and slot.css? Does it mean slot's UA style applies unconditionally?
updatae
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> When the experimental flag is turned on, any fallback slots > in non-shadow tree will be displayed as if they are > specified "display: none" as "display: contents" is > not implemented? Yes. It looks the implementation is on-going. https://codereview.chromium.org/2466243002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/html.css (right): https://codereview.chromium.org/2466243002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/html.css:1103: display: contents; Ops. I forgot to remove this from html.css. Done.
LGTM with nits For the subject and the first line of the description, "an UA sytle" should be "a UA stylesheet". https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/slots-fallback-in-document-tree-expected.html (right): https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/slots-fallback-in-document-tree-expected.html:4: <!-- See https://codereview.chromium.org/2443693003 for the current implementation. --> Maybe http://crbug.com/657748 is a better link for tracking the implementation info? https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp:165: !m_slotStyleSheet) { If the reason for having a separate UA stylesheet file only for <slot> element is to gate the behavior by the runtime flag, could you add a TODO for merging slot.css into html.css once CSSDisplayContents flag gets enabled?
Description was changed from ========== Add an UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ========== to ========== Add a UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/2466243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp:165: !m_slotStyleSheet) { On 2016/11/01 06:43:13, kochi wrote: > If the reason for having a separate UA stylesheet file only for > <slot> element is to gate the behavior by the runtime flag, could > you add a TODO for merging slot.css into html.css once > CSSDisplayContents flag gets enabled? I think we might as well add it to html.css. When display:contents is not supported, we will end up with an empty rule for slot, which is OK I think. Alternatively, you can conditionally add the slot.css source to the defaultRules string in the constructor. Having it here, means we'll get a re-collection of document rule features when we see a slot element the first time. I think it's better to parse the slot rule unconditionally.
I have found that the current approach might not be good enough. Let me re-check again.
Use html.css
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the reviews. I have changed my mind. It looks that using slot.css would not be beneficial as I thought. Rather, adding a rule to html.css would make sense because it is harmless. I have updated the patch set so it adds a rule to html.css directly. Let me land a patch.
Description was changed from ========== Add a UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This change is guarded by the same runtime flag for "display: contents". This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ========== to ========== Add a UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This rule is guarded by the same runtime flag for "display: contents". Unless the flag is enabled, the rule can not be effective because the parser does not consider it a valid rule. This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ==========
The CQ bit was unchecked by hayato@chromium.org
The CQ bit was checked by hayato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org Link to the patchset: https://codereview.chromium.org/2466243002/#ps40001 (title: "Use html.css")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add a UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This rule is guarded by the same runtime flag for "display: contents". Unless the flag is enabled, the rule can not be effective because the parser does not consider it a valid rule. This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 ========== to ========== Add a UA style, "display: contents", for <slot> The spec is: https://html.spec.whatwg.org/#flow-content-3 This rule is guarded by the same runtime flag for "display: contents". Unless the flag is enabled, the rule can not be effective because the parser does not consider it a valid rule. This change only affects slots in non-shadow trees since Blink excludes slots in shadow trees from a flat tree. Since "display: contents" is not well supported yet, the children of a slot in non-shadow trees would not be displayed. BUG=657748,660265 Committed: https://crrev.com/d582fd6159457a8a9ff4f2064ce45d8bc4626c72 Cr-Commit-Position: refs/heads/master@{#429198} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d582fd6159457a8a9ff4f2064ce45d8bc4626c72 Cr-Commit-Position: refs/heads/master@{#429198} |