Bug 194143 – Unable to move selection into editable roots with 0 height

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Unable to move selection into editable roots with 0 height

[Wenson Hsieh]

In particular, this is needed for compatibility with at least one commonly used web-based rich text editor, when pasting content. The relevant code is in Position::isCandidate():

  if (block.logicalHeight() || is<HTMLBodyElement>(m_anchorNode)) {
      …
  }

...wherein we preclude childless non-body elements with 0 height from being the anchor for a VisiblePosition.

[Wenson Hsieh]

We can almost get away with changing that check to this:

  if (block.logicalHeight() || m_anchorNode->isRootEditableElement()) {
      …
  }

...but that causes a couple of problems, as observed in our editing tests. The following two tests:

  editing/pasteboard/datatransfer-items-copy-html.html
  editing/execCommand/indent-no-visible-contents-crash.html

...exercise scenarios in which the potential visible position's anchor node is the body element, but is also non-editable, so isRootEditableElement() returns false. We can fix these tests by doing this instead:

  if (block.logicalHeight() || is<HTMLBodyElement>(m_anchorNode) || m_anchorNode->isRootEditableElement()) {
      …
  }

However, there's a third test failure I have yet to figure out: editing/pasteboard/styled-element-markup.html...

[Wenson Hsieh]

> However, there's a third test failure I have yet to figure out:
> editing/pasteboard/styled-element-markup.html...

So in this test, we try to set the DOMSelection to offset 0 in an editable div, and then we select all, copy, and finally paste. It's important to note that in this test, these steps happen _before_ the image element has loaded, which makes the height of the editable element's renderer 0 because the editable div is otherwise empty.

We're getting different results after the change because we're considering the position Offset(<div>, 0) to be a visible position, whereas we didn't before (instead, the position of Offset(<div>, 0) would be canonicalized to Before(<img>)). Prior to this change, the following would happen when canonicalizing the Position Offset(<div>, 0):

    1. We check upstream and downstream Positions (in this case, they're both still Offset(<div>, 0)) and see if they're candidates by invoking Position::isCandidate.
    2. In Position::isCandidate, the <div>'s RenderBlock's height is 0 because the image hasn't loaded, so we bail and return false.
    3. We continue canonicalization by searching the next and previous positions, and eventually settle on Before(<img>).

After the change, when calling Position::isCandidate in step (2), we notice that the anchor node is a root editable element, despite its RenderBlock's height being 0. This means we'll enter the if statement, observe that hasRenderedNonAnonymousDescendantsWithHeight returns false, and return true from Position::isCandidate because we're at the first editing position in the node.

A few observations:

- It seems like this test was intended to check the case where the image element is not empty (i.e. the image has loaded). And indeed, putting this logic in the load event handler causes the test to pass.
- In the case of a br element instead of an image, we will canonicalize Offset(<div>, 0) to Before(<br>) because the br element has a nonzero height. This means hasRenderedNonAnonymousDescendantsWithHeight returns true, and then we return false from Position::isCandidate for Offset(<div>, 0) because we are not at an editing boundary.
- It's not clear to me whether the new behavior is wrong in this case. One way to ensure that the old behavior remains is to not only check `m_anchorNode->isRootEditableElement()`, but additionally require the Position to be either at an editing boundary, or be the only editing position in the editable root.

[Radar WebKit Bug Importer]

<rdar://problem/47767284>

[Wenson Hsieh]

Created attachment 360996 [details]
Patch

[Wenson Hsieh]

Created attachment 361022 [details]
Patch for landing (w/ typo fix)

[WebKit Commit Bot]

Comment on attachment 361022 [details]
Patch for landing (w/ typo fix)

Clearing flags on attachment: 361022

Committed r240905: <https://trac.webkit.org/changeset/240905>