Fix hover style not updating after moveBefore() [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Thanks for the CL! I think it looks fine, but it's a partial fix. You can see what I mean by doing the following:
1. Loading the URL: `data:text/html,<style> .item .controls { background: blue; } .item:hover .controls { background: red; } </style> <body> <div class="item" id="itemA"> <div class="controls" id="controlsA">A</div> </div> <div class="item" id="itemB"> <div class="controls" id="controlsB">B</div> </div> </body>` in your browser
2. Don't hover over A or B at all
3. Open DevTools and `itemA.parentElement.moveBefore(itemA, null);`
4. Hover for the first time over `itemA` and note that hovering doesn't work until you then un-hover, and *re*-hover a second time. Then it works.
So I think a little more might be required to get this all wrapped up. Could you investigate what is required to fix that? And can you add a second case to capture this bug too?
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
<script src="/resources/testharness.js"></script>Its always nice to add a <link> pointing to the relevant part of the specs. Like this:
```
<link rel="help" href="https://link-to-the-spec">
```
Related details
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 3 comments![]( "Open in Gerrit")
Thanks for the CL! I think it looks fine, but it's a partial fix. You can see what I mean by doing the following:
1. Loading the URL: `data:text/html,<style> .item .controls { background: blue; } .item:hover .controls { background: red; } </style> <body> <div class="item" id="itemA"> <div class="controls" id="controlsA">A</div> </div> <div class="item" id="itemB"> <div class="controls" id="controlsB">B</div> </div> </body>` in your browser
2. Don't hover over A or B at all
3. Open DevTools and `itemA.parentElement.moveBefore(itemA, null);`
4. Hover for the first time over `itemA` and note that hovering doesn't work until you then un-hover, and *re*-hover a second time. Then it works.
So I think a little more might be required to get this all wrapped up. Could you investigate what is required to fix that? And can you add a second case to capture this bug too?
Oh, thanks for catching what I missed! I've addressed it.
Thanks for the review! I've addressed the feedback!
Its always nice to add a <link> pointing to the relevant part of the specs. Like this:
```
<link rel="help" href="https://link-to-the-spec">
```
Oh, I'll make sure to keep that in mind when writing test code from now on. Thanks!
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
1 comment
I think this looks good, and it definitely fixes the two tests (thanks for adding the second one). But I have to admit I don't understand the element rare data vector restyle flags all that well. So I'll LGTM, but please wait for Mason's LGTM too.
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
data->ClearRestyleFlags();So this affects all of these methods:
Help me understand the change. Essentially, it looks like the new code says: if this is a state preserving atomic move, *don't* clear the restyle flags (e.g. `SetChildrenOrSiblingsAffectedByHover()`). But that, by itself, doesn't seem like it would fix the general issue that we need to re-calculate the focus/hover/etc styles after an atomic move, because the move could have modified hover states, etc. I guess that recalculation is already happening since node removal invalidates style, so these flags will get recalculated anyway. So why is it necessary to *not* clear the flags for atomic moves only?
if (ElementAnimations* element_animations =Side-note: if this approach ends up being the final one, I think it'd be a good idea to add some test cases (for atomic moves) to the AffectedByPseudoTest:
Related details
- YeongHan Kim
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
+futhark who is likely a better review for this, should the approach stay based on restyle flags.
Related details
- Rune Lillesveen
- YeongHan Kim
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")
data->ClearRestyleFlags();So this affects all of these methods:
Help me understand the change. Essentially, it looks like the new code says: if this is a state preserving atomic move, *don't* clear the restyle flags (e.g. `SetChildrenOrSiblingsAffectedByHover()`). But that, by itself, doesn't seem like it would fix the general issue that we need to re-calculate the focus/hover/etc styles after an atomic move, because the move could have modified hover states, etc. I guess that recalculation is already happening since node removal invalidates style, so these flags will get recalculated anyway. So why is it necessary to *not* clear the flags for atomic moves only?
Yes, I don't understand this either. If you move a subtree from a point where you may be affected by a :hover rule to a place where you cannot, the restyle flags should be cleared.
For instance, if you have:
```
<style>
:hover #fromParent #toMove { ... }
</style>
<div id="hovered">
<div id="toParent>
</div>
<div id="fromParent">
<div id="toMove"></div>
</div>
<div>
```
and do an atomic move to #toParent, I would expect the restyle flags to be cleared for hover.
Also, why wouldn't the restyle flags be updated correctly after the move?
Related details
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 3 comments![]( "Open in Gerrit")
Thanks for the review!
This was very helpful, and I've come up with a possible solution. Could you take another look?
data->ClearRestyleFlags();Rune LillesveenSo this affects all of these methods:
Help me understand the change. Essentially, it looks like the new code says: if this is a state preserving atomic move, *don't* clear the restyle flags (e.g. `SetChildrenOrSiblingsAffectedByHover()`). But that, by itself, doesn't seem like it would fix the general issue that we need to re-calculate the focus/hover/etc styles after an atomic move, because the move could have modified hover states, etc. I guess that recalculation is already happening since node removal invalidates style, so these flags will get recalculated anyway. So why is it necessary to *not* clear the flags for atomic moves only?
Yes, I don't understand this either. If you move a subtree from a point where you may be affected by a :hover rule to a place where you cannot, the restyle flags should be cleared.
For instance, if you have:
```
<style>
:hover #fromParent #toMove { ... }
</style>
<div id="hovered">
<div id="toParent>
</div>
<div id="fromParent">
<div id="toMove"></div>
</div>
<div>
```and do an atomic move to #toParent, I would expect the restyle flags to be cleared for hover.
Also, why wouldn't the restyle flags be updated correctly after the move?
I was misunderstanding the flags, and it seems like the tests were passing by coincidence. Sorry for the confusion.
As you pointed out, since the position has changed, clearing the flags is correct. The issue was that the style wasn't being recalculated after clearing them, and I expect the updated patch should fix it.
Thanks for the guidance!
Side-note: if this approach ends up being the final one, I think it'd be a good idea to add some test cases (for atomic moves) to the AffectedByPseudoTest:
This approach was incorrect, so I'm marking this as done without implementing it.
Related details
- Dominic Farolino
- Mason Freed
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Can you describe the solution approach in the commit description, and explain how it works?
Related details
- Mason Freed
- Rune Lillesveen
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Ok! I've added more details to the commit description.
Related details
- 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. |