[anchor] Request main frame animation for delayed animation start. [chromium/src : main]
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Okay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Related details
- Rune Lillesveen
- Stefan Zager
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 added 2 comments![]( "Open in Gerrit")
} else if (layout_object->MayContainAnchor() && old_style &&Can you have a layout_object and a null old_style at the same time? We do access old_style without a nullptr check a bit further up.
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Okay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Why can't we handle this in LayoutObject::SetStyle() instead?
Related details
- Morten Stenshorne
- Stefan Zager
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. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
} else if (layout_object->MayContainAnchor() && old_style &&Can you have a layout_object and a null old_style at the same time? We do access old_style without a nullptr check a bit further up.
Uhm.. yeah, that seems impossible. I was unsure about old_style. There are some scattered nullptr checks above, but not consistently so.
I'll deal with this if we decide to keep the code here.
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Why can't we handle this in LayoutObject::SetStyle() instead?
Sure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
Related details
- Rune Lillesveen
- Stefan Zager
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 added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Sure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
OK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
Related details
- Morten Stenshorne
- Stefan Zager
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. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Rune LillesveenSure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
OK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
I don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
- Rune Lillesveen
- Stefan Zager
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 added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Rune LillesveenSure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
Morten StenshorneOK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
I don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I haven't seen the 'spaghetti', so it's a bit hard to tell, but I'm leaning towards having the logic in LayoutObject. How 'spaghetti' is it?
Related details
- Morten Stenshorne
- Stefan Zager
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. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Rune LillesveenSure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
Morten StenshorneOK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
Rune LillesveenI don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I haven't seen the 'spaghetti', so it's a bit hard to tell, but I'm leaning towards having the logic in LayoutObject. How 'spaghetti' is it?
Oh, it's just this: https://chromium-review.googlesource.com/c/chromium/src/+/7451459/1/third_party/blink/renderer/core/layout/layout_object.cc
Maybe it's just macaroni.
Related details
- Rune Lillesveen
- Stefan Zager
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 added 1 comment![]( "Open in Gerrit")
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Rune LillesveenSure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
Morten StenshorneOK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
Rune LillesveenI don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneI don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I haven't seen the 'spaghetti', so it's a bit hard to tell, but I'm leaning towards having the logic in LayoutObject. How 'spaghetti' is it?
Oh, it's just this: https://chromium-review.googlesource.com/c/chromium/src/+/7451459/1/third_party/blink/renderer/core/layout/layout_object.cc
Maybe it's just macaroni.
- Morten Stenshorne
- Stefan Zager
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. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
} else if (layout_object->MayContainAnchor() && old_style &&Morten StenshorneCan you have a layout_object and a null old_style at the same time? We do access old_style without a nullptr check a bit further up.
Uhm.. yeah, that seems impossible. I was unsure about old_style. There are some scattered nullptr checks above, but not consistently so.
I'll deal with this if we decide to keep the code here.
Done
layout_object->SetNeedsLayout(layout_invalidation_reason::kStyleChange);Rune LillesveenOkay with SetNeedsLayout() here?
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneWhy can't we handle this in LayoutObject::SetStyle() instead?
Rune LillesveenSure, we can do that - before the `apply_changes == ApplyStyleChanges::kNo` check. Would you prefer that?
Morten StenshorneOK, so that's the reason. The pattern we've used before is to force apply_changes to kYes. Will that be a problem in this case?
Rune LillesveenI don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
Morten StenshorneI don't think so, and it was my actually my initial approach, before I came up with this, which was less "spaghetti" and easier to reason about (well, that was my impression, anyway).
In fact, this alternative approach is also what I'm asking about in my first comment here:
Another option would be to set apply_changes to yes here, and then mark for layout somewhere in Style{Will,Did}Change.
I haven't seen the 'spaghetti', so it's a bit hard to tell, but I'm leaning towards having the logic in LayoutObject. How 'spaghetti' is it?
Rune LillesveenOh, it's just this: https://chromium-review.googlesource.com/c/chromium/src/+/7451459/1/third_party/blink/renderer/core/layout/layout_object.cc
Maybe it's just macaroni.
I love macaroni.
- Rune Lillesveen
- Stefan Zager
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 added 4 comments![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start."start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
needed.Why don't we get a re-layout when the applied translateX() animation starts applying translations different from 0 on the main thread without the change in this CL?
<title>Animated anchor transform, delayed animation start</title>I don't see any delayed start of the animation here? You mean the animation stays at translateX(0) for half the animation?
try { await done.promise; } catch(e) {}Why try/catch this one?
Related details
- Morten Stenshorne
- Stefan Zager
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. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 4 comments![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start."start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
That's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Why don't we get a re-layout when the applied translateX() animation starts applying translations different from 0 on the main thread without the change in this CL?
Because it all happens on the compositor thread otherwise, I suppose.
<title>Animated anchor transform, delayed animation start</title>I don't see any delayed start of the animation here? You mean the animation stays at translateX(0) for half the animation?
Yes, that's it.
try { await done.promise; } catch(e) {}Why try/catch this one?
I think I got the test runner to freeze or crash otherwise, at least if the test fails.
Related details
- Rune Lillesveen
- Stefan Zager
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 added 2 comments![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
That's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
I don't understand why.
Morten StenshorneWhy don't we get a re-layout when the applied translateX() animation starts applying translations different from 0 on the main thread without the change in this CL?
Because it all happens on the compositor thread otherwise, I suppose.
The animation has to apply its effects on the main thread too. Otherwise you get the wrong results from getComputedStyle().
Related details
- Morten Stenshorne
- Stefan Zager
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. |