Cache live Range in document in SelectionController [chromium/src : main]
Dominic Farolino (Gerrit)
Dominic Farolino added 2 comments![]( "Open in Gerrit")
- Clear cache on state preserving atomic moveBeforeDo you know why we weren't doing this before? Was it an oversight, or was it because we didn't have the caching that this CL provided.
[FAIL] anchorNode snaps up to parent when removed (no asserts)I agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
Related details
- Chris Harrelson
- Orko Garai
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. |
Orko Garai (Gerrit)
Orko Garai added 2 comments![]( "Open in Gerrit")
- Clear cache on state preserving atomic moveBeforeDo you know why we weren't doing this before? Was it an oversight, or was it because we didn't have the caching that this CL provided.
This was an issue always IIUC. It seems that because we weren't caching automatically when selecting using mouse, we didn't need to clear the cache for the tests to pass since the moveBefore tests already accounted for it, and didn't call anchorNode() or focusNode() before moveBefore. But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
[FAIL] anchorNode snaps up to parent when removed (no asserts)I agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
IIUC the atomic moveBefore and node removal cases are to be handled differently, i.e. in case of atomic moveBefore the stale cache needs to be cleared to re-calculate the proper selection after the move. But in case of node removal, a remove mutation is to be performed on the existing selection. At least that's what seems to be the case based on the WPT expectations for these two cases. Or is this a test bug?
Related details
- Chris Harrelson
- Dominic Farolino
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 2 comments![]( "Open in Gerrit")
- Clear cache on state preserving atomic moveBeforeOrko GaraiDo you know why we weren't doing this before? Was it an oversight, or was it because we didn't have the caching that this CL provided.
This was an issue always IIUC. It seems that because we weren't caching automatically when selecting using mouse, we didn't need to clear the cache for the tests to pass since the moveBefore tests already accounted for it, and didn't call anchorNode() or focusNode() before moveBefore. But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
Moving this conversation down to the other comment...
[FAIL] anchorNode snaps up to parent when removed (no asserts)Orko GaraiI agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
IIUC the atomic moveBefore and node removal cases are to be handled differently, i.e. in case of atomic moveBefore the stale cache needs to be cleared to re-calculate the proper selection after the move. But in case of node removal, a remove mutation is to be performed on the existing selection. At least that's what seems to be the case based on the WPT expectations for these two cases. Or is this a test bug?
Continuing here from the other comment...
But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
OK so in the atomic move case, you could observe the bug by:
1. Mouse selection
2. Observing `node.anchorNode`
3. Calling `moveBefore()`
This is because observing `anchorNode` produced a cache that didn't get updated during the move. As long as you didn't observe `anchorNode`, then there wasn't even a cache that could get out of date during the move, so you wouldn't see the bug.
But the actual `remove()` case is the exact opposite, it seems? Before this CL, observing `anchorNode` before `remove()` creates a cached range that gets properly mutated during `remove()`. But why does omitting the `anchorNode` observation before `remove()` cause the asserts *after* it to fail?
Related details
- Chris Harrelson
- Orko Garai
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. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
[FAIL] anchorNode snaps up to parent when removed (no asserts)Orko GaraiI agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
Dominic FarolinoIIUC the atomic moveBefore and node removal cases are to be handled differently, i.e. in case of atomic moveBefore the stale cache needs to be cleared to re-calculate the proper selection after the move. But in case of node removal, a remove mutation is to be performed on the existing selection. At least that's what seems to be the case based on the WPT expectations for these two cases. Or is this a test bug?
Continuing here from the other comment...
But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
OK so in the atomic move case, you could observe the bug by:
1. Mouse selection
2. Observing `node.anchorNode`
3. Calling `moveBefore()`This is because observing `anchorNode` produced a cache that didn't get updated during the move. As long as you didn't observe `anchorNode`, then there wasn't even a cache that could get out of date during the move, so you wouldn't see the bug.
But the actual `remove()` case is the exact opposite, it seems? Before this CL, observing `anchorNode` before `remove()` creates a cached range that gets properly mutated during `remove()`. But why does omitting the `anchorNode` observation before `remove()` cause the asserts *after* it to fail?
But why does omitting the anchorNode observation before remove() cause the asserts after it to fail?
Because DOMSelection uses visible range and calculating visible range for the first time after removing the anchor has a different value as opposed to applying a mutation on an existing selection causing it to "snap up" to the parentDiv with the same offset where the parentParagraph was initially. This "snapping up" wouldn't happen if we just calculated a new visual selection, and thus return an anchor value where the child paragraph is. So the question here is which is correct. According the the wpt test it should be the "snapped up" value, correct?
Related details
- Chris Harrelson
- Dominic Farolino
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")
[FAIL] anchorNode snaps up to parent when removed (no asserts)Orko GaraiI agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
Dominic FarolinoIIUC the atomic moveBefore and node removal cases are to be handled differently, i.e. in case of atomic moveBefore the stale cache needs to be cleared to re-calculate the proper selection after the move. But in case of node removal, a remove mutation is to be performed on the existing selection. At least that's what seems to be the case based on the WPT expectations for these two cases. Or is this a test bug?
Orko GaraiContinuing here from the other comment...
But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
OK so in the atomic move case, you could observe the bug by:
1. Mouse selection
2. Observing `node.anchorNode`
3. Calling `moveBefore()`This is because observing `anchorNode` produced a cache that didn't get updated during the move. As long as you didn't observe `anchorNode`, then there wasn't even a cache that could get out of date during the move, so you wouldn't see the bug.
But the actual `remove()` case is the exact opposite, it seems? Before this CL, observing `anchorNode` before `remove()` creates a cached range that gets properly mutated during `remove()`. But why does omitting the `anchorNode` observation before `remove()` cause the asserts *after* it to fail?
But why does omitting the anchorNode observation before remove() cause the asserts after it to fail?
Because DOMSelection uses visible range and calculating visible range for the first time after removing the anchor has a different value as opposed to applying a mutation on an existing selection causing it to "snap up" to the parentDiv with the same offset where the parentParagraph was initially. This "snapping up" wouldn't happen if we just calculated a new visual selection, and thus return an anchor value where the child paragraph is. So the question here is which is correct. According the the wpt test it should be the "snapped up" value, correct?
Because DOMSelection uses visible range and calculating visible range for the first time after removing the anchor has a different value as opposed to applying a mutation on an existing selection causing it to "snap up"
Right, I guess this gets at the heart of my question then. Why does calculating visible range for the first time after anchor removal have a different value compared with applying amutation on an existing cached range?
So the question here is which is correct. According the the wpt test it should be the "snapped up" value, correct?
Yes, I think this CL produces the correct behavior. I'm just really trying to understand why. Can you comment on why calculating visual selection without a cache produces a different range than if the cache is present? My understanding is that this CL fixes the problem by always making sure that the visual selection is backed by a cached range. Is this what the spec does too? If so, then I think we're probably good to land this.
Related details
- Chris Harrelson
- Orko Garai
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. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
[FAIL] anchorNode snaps up to parent when removed (no asserts)Orko GaraiI agree with the changes in `selection-preserve.html`, but now that I look at this closer, I'm confused why this was failing? The test in `selection-preserve.html` shows that to work around the Blink selection bug, you should NOT observe things like `anchorNode` before moving or removing the node.
But looking at this expectation file, it looks like the version of the test that did NOT observe `anchorNode` first failed, while the one that DID observe it passed. Do you know what's going on?
Dominic FarolinoIIUC the atomic moveBefore and node removal cases are to be handled differently, i.e. in case of atomic moveBefore the stale cache needs to be cleared to re-calculate the proper selection after the move. But in case of node removal, a remove mutation is to be performed on the existing selection. At least that's what seems to be the case based on the WPT expectations for these two cases. Or is this a test bug?
Orko GaraiContinuing here from the other comment...
But if in some real world use case those functions are called from a web application after mouse selection and before moveBefore, this was still an issue and provided different results vs. those functions not being called.
OK so in the atomic move case, you could observe the bug by:
1. Mouse selection
2. Observing `node.anchorNode`
3. Calling `moveBefore()`This is because observing `anchorNode` produced a cache that didn't get updated during the move. As long as you didn't observe `anchorNode`, then there wasn't even a cache that could get out of date during the move, so you wouldn't see the bug.
But the actual `remove()` case is the exact opposite, it seems? Before this CL, observing `anchorNode` before `remove()` creates a cached range that gets properly mutated during `remove()`. But why does omitting the `anchorNode` observation before `remove()` cause the asserts *after* it to fail?
Dominic FarolinoBut why does omitting the anchorNode observation before remove() cause the asserts after it to fail?
Because DOMSelection uses visible range and calculating visible range for the first time after removing the anchor has a different value as opposed to applying a mutation on an existing selection causing it to "snap up" to the parentDiv with the same offset where the parentParagraph was initially. This "snapping up" wouldn't happen if we just calculated a new visual selection, and thus return an anchor value where the child paragraph is. So the question here is which is correct. According the the wpt test it should be the "snapped up" value, correct?
Because DOMSelection uses visible range and calculating visible range for the first time after removing the anchor has a different value as opposed to applying a mutation on an existing selection causing it to "snap up"
Right, I guess this gets at the heart of my question then. Why does calculating visible range for the first time after anchor removal have a different value compared with applying amutation on an existing cached range?
So the question here is which is correct. According the the wpt test it should be the "snapped up" value, correct?
Yes, I think this CL produces the correct behavior. I'm just really trying to understand why. Can you comment on why calculating visual selection without a cache produces a different range than if the cache is present? My understanding is that this CL fixes the problem by always making sure that the visual selection is backed by a cached range. Is this what the spec does too? If so, then I think we're probably good to land this.
Based on https://issues.chromium.org/issues/379275917#comment3 and prior fix attempts, it seemed like that was expected and caching in selection_controller could be a potential solution and so I focused on that (and fixing any regressions). I can investigate some more and report back, but if others more familiar with how visible selection works in this case, please feel free to chime in.
Related details
- Chris Harrelson
- Dominic Farolino
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. |
Orko Garai (Gerrit)
@d...@chromium.org Here's what I found:
In the [anchor-removal.html](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/selection/anchor-removal.html;l=25-31;drc=b4ccf86f687e3d3b3cb487efe628c57a1cbfc531;bpv=0;bpt=0) test, initially the anchor is "Parent paragraph". So when `parentParagraph` is removed, if there is a live range, the anchor needs to be updated to `parentDiv"@offsetInAnchor[1]` as per [live range pre-remove steps](https://dom.spec.whatwg.org/#live-range-pre-remove-steps).
There was no live range to begin with if there is no assert before the anchor removal. So when we try to [create a new live range](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/editing/dom_selection.cc;l=684;drc=b4ccf86f687e3d3b3cb487efe628c57a1cbfc531;bpv=0;bpt=0), it uses a [visible range computed from the current SelectionInDomTree](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/editing/selection_editor.cc;l=486;drc=b4ccf86f687e3d3b3cb487efe628c57a1cbfc531;bpv=0;bpt=0). Even though the `SelectionInDomTree` stored in the SelectionEditor gets updated to reflect the correct "snapped up" anchor `DIV id="parentDiv"@offsetInAnchor[1]`, the visible selection computed from it would always return a "canonicalized"/"normalized" anchor which will be the [next visually equivalent candidate](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/editing/visible_units.cc;l=199;drc=b4ccf86f687e3d3b3cb487efe628c57a1cbfc531;bpv=0;bpt=0) `#text "Child paragraph one"@offsetInAnchor[0]`.
So it's not that the VisibleSelection is different depending on whether a live range is cached or not. The VisibleSelection will always point to the child paragraph as it's "visually equivalent". The issue is that when we construct a new live range we use VisibleSelection at that point, which produces a different live Range after removal vs. [applying the effect of anchor removal on an existing live range](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/range.cc;l=1490;drc=b4ccf86f687e3d3b3cb487efe628c57a1cbfc531;bpv=0;bpt=0).
It seems the test without assert implicitly expects there to be a live range already before the anchor removal and for that range to be updated as per the live range pre-remove steps in the spec. While I don't see this expectation of a live range to always exist explicitly mentioned in the spec, if this is indeed the correct expectation, is this fix of ensuring a live range is created and cached in SelectionController not what we want? And maybe this should be clarified in the spec as well? Otherwise, should we remove this `(no-asserts)` sub-test or update it to also support the "visually equivalent" anchor or mark it tentative until it's clarified in the spec?
An alternative approach as attempted in https://crrev.com/c/6132497 is creating a new Range using the `SelectionInDomTree` instead of using `VisibleSelection`. Do you think we should pursue that instead (although the side-effects of such a change will need to be evaluated again)? It will provide the correct results as well, but not by ensuring a live range exists as per the test expectation here.
FWIW I found some additional context about visible selection vs. interop here: [Improving Interoperatbility of Selection](https://goo.gl/9v1zOK)
Orko Garai (Gerrit)
@d...@chromium.org gentle ping
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Even though the SelectionInDomTree stored in the SelectionEditor gets updated to reflect the correct "snapped up" anchor DIV id="parentDiv"@offsetInAnchor[1], the visible selection computed from it would always return a "canonicalized"/"normalized" anchor which will be the next visually equivalent candidate #text "Child paragraph one"@offsetInAnchor[0].
The VisibleSelection will always point to the child paragraph as it's "visually equivalent". The issue is that when we construct a new live range we use VisibleSelection at that point, which produces a different live Range after removal vs. applying the effect of anchor removal on an existing live range.
So compared to live ranges, VisibleSelection just gets updated/modified differently, with different semantics: i.e., choosing the "next visually equivalent" vs. using the live range snapping rules. Is this spec'd? I guess not, since I only see "visually equivalent" appear a couple of times in https://w3c.github.io/selection-api/#dom-selection-containsnode, and nowhere else. That's unfortunate, if I'm correct.
It seems the test without assert implicitly expects there to be a live range already before the anchor removal and for that range to be updated as per the live range pre-remove steps in the spec. While I don't see this expectation of a live range to always exist explicitly mentioned in the spec, if this is indeed the correct expectation, is this fix of ensuring a live range is created and cached in SelectionController not what we want?
I actually do think the spec says there should always be a live range backing the selection. Consider https://w3c.github.io/selection-api/#ref-for-dfn-selection-2, which says a selection should always have a range. See https://w3c.github.io/selection-api/#user-interactions which describes this range that always backs the selection, as well as text like https://w3c.github.io/selection-api/#ref-for-dfn-selection-2:~:text=If%20the%20user%20creates,anchor%20is%20the%20end too, which indicates that the range is created whenever the visual selection is created (even before selection APIs are invoked by script), and is updated accordingly.
Does this answer your question? Since there is a range backing the selection at all times, it seems like how we update our visual selection is just not spec'd, and your change here kind of covers up that problem which is good.
Related details
- Chris Harrelson
- Orko Garai
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. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
Yes, VisibleSelection seems like an internal concept and not spec'd based on what I gathered. It can be used to produce the correct initial live range, but it doesn't follow the snapping rules after.
Regarding the live ranges always backing selections, I wasn't entirely sure earlier from reading the spec but your explanation helps clarify the expectation there.
So it seems that we DO want to ensure a live range exists initially as per the spec, which this change ensures, in which case is it acceptable to proceed with this change?
Whether we want to continue using VisibleSelection to create the initial live range is the other question. It seems to not have any issues currently and changing that could be risky as well. But I suppose that is a separate problem from ensuring a live range exists initially?
Related details
- Chris Harrelson
- Dominic Farolino
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
1 comment
So it seems that we DO want to ensure a live range exists initially as per the spec, which this change ensures, in which case is it acceptable to proceed with this change?
I *think* that's correct, yes. I'll approve this change but let's hold off on landing it until siliu@ weighs in, since they're explicitly a core/editing OWNER. It would be great to get a second opinion.
Related details
- Chris Harrelson
- Orko Garai
- Siye Liu
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. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
- Chris Harrelson
- Siye Liu
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. |
Utkarsh Pathak (Gerrit)
Utkarsh Pathak added 1 comment![]( "Open in Gerrit")
As Dominic also referenced to the spec https://w3c.github.io/selection-api/#ref-for-dfn-selection-2, a selection should always have a range associated with it, which is actually ensured with this change so this change LGTM. But we can also have a second set of eyes from @ashi...@microsoft.com who worked on similar lines in https://chromium-review.googlesource.com/c/chromium/src/+/6132497
Related details
- Ashish Kumar
- Chris Harrelson
- Orko Garai
- Siye Liu
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. |
Ashish Kumar (Gerrit)
Ashish Kumar added 2 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::
SelectionControllerCacheRangeOfDocumentEnabled() &&
GetDocument().StatePreservingAtomicMoveInProgress()) {
ClearDocumentCachedRange();
}Why is this needed?
The change mostly lgtm, with a doubt in the selection_editor.
Related details
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::
SelectionControllerCacheRangeOfDocumentEnabled() &&
GetDocument().StatePreservingAtomicMoveInProgress()) {
ClearDocumentCachedRange();
}Why is this needed?
The previously calculated range becomes invalid during atomic moveBefore. So this forces a re-calculation. Without this the moveBefore tests would fail.
Related details
- Ashish Kumar
- Chris Harrelson
- Siye Liu