Move overlay property behind a flag [chromium/src : main]
Rune Lillesveen (Gerrit)
Rune Lillesveen added 3 comments![]( "Open in Gerrit")
}If we're doing this regardlessly, should we pull it out of the display:none block above and re-use that result here?
if (should_remove) {I think you can simplify to:
```
const ComputedStyle* style = ComputedStyle::NullifyEnsured(element->GetComputedStyle());
if (!style || (RuntimeEnabledFeatures::OverlayPropertyEnabled() && style->Overlay() == EOverlay::kNone)) {
to_remove.push_back(element);
}
```
StyleType() == kPseudoIdBackdrop;How about:
```
return StyleType() == kPseudoIdBackdrop || (element.IsInTopLayer() && (!RuntimeEnabledFeatures::OverlayPropertyEnabled() || Overlay() == EOverlay::kAuto));
```
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
// UA rule: ::scroll-marker-group { contain: size !important; }FWIW, ::scroll-marker-group isn't done here anymore; this part of the comment is outdated.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
If we're doing this regardlessly, should we pull it out of the display:none block above and re-use that result here?
Done
if (should_remove) {I think you can simplify to:
```
const ComputedStyle* style = ComputedStyle::NullifyEnsured(element->GetComputedStyle());
if (!style || (RuntimeEnabledFeatures::OverlayPropertyEnabled() && style->Overlay() == EOverlay::kNone)) {
to_remove.push_back(element);
}
```
Done
StyleType() == kPseudoIdBackdrop;How about:
```
return StyleType() == kPseudoIdBackdrop || (element.IsInTopLayer() && (!RuntimeEnabledFeatures::OverlayPropertyEnabled() || Overlay() == EOverlay::kAuto));
```
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
2 comments
if (element == state.GetDocument().documentElement() ||You already have this as `is_document_element` now.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
if (element == state.GetDocument().documentElement() ||You already have this as `is_document_element` now.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Joey Arhar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
// UA rule: ::scroll-marker-group { contain: size !important; }I think this comment about ::scroll-marker-group is just stale. Remove it?
ComputedStyle::NullifyEnsured(element->GetComputedStyle());Is the change to use NullifyEnsured intended? It seems like a potential change in behavior that isn't guarded by the flag.
to_remove.push_back(element);Does this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```
With overlay disabled, this stays red.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
ComputedStyle::NullifyEnsured(element->GetComputedStyle());Is the change to use NullifyEnsured intended? It seems like a potential change in behavior that isn't guarded by the flag.
I asked for that change. The existing code would have different behavior for display and overlay transitions of different length depending on whether you invoked getComputedStyle() or not.
That is, if you have a transition-duration for overlay that's longer than the one for display, you would not remove the element from the top layer when going to display:none if the author used getComputedStyle() to ensure the styles for that display:none element.
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
ComputedStyle::NullifyEnsured(element->GetComputedStyle());Rune LillesveenIs the change to use NullifyEnsured intended? It seems like a potential change in behavior that isn't guarded by the flag.
I asked for that change. The existing code would have different behavior for display and overlay transitions of different length depending on whether you invoked getComputedStyle() or not.
That is, if you have a transition-duration for overlay that's longer than the one for display, you would not remove the element from the top layer when going to display:none if the author used getComputedStyle() to ensure the styles for that display:none element.
Should we put this behind `!RuntimeEnabledFeatures::OverlayPropertyEnabled()`? I don't know a lot about the risks here, and I'm okay with whichever approach.
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
// UA rule: ::scroll-marker-group { contain: size !important; }I think this comment about ::scroll-marker-group is just stale. Remove it?
Done
ComputedStyle::NullifyEnsured(element->GetComputedStyle());Rune LillesveenIs the change to use NullifyEnsured intended? It seems like a potential change in behavior that isn't guarded by the flag.
Philip RogersI asked for that change. The existing code would have different behavior for display and overlay transitions of different length depending on whether you invoked getComputedStyle() or not.
That is, if you have a transition-duration for overlay that's longer than the one for display, you would not remove the element from the top layer when going to display:none if the author used getComputedStyle() to ensure the styles for that display:none element.
Should we put this behind `!RuntimeEnabledFeatures::OverlayPropertyEnabled()`? I don't know a lot about the risks here, and I'm okay with whichever approach.
Ok, I put the NullifyEnsured behind a feature flag check
to_remove.push_back(element);Does this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```With overlay disabled, this stays red.
Yes, this change leaves element stuck in the top layer until the element gets display:none'd. I talked to Rune and Tab in an email thread about this, and also opened a csswg issue: https://github.com/w3c/csswg-drafts/issues/13200
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
to_remove.push_back(element);Joey ArharDoes this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```With overlay disabled, this stays red.
Yes, this change leaves element stuck in the top layer until the element gets display:none'd. I talked to Rune and Tab in an email thread about this, and also opened a csswg issue: https://github.com/w3c/csswg-drafts/issues/13200
To elaborate - I don't think we need to support this case with top layer animations where you're animating from top layer to an in-flow thing instead of just display:none. Tab pointed out that we should be able to just use view transitions if people want to do that.
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
to_remove.push_back(element);Joey ArharDoes this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```With overlay disabled, this stays red.
Joey ArharYes, this change leaves element stuck in the top layer until the element gets display:none'd. I talked to Rune and Tab in an email thread about this, and also opened a csswg issue: https://github.com/w3c/csswg-drafts/issues/13200
To elaborate - I don't think we need to support this case with top layer animations where you're animating from top layer to an in-flow thing instead of just display:none. Tab pointed out that we should be able to just use view transitions if people want to do that.
The example here doesn't have any animations though, it's just a dialog that gets opened and closed. Is it intended that the backdrop will not be removed for this?
Related details
- Joey Arhar
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
to_remove.push_back(element);Joey ArharDoes this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```With overlay disabled, this stays red.
Joey ArharYes, this change leaves element stuck in the top layer until the element gets display:none'd. I talked to Rune and Tab in an email thread about this, and also opened a csswg issue: https://github.com/w3c/csswg-drafts/issues/13200
Philip RogersTo elaborate - I don't think we need to support this case with top layer animations where you're animating from top layer to an in-flow thing instead of just display:none. Tab pointed out that we should be able to just use view transitions if people want to do that.
The example here doesn't have any animations though, it's just a dialog that gets opened and closed. Is it intended that the backdrop will not be removed for this?
The dialog isn't really closed though since its forced open while internally in a closed state via the display:block rule.
I suppose that we could rewrite how ::backdrop works to render when the element is internally in an open state rather than when the element is being rendered in the top layer, but that would make it more complicated to implement a top layer exit animation since we still want the backdrop to be rendered during the exit animation but the element is internally closed...
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
2 comments
to_remove.push_back(element);Joey ArharDoes this change not leave elements stuck in the top layer? For example:
```
<!DOCTYPE html>
<style>
dialog { display: block; }
dialog::backdrop { background: red; }
</style>
<dialog id='target'>After one second, the red backdrop should go away</dialog>
<script>
target.showModal();
setTimeout(() => {
target.close();
}, 1000);
</script>
```With overlay disabled, this stays red.
Joey ArharYes, this change leaves element stuck in the top layer until the element gets display:none'd. I talked to Rune and Tab in an email thread about this, and also opened a csswg issue: https://github.com/w3c/csswg-drafts/issues/13200
Philip RogersTo elaborate - I don't think we need to support this case with top layer animations where you're animating from top layer to an in-flow thing instead of just display:none. Tab pointed out that we should be able to just use view transitions if people want to do that.
Joey ArharThe example here doesn't have any animations though, it's just a dialog that gets opened and closed. Is it intended that the backdrop will not be removed for this?
The dialog isn't really closed though since its forced open while internally in a closed state via the display:block rule.
I suppose that we could rewrite how ::backdrop works to render when the element is internally in an open state rather than when the element is being rendered in the top layer, but that would make it more complicated to implement a top layer exit animation since we still want the backdrop to be rendered during the exit animation but the element is internally closed...
- Joey Arhar
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Commit-Queue | +2 |
1 comment
ComputedStyle::NullifyEnsured(element->GetComputedStyle());Rune LillesveenIs the change to use NullifyEnsured intended? It seems like a potential change in behavior that isn't guarded by the flag.
Philip RogersI asked for that change. The existing code would have different behavior for display and overlay transitions of different length depending on whether you invoked getComputedStyle() or not.
That is, if you have a transition-duration for overlay that's longer than the one for display, you would not remove the element from the top layer when going to display:none if the author used getComputedStyle() to ensure the styles for that display:none element.
Joey ArharShould we put this behind `!RuntimeEnabledFeatures::OverlayPropertyEnabled()`? I don't know a lot about the risks here, and I'm okay with whichever approach.
Ok, I put the NullifyEnsured behind a feature flag check
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Move overlay property behind a flag
This patch makes the overlay property obsolete by keeping elements in
the top layer until they are display:none. This enables exit animations
with only transitioning the display property instead of transitioning
both the display and overlay properties.
- M third_party/blink/renderer/core/css/css_properties.json5
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/css/resolver/style_resolver.cc
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/virtual/overlay-disabled/README.md
Code-Review: +1 by Joey Arhar, +1 by Rune Lillesveen, +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |