Fix scroll marker updates during Find on Page [chromium/src : main]
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
3 comments
Daniil SakhapovAdding @awogb...@chromium.org who initially wrote this to review.
Done
TAG=agyDaniil SakhapovWhat is this?
Also, do you have an associated bug?
tag to mark that I used AI assistance with this CL.
added bug as well.
Node* target_node = box.GetNode();Daniil SakhapovHmm should this be `box` or `current_box`? I think `box` is the original thing that a scrollIntoView was requested on, while `current_box` is the current thing within the scroll ancestry that we want to scroll into view.
Yeah, you're right. Using only box was causing us to pin markers from other groups because we didn't check which group we are currently scrolling.
But we can't just use current_box either, because we need to find marker for target box, not for the container itself.
So I updated FindScrollMarkerForTargetedScroll to also take group. We still start searching from box, but we stop at the container and only return marker if it belongs to the group we are currently processing.
Related details
- David A
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
Node* target_node = box.GetNode();Daniil SakhapovHmm should this be `box` or `current_box`? I think `box` is the original thing that a scrollIntoView was requested on, while `current_box` is the current thing within the scroll ancestry that we want to scroll into view.
Yeah, you're right. Using only box was causing us to pin markers from other groups because we didn't check which group we are currently scrolling.
But we can't just use current_box either, because we need to find marker for target box, not for the container itself.
So I updated FindScrollMarkerForTargetedScroll to also take group. We still start searching from box, but we stop at the container and only return marker if it belongs to the group we are currently processing.
I don't think we would have this problem of finding the scroll marker to pin if this was implemented the way that was specified in the [resolution](https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2489046785):
RESOLVED: Adopt proposal in https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2448204794 to anchor to identified elements
In particular, each ancestor scrollable area should set the last targeted element to current_box (or box) and then trigger [step 3 of the marker update algorithm](https://www.w3.org/TR/css-overflow-5/#active-scroll-marker:~:text=If%20scroller%20has%20a%20non%2Dnull%20current%20scroll%20target):
3. If scroller has a non-null current scroll target
Let active be the first item in targets encountered by a reverse tree order walk starting from the current scroll target, or the first item in tree order in targets if no target is found in the previous walk.
Rather than adding additional complexity to "pin" and "unpin" markers I would prefer to see the specified fix of having scrollable areas track the last scrolled element and using that as the starting point when updating the active marker.
Related details
- Daniil Sakhapov
- David A
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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
Node* target_node = box.GetNode();Daniil SakhapovHmm should this be `box` or `current_box`? I think `box` is the original thing that a scrollIntoView was requested on, while `current_box` is the current thing within the scroll ancestry that we want to scroll into view.
Robert FlackYeah, you're right. Using only box was causing us to pin markers from other groups because we didn't check which group we are currently scrolling.
But we can't just use current_box either, because we need to find marker for target box, not for the container itself.
So I updated FindScrollMarkerForTargetedScroll to also take group. We still start searching from box, but we stop at the container and only return marker if it belongs to the group we are currently processing.
I don't think we would have this problem of finding the scroll marker to pin if this was implemented the way that was specified in the [resolution](https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2489046785):
RESOLVED: Adopt proposal in https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2448204794 to anchor to identified elements
In particular, each ancestor scrollable area should set the last targeted element to current_box (or box) and then trigger [step 3 of the marker update algorithm](https://www.w3.org/TR/css-overflow-5/#active-scroll-marker:~:text=If%20scroller%20has%20a%20non%2Dnull%20current%20scroll%20target):
3. If scroller has a non-null current scroll target
Let active be the first item in targets encountered by a reverse tree order walk starting from the current scroll target, or the first item in tree order in targets if no target is found in the previous walk.Rather than adding additional complexity to "pin" and "unpin" markers I would prefer to see the specified fix of having scrollable areas track the last scrolled element and using that as the starting point when updating the active marker.
I agree with your direction, but I don't have any more time to spend on this, unfortunetly.
So that this bug will have to stay until a proper fix is landed then.
Related details
- David A
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
Node* target_node = box.GetNode();Daniil SakhapovHmm should this be `box` or `current_box`? I think `box` is the original thing that a scrollIntoView was requested on, while `current_box` is the current thing within the scroll ancestry that we want to scroll into view.
Robert FlackYeah, you're right. Using only box was causing us to pin markers from other groups because we didn't check which group we are currently scrolling.
But we can't just use current_box either, because we need to find marker for target box, not for the container itself.
So I updated FindScrollMarkerForTargetedScroll to also take group. We still start searching from box, but we stop at the container and only return marker if it belongs to the group we are currently processing.
Daniil SakhapovI don't think we would have this problem of finding the scroll marker to pin if this was implemented the way that was specified in the [resolution](https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2489046785):
RESOLVED: Adopt proposal in https://github.com/w3c/csswg-drafts/issues/10738#issuecomment-2448204794 to anchor to identified elements
In particular, each ancestor scrollable area should set the last targeted element to current_box (or box) and then trigger [step 3 of the marker update algorithm](https://www.w3.org/TR/css-overflow-5/#active-scroll-marker:~:text=If%20scroller%20has%20a%20non%2Dnull%20current%20scroll%20target):
3. If scroller has a non-null current scroll target
Let active be the first item in targets encountered by a reverse tree order walk starting from the current scroll target, or the first item in tree order in targets if no target is found in the previous walk.Rather than adding additional complexity to "pin" and "unpin" markers I would prefer to see the specified fix of having scrollable areas track the last scrolled element and using that as the starting point when updating the active marker.
I agree with your direction, but I don't have any more time to spend on this, unfortunetly.
So that this bug will have to stay until a proper fix is landed then.
I'm okay with landing this bugfix, as long as the complexity is limited to some moved code and the additional logic in this block. However, I think we should leave a TODO referencing https://crbug.com/380062280 to remove this code once the targeted element is properly tracked.
However, this needs to start the search from the previous current_box, otherwise you won't find the correct pinned marker for nested scrollers.
E.g.
```html
<style>
#a, #b {
scroll-marker-group: after;
}
#b::scroll-marker, #c::scroll-marker {
content: "Marker";
}
</style>
<div id=a>
<div id=b>
<div id=c></div>
</div>
</div>
<script>
document.querySelector('#c').scrollIntoView();
</script>
```
In this case, we should pin #c for the group in #b, and pin #b for the group in #a.
Related details
- Daniil Sakhapov
- David A
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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
Node* target_node = box.GetNode();- David A
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |