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

Issue 174349 link

Starred by 27 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Contenteditable with :first-letter styling

Reported by s.dro...@gmail.com, Feb 5 2013

Issue description

UserAgent: Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.1; WOW64; Trident/6.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E)

Steps to reproduce the problem:
1. Create a HTML page with a long paragraph
2. Style the first letter of the paragraph with  p:first-letter
3. Make the surronding paragraph element contenteditable (contenteditable="true")
4. Press newline at any part of the text
5. Press enter before the last character

What is the expected behavior?
A new paragraph, starting left from the cursor with the first letter styled.

Can be viewed in Firefox.

What went wrong?
The Paragraph is staring one character left from the cursor.

In case of the last character, you can't navigate to the end of the text anymore.

Did this work before? N/A 

Chrome version: 24.0.1312.57  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)

Probably problem counting the cursor position.
 
other styling.html
1.2 KB View Download
simple.html
1.2 KB View Download

Comment 1 by tkent@chromium.org, Feb 6 2013

Labels: Area-WebKit WebKit-Editing
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-WebKit -WebKit-Editing Cr-Content Cr-Content-Editing
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 5 2013

Labels: -Cr-Content Cr-Blink
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 6 2013

Labels: -Cr-Content-Editing Cr-Blink-Editing

Comment 5 by yosin@chromium.org, Jul 26 2013

Owner: yosin@chromium.org
Status: Started
Working https://codereview.chromium.org/20681004/

The root cause is Blink implementation doesn't consider text node can associate to two RenderTextFragment objects, one for first letter and another is remaining text.

So, we should change following:
 - HitResult::node()->renderer()
 - Position::anchorNode()->renderer()
 - PositionIterator::anchorNode()->renderer()

Comment 6 by yosin@chromium.org, Aug 1 2013

 Issue 266334  has been merged into this issue.

Comment 7 by yosin@chromium.org, Aug 8 2013

Blockedon: chromium:269556

Comment 8 by yosin@chromium.org, Aug 8 2013

Blockedon: -chromium:269556

Comment 9 by yosin@chromium.org, Aug 8 2013

Blocking: chromium:269556
Blocking: -chromium:269556

Comment 11 by yosin@chromium.org, Aug 26 2013

Cc: nyerramilli@chromium.org
 Issue 278104  has been merged into this issue.
Labels: -OS-Windows -Cr-Blink
A partial patch is in review:  https://codereview.chromium.org/25668008/
 Issue 327112  has been merged into this issue.
 Issue 372732  has been merged into this issue.

Comment 15 by yosin@chromium.org, Jul 11 2014

 Issue 351764  has been merged into this issue.

Comment 16 by yosin@chromium.org, Sep 30 2014

Blockedon: chromium:391288
Cc: dsinclair@chromium.org chrome-editing@google.com yosin@chromium.org
Owner: ----
Status: Available
Once CSS :first-letter pseudo element is a DOM pseudo element, we can handle it easier.
See http://crrev.com/571603003: Convert first letter into a pseudo element.
 Issue 450243  has been merged into this issue.

Comment 18 by yosin@chromium.org, Aug 11 2015

Blocking: chromium:469901

Comment 19 by yosin@chromium.org, Aug 11 2015

Blocking: chromium:484479

Comment 20 by yosin@chromium.org, Aug 11 2015

Blocking: chromium:472093

Comment 21 by yosin@chromium.org, Aug 11 2015

Blocking: chromium:269556

Comment 22 by yosin@chromium.org, Aug 11 2015

Blocking: chromium:406218
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 12 2015

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=200407

------------------------------------------------------------------
r200407 | yosin@chromium.org | 2015-08-12T18:16:40.715044Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Position.cpp?r1=200407&r2=200406&pathrev=200407
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutText.h?r1=200407&r2=200406&pathrev=200407
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutText.cpp?r1=200407&r2=200406&pathrev=200407

Move Position::isRenderedCharacter() for Text node to LayoutText

This patch moves an implementation of |Position::isRenderedCharacter()| for
|Text| node to |LayoutText| to reduce usage of |InlineBox| in |Position| class
for improving code health and helping identify layout API for editing.

This patch is a preparation of handling of :first-letter pseudo element in
selection.

Following patch will move other |InlineBox| references in editing/ to layout/.

BUG=174349,  518738 
TEST=n/a; no behavior changes

Review URL: https://codereview.chromium.org/1285123002
-----------------------------------------------------------------
Blocking: chromium:528471

Comment 25 by yosin@chromium.org, Sep 14 2015

Issue 17528 has been merged into this issue.

Comment 26 by yosin@chromium.org, Sep 14 2015

Blocking: chromium:17528
Project Member

Comment 27 by bugdroid1@chromium.org, Sep 23 2015

Labels: merge-merged-2490
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7e11caa505f809e433d8f2cf11f92f25e1a667e

commit b7e11caa505f809e433d8f2cf11f92f25e1a667e
Author: yosin@chromium.org <yosin@chromium.org>
Date: Wed Aug 12 18:16:40 2015

Move Position::isRenderedCharacter() for Text node to LayoutText

This patch moves an implementation of |Position::isRenderedCharacter()| for
|Text| node to |LayoutText| to reduce usage of |InlineBox| in |Position| class
for improving code health and helping identify layout API for editing.

This patch is a preparation of handling of :first-letter pseudo element in
selection.

Following patch will move other |InlineBox| references in editing/ to layout/.

BUG=174349,  518738 
TEST=n/a; no behavior changes

Review URL: https://codereview.chromium.org/1285123002

git-svn-id: svn://svn.chromium.org/blink/trunk@200407 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[modify] http://crrev.com/b7e11caa505f809e433d8f2cf11f92f25e1a667e/third_party/WebKit/Source/core/dom/Position.cpp
[modify] http://crrev.com/b7e11caa505f809e433d8f2cf11f92f25e1a667e/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] http://crrev.com/b7e11caa505f809e433d8f2cf11f92f25e1a667e/third_party/WebKit/Source/core/layout/LayoutText.h

Comment 28 by yosin@chromium.org, Sep 25 2015

Blocking: chromium:534785

Comment 29 by yosin@chromium.org, Sep 29 2015

Blocking: chromium:536914
Blocking: chromium:537888

Comment 31 by yosin@chromium.org, Oct 19 2015

Blocking: chromium:543886

Comment 32 by yosin@chromium.org, Oct 22 2015

Blocking: chromium:545520
Project Member

Comment 33 by bugdroid1@chromium.org, Nov 5 2015

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

commit 183a61e97bbb014b3b0a55e92cca03151632d1fe
Author: yosin <yosin@chromium.org>
Date: Thu Nov 05 10:05:52 2015

Make most{Backward,Forward}CaretPosition() to handle first-letter pseudo element

Before this patch, Blink doesn't work well for first-letter pseudo element
by two reasons:
 1. Blink doesn't use first-letter text and uses only remaining text.
 2. Scanning |InlineTextBox| with DOM offset rather than offset of
    |LayoutText::m_text|

This patch makes |most{Backward,Forward}CaretPosition()| and associated
functions to handle first-letter pseudo element with considering above.

This patch also updates layout test expectation texts as result of correct
handling of first-letter pseudo element.

Background:
In layout tree, |Text| node having first-letter style represented by two
|LayoutTextFragment| objects, one for first-letter text and another for
remaining text. |LayoutTextFragment| is derived from |LayutText|.

Note: first-letter text can be multiple characters, which
can contain leading whitespaces of |Text| node and punctuation characters
and a letter character.

|LayoutText::m_text| holds a text for layout/paint, which is different from
text in DOM node, e.g. applying CSS text-transform.

BUG=174349,  545520 
TEST=webkit_unit_tests --gtest_filter=VisibleUnits.mostBackwardCaretPositionFirstLetter
TEST=webkit_unit_tests --gtest_filter=VisibleUnits.mostForwardCaretPositionFirstLetter

Review URL: https://codereview.chromium.org/1404423005

Cr-Commit-Position: refs/heads/master@{#358020}

[modify] http://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe/third_party/WebKit/LayoutTests/fast/text/first-letter-bad-line-boxes-crash-expected.txt
[modify] http://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe/third_party/WebKit/LayoutTests/fast/text/insert-text-crash-expected.txt
[modify] http://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] http://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe/third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp
[modify] http://crrev.com/183a61e97bbb014b3b0a55e92cca03151632d1fe/third_party/WebKit/Source/core/layout/LayoutTextFragment.h

Cc: ssamanoori@chromium.org
Labels: Needs-Feedback
Tested the issue on Windows 7 using 48.0.2560.0 with below steps:

1.Opened attached simple.html in chrome.
2.Pressed enter before the last character.
3.Observed that the style is applied to one character left from the cursor.

Please find attached screen cast and confirm anything missed here.

Could anyone please provide us the repro steps to verify the fix from test team end.

Thanks,

174349.mp4
623 KB Download

Comment 35 by yosin@chromium.org, Nov 24 2015

Blocking: chromium:559098

Comment 36 by yosin@chromium.org, Nov 24 2015

Blocking: chromium:560689
Cc: ranjitkan@chromium.org
Could any one please update us as requested in comment # 34.

Comment 38 by yosin@chromium.org, Jan 22 2016

Blocking: chromium:570253

Comment 39 by yosin@chromium.org, Jan 27 2016

Blocking: chromium:581209
The bug still exists in Chromium 49.0.2623.108. On simple.html, put the cursor between the last t and the period of the entire paragraph. Hit enter. The t and period both disappear.

test.mkv
311 KB Download

Comment 41 by yosin@chromium.org, Jun 27 2016

Blocking: 619379
Blocking: 435778

Comment 43 by donnd@chromium.org, May 11 2017

Cc: donnd@chromium.org

Comment 44 by yosin@chromium.org, Sep 15 2017

Blockedon: 762859
Labels: Pri-3

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

Blockedon: 808516

Sign in to add a comment