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)