New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 808516 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug

Blocking:
issue 174349



Sign in to add a comment

Deleting anything in a content editable div with ::first-letter CSS deletes everything.

Reported by jmadde...@gmail.com, Feb 2 2018

Issue description

Chrome Version       : 63.0.3239.132
URLs (if applicable) : https://codepen.io/jmadden85/pen/wyMPoG
Other browsers tested: Firefox@57.0.1 OK, Safari@11.0.3 OK, Edge@41.16299.15.0 
  Add OK or FAIL, along with the version, after other browsers where you
have tested this issue:
     Safari: OK 11.0.3
    Firefox: OK 57.0.1
       Edge: OK (different issues) 41.16299.15.0 

What steps will reproduce the problem?
(1) Create a div and add the contenteditable attribute.
(2) Give the div the CSS-Pseudo-Element ::first-letter with any styling (tested with color, font-size, font-weight).
(3) Enter some text into the div.
(4) Select any part of the text
(5) Hit delete

What is the expected result?
The selection, or if no selection the text immediately to the left of the caret should be deleted.

What happens instead?
The entire text is deleted.


Please provide any additional information below. Attach a screenshot if
possible.

 
Labels: Needs-Triage-M63
Components: Blink>Editing
Cc: vamshi.k...@techmahindra.com
Labels: -Pri-3 Triaged-ET M-66 FoundIn-66 Target-66 OS-Linux OS-Mac OS-Windows Pri-2
Thanks for filing the issue!

Able to reproduce the issue on reported chrome version 63.0.3239.132 and on the latest canary 66.0.3344.0 using Windows 10, Ubuntu 14.04 and Mac 10.13.1. As the issue is seen from M60(60.0.3112.30) considering it as Non-Regression and marking it as Untriaged. 

Comment 4 by yosin@chromium.org, Feb 14 2018

Blocking: 174349
Components: -Blink>Editing Blink>Editing>Command
Status: Available (was: Unconfirmed)
It seems DeleteSelectionCommand is broken with ::first-letter.

Comment 5 by yosin@chromium.org, Feb 15 2018

Owner: yosin@chromium.org
Status: Started (was: Available)
In review: http://crrev.com/c/920051

Comment 6 by yosin@chromium.org, Feb 15 2018

The text node is removed by RemoveNode() call in below:

void DeleteSelectionCommand::HandleGeneralDelete(...) {
...
    // The selection to delete is all in one node.
    if (!start_node->GetLayoutObject() ||
        (!start_offset && downstream_end_.AtLastEditingPositionForNode())) {
      RemoveNode(start_node, editing_state);
      if (editing_state->IsAborted())
        return;
    }
Because DeleteTextFromNode() detaches layout object from |start_node|


Detaching layout object is done by Text::UpdateTextLayoutObject()
by ShouldUpdateLayoutByReattaching() returns true.



static bool ShouldUpdateLayoutByReattaching(const Text& text_node,
                                            LayoutText* text_layout_object) {
...
  if (text_layout_object->IsTextFragment()) {
    // Changes of |textNode| may change first letter part, so we should
    // reattach.
    return ToLayoutTextFragment(text_layout_object)
        ->GetFirstLetterPseudoElement();
  }
  return false;
}


Project Member

Comment 7 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1ed8403d631abd0a3d010cf03794449c9289bff

commit c1ed8403d631abd0a3d010cf03794449c9289bff
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Feb 16 06:41:15 2018

Make DeleteSelectionCommand to handle ::first-letter correctly

This patch changes |DeleteSelectionCommand::HandleGeneralDelete()| to use the
clean layout tree for calling |start_node->GetLayoutObject()| after calling
|DeleteTextFromNode()| or |RemoveChildrenInRange()| to avoid removing
|start_node|.


Note: When |Text| node with ::first-letter is modified, Blink detach
layout object from it in |Text::UpdateTextLayoutObject()| to follow
|ShouldUpdateLayoutByReattaching()| which returns true for |true| for
|Text| node with ::first-letter.

Bug:  808516 
Change-Id: I7139398ca3938e56960c81c9196bd58e91bed668
Reviewed-on: https://chromium-review.googlesource.com/920051
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537233}
[modify] https://crrev.com/c1ed8403d631abd0a3d010cf03794449c9289bff/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
[modify] https://crrev.com/c1ed8403d631abd0a3d010cf03794449c9289bff/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp

Comment 8 by yosin@chromium.org, Feb 27 2018

Status: Fixed (was: Started)

Sign in to add a comment