Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cssom-view] Remove caret range concept from CaretPosition interface#10230

Open
siliu1 opened this issue Apr 19, 2024 · 9 comments
Open

[cssom-view] Remove caret range concept from CaretPosition interface #10230

siliu1 opened this issue Apr 19, 2024 · 9 comments

Comments

@siliu1
Copy link
Contributor

siliu1 commented Apr 19, 2024

Current spec for CaretPosition interface has a caret range concept which is a live range. The caret range is only used to get the client rect of the CaretPosition. We should avoid creating the live range upfront because:

  1. the live range is not used at all except for getting client rect.
  2. Maintain live range can be expensive during DOM mutation.
  3. No browsers implemented the algorithm by creating live range upfront. (Firefox and Chromium both create a live range only when CaretPosition::getClientRect() is invoked.)

Propose to remove caret range concept from CaretPosition interface.

@sanketj
Copy link
Member

sanketj commented Apr 19, 2024

Completely agree that caret position shouldn't be live or backed by a live range, because of the perf concerns you called out. I think the spec should be updated to explicitly mention that.

@annevk
Copy link
Member

annevk commented Apr 23, 2024

Yeah, I think the current definition is broken as someone could mutate the range and that would impact the result, which isn't what you want to happen.

@sanketj
Copy link
Member

sanketj commented Apr 26, 2024

@smaug---- @zcorpan Any thoughts?

Firefox is the only browser that ships this API at the moment and even Firefox doesn't appear to use a live range in its implementation: https://searchfox.org/mozilla-central/source/dom/base/nsDOMCaretPosition.h#27. So I think we should update the spec, remove the "caret range" concept and explicitly mention that CaretPosition shouldn't be backed by a live range under the hood.

@smaug----
Copy link

yeah, the spec doesn't really make sense. Gecko does internally create a temporary Range just to call getBoundingClientRect and after that the Range is released, so it won't observe mutations to DOM.

I think this stuff comes initially from 532723f

I don't see that being discussed in
https://www.w3.org/Bugs/Public/buglist.cgi?product=CSS&component=CSSOM%20View&resolution=---
So perhaps it came from the mailing list.

Here https://lists.w3.org/Archives/Public/www-style/2009Nov/0244.html
I didn't try to find the relevant TPAC meeting minutes.

Oh, and looks like I complained about the range already 13 years ago :) https://lists.w3.org/Archives/Public/www-style/2011May/0051.html

@siliu1
Copy link
Contributor Author

siliu1 commented May 3, 2024

Thanks for the input. It seems that @smaug---- from Mozilla, @annevk from Apple, and @sanketj and myself from Microsoft all agree that CaretPosition should NOT be backed by a live range.

@astearns Is it possible to get to resolution of this issue without bringing up to CSSWG meeting? Thanks.

@astearns astearns added cssom-view-1 Async Resolution: Proposed Candidate for auto-resolve with stated time limit labels May 3, 2024
@siliu1
Copy link
Contributor Author

siliu1 commented May 6, 2024

cc'ing Googler @mfreed7.

@mfreed7
Copy link

mfreed7 commented May 6, 2024

cc'ing Googler @mfreed7.

LGTM. Incidentally, I'm also in favor of going back in time and making every range a static range, but that's a tougher lift.

@astearns
Copy link
Member

astearns commented May 6, 2024

The CSSWG will automatically accept this resolution one week from now if no objections are raised here. Anyone can add an emoji to this comment to express support. If you do not support this resolution, please add a new comment.

Proposed Resolution: Remove caret range concept (and live ranges) from CaretPosition interface.

@astearns astearns added Async Resolution: Call For Consensus Resolution will be called after time limit expires and removed Async Resolution: Proposed Candidate for auto-resolve with stated time limit labels May 6, 2024
@astearns
Copy link
Member

RESOLVED: Remove caret range concept (and live ranges) from CaretPosition interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants