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

Clarify what getTargetRanges should return when all content in an inline element is removed #158

Open
masayuki-nakano opened this issue Sep 9, 2024 · 14 comments

Comments

@masayuki-nakano
Copy link

I think that this is the first step to define getTargetRanges behavior. (spinning off from #146)

  1. Backspace when <p>ab<span>c[]</span>de</p>
  2. Backspace when <p>ab<span>c</span>[]de</p>
  3. Delete when <p>ab<span>[]c</span>de</p>
  4. Delete when <p>ab[]<span>c</span>de</p>

In all cases, browsers delete <span> rather than only "c". According to input-events-get-target-ranges-backspace.tentative.html and input-events-get-target-ranges-forwarddelete.tentative.html, browsers (try to) return:

  1. <p>ab<span>[c]</span>de</p>
  2. <p>ab<span>[c]</span>de</p>
  3. <p>ab<span>[c]</span>de</p>
  4. <p>ab<span>[c]</span>de</p>

I think that current behavior is better for implementing same behavior in any browsers because if browsers computes the range to delete, they just need to shrink down the range to minimize the range in leaf nodes.

I think that under current definition, the <span> should be included, i.e., <p>ab{<span>c</span>}de</p> is the expected one. However, it's harder to extend the range from performance point of view. Additionally, even if an inline element which gives a style like <b> is deleted from the DOM, new typing text could preserve the style. In such case, including parent elements may make web apps confused. Therefore, I believe that it's the best to shrink the target range as far as possible.

(Although out of scope of this issue, think Backspace when <p>abc</p><p>[]def<br>ghi</p> case. All browsers tries to return a range around </p><p>. However, the first text node def and the <br> in the second paragraph will be moved to the first paragraph, therefore, in strictly conforming to current definition, <p>abc[</p><p>def<br>}ghi</p> should be included, but def will be preserved so that the range is not meaningful for web apps. So, anyway the spec should clarify getTargetRanges behavior, and the my recommending behavior makes the rule of this kind of situations simpler too. Since the first line in the second <p> may be wrapped with some inline elements. Then, browsers do not need to consider whether the range ends before inline elements or at first text.)

@masayuki-nakano
Copy link
Author

Oh, I filed #112 on 2020...

@masayuki-nakano
Copy link
Author

As far as I've checked for Backspace cases, <p>a<span>bc[]</span>d</p> and <p>a<span>bc</span>[]d</p> make getTargetRanges() return <p>a<span>b[c]</span>d</p>. So, shrinking the range into text node or around leaf node (e.g., <br>, <img>) as far as possible may make sense.

@masayuki-nakano
Copy link
Author

masayuki-nakano commented Sep 9, 2024

Backspace when <p>a<span><img></span>[]b</p>, all browsers return <p>a<span>{<img>}</span>b</p> (although Firefox does not delete the <span>, this is a bug).

Backspace when <p>a<span><img><img></span>[]b</p>, all browsers return <p>a<span><img>{<img>}</span>b</p>. So, shrinking target range around leaf node is reasonable for defining the behavior simply.

@masayuki-nakano
Copy link
Author

(Once it's agreed or there should be WPT for considering the final answer, I'll write WPT. However, I'd like to know the opinions before writing tests.)

@johanneswilm
Copy link
Contributor

Also these proposals are general proposals for how contenteditable/execCommand should work, given that the other browser makers are not willing to have divergent target ranges between contenteditable and EditContext, correct?

@johanneswilm
Copy link
Contributor

According to input-events-get-target-ranges-backspace.tentative.html and input-events-get-target-ranges-forwarddelete.tentative.html, browsers (try to) return

Those two tentative tests test things that are not specified by any specification, correct? Therefore we cannot use these as source of truth. I don't necessarily disagree with browsers behaving this way, but given that the browsers will depend on platform editing behavior, and such behavior may change in the future, I don't see how we can specify that.

@masayuki-nakano
Copy link
Author

masayuki-nakano commented Sep 9, 2024

Also these proposals are general proposals for how contenteditable/execCommand should work, given that the other browser makers are not willing to have divergent target ranges between contenteditable and EditContext, correct?

Right, first, getTargetRanges should be defined deeper to make getting same result in EditContext because in contenteditable, the result should represent the result on the browser, but in EditContext, there is no actual result, but web app developers want "recommendation" to delete the range. My suggestion is, browsers should return minimized range to delete. Then, web app developers can do more things if their app should work as so.

(err, but I don't suggest web browsers should change/align the editing result. I just want the normalization rules of target ranges to represent same intentions of browsers with same StaticRange.)

According to input-events-get-target-ranges-backspace.tentative.html and input-events-get-target-ranges-forwarddelete.tentative.html, browsers (try to) return

Those two tentative tests test things that are not specified by any specification, correct?

I wrote the expectations to conform to the old definition of getTargetRnages(). IIRC, that was defined to each target range should contain all nodes will be "affected" by the builtin editor handling.

Therefore we cannot use these as source of truth.

No, the result are truly the browsers how the API implemented. It's useful to discuss the clearer definition. (But the expectations are not useful.)

I don't necessarily disagree with browsers behaving this way, but given that the browsers will depend on platform editing behavior, and such behavior may change in the future, I don't see how we can specify that.

I don't point the differences between platforms (at least in this issue). My point here is, multiple DOM ranges can represent almost same range in the DOM tree. E.g.,

  • {<span>a</span>}
  • {<span>a]</span>
  • <span>{a</span>]
  • <span>{a}</span>
  • <span>[a]</span>
  • <span>{a]</span>
  • <span>[a}</span>
  • etc.

So, there should be a rule to "normalize" target ranges around element boundaries and I'd like the spec clarify the rules. In other words, I'd like to avoid compatibility issues between browsers when they intent to return same target range.

@masayuki-nakano
Copy link
Author

(And it seems that minimized ranges are same as or similar to current browser implementations. So I think that I don't suggest risky spec changes.)

@johanneswilm
Copy link
Contributor

@masayuki-nakano If you only take the most central leave node, how would the user remove the link in a case like this?

  1. <p>Please visit my outdated website: <a href="...">h</a>[].</p>

Hitting backspace once, would create these target ranges, if I understand you right.

  1. <p>Please visit my outdated website: <a href="...">{h}</a>[].</p>

If the beforeinput event is not cancelled, the result will then look like this:

  1. <p>Please visit my outdated website: <a href="...">[]</a>.</p>

The link will be invisible, yet if the user starts typing, the typed text will show up inside the link:

4a. <p>Please visit my outdated website: <a href="...">new typed text[]</a>.</p>

If the user instead uses the arrow keys to move right and left, there will be an empty link stuck there forever, correct?

4b. <p>Please visit my outdated websi[]te: <a href="..."></a>.</p>

@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 12, 2024

@masayuki-nakano The two tentative tests you created and linked to here seem to say that you want getTargetRanges() to return something other than what is the affected range. See for example this comment:

// Should delete the <span> element because it becomes empty.
// However, we need discussion whether the <span> element should be
// contained by a range of getTargetRanges().

https://github.com/web-platform-tests/wpt/blob/868541df96894653a8037568e2137a8481e676a5/input-events/input-events-get-target-ranges-backspace.tentative.html#L74-L77

So you are saying you would want to delete the span as default action, but you don't want to include it in the target range. I don't think I understand why you want want to make a difference there. In the example in my last comment, I think it would be quite bad if an inline element is just left there without any way of being able to remove it.

However, it's harder to extend the range from performance point of view.

What performance are we talking about here? The speed of an app that receives typing is naturally limited by the human not being able to type more than at a certain speed. That's why I don't think we have the same kind of speed requirements as for example 3D games, etc. .But maybe I am just not understanding what kind of performance you are thinking of?

Additionally, even if an inline element which gives a style like <b> is deleted from the DOM, new typing text could preserve the style. In such case, including parent elements may make web apps confused. Therefore, I believe that it's the best to shrink the target range as far as possible.

I wouldn't like browser's to natively do that. But if they do, I assume the next editing operation will include the insertion of the new piece of content.

@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 16, 2024

I think there has been some confusion as to whether browsers should or should not reinsert inline dom nodes when typing after deletion.

I believe:

  1. It is wrong to reinsert dom inline dom nodes when typing after deleting. See Contenteditable re-creating deleted children when it shouldn't editing#468
  2. Chromium and potentially other browsers should be fixed not to do that any more. That should also make them compliant in relation to the Input Events spec.
  3. The ranges returned by getTargetRanges() mentioned above in this issue should correspond to what is actually being deleted. Fixing 2 should also remove any confusion there may still be.

@smaug----
Copy link

There was some (TPAC) discussion related to this in w3c/editing#468 to consider to keep the inline element there and remove it later if needed. That will be investigated hopefully soon either by webkit or chromium folks.
It might prove to be non-web-compatible approach though, in which case we need to come back to this (and probably document what browsers do, which is not what the spec says).

@masayuki-nakano
Copy link
Author

@johanneswilm

2. Chromium and potentially other browsers should be fixed not to do that any more. That should also make them compliant in relation to the Input Events spec.

I commented in w3c/editing#468. I think it's impossible to change the default behavior for backward compatibility. However, I'm open for new API to clear the pending style cache.

3. The ranges returned by `getTargetRanges()` mentioned above in this issue should correspond to what is actually being deleted. Fixing 2 should also remove any confusion there may still be.

I wonder, can web developers trust getTargetRanges()? 😥 (Yes, I should not say so as a browser developer!) In my understanding, editor apps listening to (and prevent) beforeinput modify the DOM by themselves with using the result as a hint. If it's true, simple rule makes it easier browsers to behave exactly same (except the platform/browser specific rules like per word deletion). Therefore, I suggested the simplest one. However, I'm open for other ideas if it's not so potentially risky for the compatibility between browsers.

@johanneswilm
Copy link
Contributor

johanneswilm commented Oct 7, 2024

From TPAC 2024 minutes:

RESOLUTION: We wait for #468 before going further with this.

@johanneswilm johanneswilm removed the Agenda+ Queue this item for discussion at the next WG meeting label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants