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

Should backspace merge text with last cell's content of a table above ? #164

Closed
javifernandez opened this Issue Jun 9, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@javifernandez

javifernandez commented Jun 9, 2017

Let's consider the following example:

<div contenteditable=true>
  <table border=1>
    <tr>
      <td>Cell 1</td>
      <td>Cell 2</td>
    </tr>
  </table>
Testing
</div>

When placing the editing cursor at "T^esting", hitting backspace twice behaves differently in each browser:

When I tried on Chrome/Safari,

  • First backspace: removed "T" then move caret before "e"
  • Second backspace: Move "esting" after "2"

When I tried on Edge,

  • First backspace: removed "T" then move caret after "2"
  • Second backspace: remove "2"

When I tried on Firefox:

  • First backspace: removed "T" then move caret before "e"
  • Second backspace: Does nothing

Bear in mind that Chrome and Safari having the same behavior doesn't mean an explicit consensus; it's just that both engines still share the same codebase for that use case.

@frivoal

This comment has been minimized.

frivoal commented Jun 9, 2017

In a related but different case, note that if you select from "2" to "e" (inclusive), browsers also behave differently from each-other.

Similarly to the comment I left in #163 (comment), I wonder what is the best approach to specify this, assuming we can agree on the preferred behavior.

@johanneswilm

This comment has been minimized.

Contributor

johanneswilm commented Jun 9, 2017

This one is more tricky. One might also expect it to select the table with the second backspace. Unless everyone can magically agree, I would think this is best left for JS editors to test and figure out and come up with a defacto standard over time, which then can be put into a spec.

@javifernandez

This comment has been minimized.

javifernandez commented Jun 13, 2017

I think that it's agreed that hitting backspace with the cursor set right after a table in the same line will select the whole table; further backspace hit will delete the whole table. I think this is the same behavior for images inside a contenteditable areas.

I think it makes sense that when placing the cursor in the text paragraph bellow, once reaching the start of the paragraph by hitting backspace it should be moved to the last position of the line above; in this case just after the table. Hence, we are in the scenario described above, where additional backspace hit will select the table.

@javifernandez

This comment has been minimized.

javifernandez commented Mar 7, 2018

It seems this issue is stalled. Considering that FF stops the backspace when reaching the beginning of the paragraph, could we agreed on that behavior as a compromise solution while we think on smarter ways if dealing with the table above ? I'll do the change in WebKit and Blink, so there will be 3 major browsers with the same behavior: clearly a win in terms of interoperability.

Besides the interoperability issue we have now, I think that merging the contents outside the table, from the paragraph bellow is really weird. I'd say more, it's a bug.

What do you think ?

@javifernandez

This comment has been minimized.

javifernandez commented Mar 8, 2018

Let me add some additional thoughts about this specific case and how it fir in the current spec.

1- This use case corresponds to section "9.12 The delete command" (same as hitting backspace).

In the note at the beginning, there is the following statement:

Backspacing at the start of most blocks merges with the previous block. (Visually, this is a matter of deleting a line break.)

I really think the above statement is a bit ambiguous; using the the term "most" gives poor information. At least, we should list there the blocks we already consider not valid candidates for block merging. Maybe tables ?

2- This use case corresponds to the step 16 "General block-merging case."

I guess that we can consider that start node is <div contenteditable=true> as soon as we hit backspace with the cursor just after the character "T". We could also have thought that we are at the anonymous block that contains the text, with offset 0, but then we would need an additional backspace to activate the block merging (if I have understood correctly the algorithm).

So, we are at start_node <div contenteditable=true> and start_offset 1. There is indeed a start_node's child with index start_offset -1, which is precisely the table. The first condition doesn't apply because the table is visible, but the second condition does apply.

So, start_node is set to the <table> node. I'm having hard time to figure out how to set the start_offset to length of the start_node in this case (I assume there is something about how to do that for table elements). Lets assume start_node ends up being the last <td> of the last <tr> in the table and the start_offset the length of such <td> element.

Once we are here, we can indeed merge the anonymous block containing the rest of the text ("esting") with the contents of the <td> In order to do that, we must follow the steps 17, 18 and 19 with the start_node and start_offset defined above.

And here is where I see a contradiction in the current spec. The step 19 is precisely the section "9.5 Deleting the selection". In such section there is a note at the step 14 that states:

We don't let either start block or end block be a td or th. This means we'll never merge to or from a td or th.

This statement implies that we can't merge contents of a different block into the

elements. This is compatible with the ambiguous statement that I remarked at the beginning; so perhaps we can define tables a not valid candidates for block merging after all, as I suggested before.

Such note also states:

This matches Firefox 5.0a2, and reportedly Word as well. Chrome 13 dev and Opera 11.11 allow merging from a non-table cell end block to a table cell start block, but not vice versa. In IE9 the delete key just does nothing.

This is just a way of saying that there is no interoperability in this regard and I think the spec should aim for solving that issue. Considering that FF and Edge are compatible and that this proposal is to make Safari and Chrome follow that path, I think we should seriously considering it.

@javifernandez

This comment has been minimized.

javifernandez commented Mar 19, 2018

@johanneswilm could you add some feedback on this issue ? I'm trying to convince Chromium and Safari to change their behavior so both match Edge and Firefox, but I'd need some kind of consensus here before. It'd be a clear win in terms of interoperability, so I think it'd worth the effort.

@rniwa

This comment has been minimized.

Contributor

rniwa commented Mar 19, 2018

We're not going to change WebKit's behavior here since this is what TextEdit and other native applications on macOS does.

@javifernandez

This comment has been minimized.

javifernandez commented Mar 19, 2018

Ok, thanks for the feedback.

@javifernandez

This comment has been minimized.

javifernandez commented Mar 20, 2018

I've been checking out other editor's behavior and got the following, so far:

  • Google Doc: Content is not merged in the table above; backspace stops at the beginning of the paragraph.
  • MS Word: Content is not merged in the table above; backspace continues in the paragraph above, deleting the table's content.

aarongable pushed a commit to chromium/chromium that referenced this issue Mar 28, 2018

Backspacing shouldn't merge content into tables except for macOS
When uses hits backspace at the start of a paragraph, the content in
that block is merged in the previous block in the paragraph above. All
the browsers have the same behavior except in the case of such previous
block being a table.

Firefox and Edge both avoid merging the content into the table. I've
filed a issue in the Editing TF [1] to get some consensus, but it seems
that current behavior was implemented to match TextEdit and other native
text editing apps in macOS.

Considering that both MS Word and Google Docs avoid merging content on
backspacing, I think it's a good option for the end user that Chrome
implements that behavior as well, improving also the browsers
interoperability (we'll match Firefox and Edge).

I'll keep the current logic for macOS using an EditingBehavior flag, so
we can switch to a different behavior based on platform.

[1] w3c/editing#164

Bug: 731000
Change-Id: Ibc5cad576b7115877a36929f717ff5ed194d0977
Reviewed-on: https://chromium-review.googlesource.com/981137
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546475}
@johanneswilm

This comment has been minimized.

Contributor

johanneswilm commented Apr 28, 2018

@javifernandez Sorry, somehow missed this. The change you made in Chromium seems OK. I would expect that most if not all JavaScript editors to implement their own code for dealing with everything beyond certain simple text node operation by themselves. That being said, even if JS editors implement things themselves, it will make their job easier if all major browsers behave the same.

I looked at Apple iCloud Pages in Safari on Mac -- it doesn't seem to follow the behavior of native apps there either. Instead it seems to not react at all the first time backspace is hit, and the second time it deletes the table. It doesn't merge content into the table

@johanneswilm

This comment has been minimized.

Contributor

johanneswilm commented Apr 28, 2018

@javifernandez Do you still want us to change the text of the draft spec even though you are aware that not all browser will follow the text there?

@javifernandez

This comment has been minimized.

javifernandez commented Apr 29, 2018

I'm not sure about the change in the spec, given that not all the browsers will follow, as you said. Current spec, stating that there will be such different seems enough for now.

@johanneswilm

This comment has been minimized.

Contributor

johanneswilm commented Apr 30, 2018

Ok, closing this issue then. Please reopen if anyone disagrees!

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