Last Comment Bug 681626 - Inconsistent insertion point at end of contentediable with trailing space.
: Inconsistent insertion point at end of contentediable with trailing space.
Status: NEW
: testcase
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: x86_64 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 751513
  Show dependency treegraph
 
Reported: 2011-08-24 06:04 PDT by Tom Hindle
Modified: 2012-06-06 19:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
InconsistentHandlingOfTrailingSpace.html (73 bytes, text/html)
2011-08-24 06:04 PDT, Tom Hindle
no flags Details
Patch the removes code that causes the bug. (684 bytes, patch)
2011-09-02 07:01 PDT, Tom Hindle
no flags Details | Diff | Splinter Review

Description Tom Hindle 2011-08-24 06:04:25 PDT
Created attachment 555380 [details]
InconsistentHandlingOfTrailingSpace.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.112 Safari/535.1

Steps to reproduce:

Load attached test case.
Which contains a contenteditable body with the following Inner Html:
"<div>hello </div>"

1. Click in space right of hello.
2. Press key 'k'


Actual results:

1. After click initially insertion point is displayed one space right of hello.
2. After pressing 'k' "hellok" is displayed.


Expected results:

bar ('|') means insertion point.

Either (like chrome 13) display "hello|" then after keypress display "hellok|" 

OR (like IE 9) display "hello |" then after keypress display "hello k|"

As a work around replacing trailing spaces with &nbsp; works consistently.
Comment 1 (mostly gone) XtC4UaLL [:xtc4uall] 2011-08-24 16:34:44 PDT
Confirmed against Nightly but I'd be surprised this not been filed already.
Comment 2 Tom Hindle 2011-09-02 05:55:57 PDT
nsWSRunObject::InsertText (in editor/libeditor/html/nsWSRunObject.cpp) deletes the Whitespace:


    else if (beforeRun->mType & eTrailingWS)
    {
      // need to delete the trailing ws that is before insertion point, because it 
      // would become significant after text inserted.
      res = DeleteChars(beforeRun->mStartNode, beforeRun->mStartOffset, *aInOutParent, *aInOutOffset,
                        eOutsideUserSelectAll);
      NS_ENSURE_SUCCESS(res, res);
    }

stacktrace:

#0  nsWSRunObject::InsertText (this=0xbf820a80, aStringToInsert=..., 
    aInOutParent=0xbf820b34, aInOutOffset=0xbf820b38, aDoc=0x972768fc)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsWSRunObject.cpp:334
#1  0xb5d3a47e in nsHTMLEditRules::WillInsertText (this=0x97587400, 
    aAction=2000, aSelection=0x973f8200, aCancel=0xbf820ce4, 
    aHandled=0xbf820ce8, inString=0xbf820da4, outString=0xbf820c08, 
    aMaxLength=-1)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:1480
#2  0xb5d44905 in WillDoAction (aHandled=0xbf820ce8, aCancel=0xbf820ce4, 
    aInfo=0xbf820c9c, aSelection=0x973f8200, this=0x97587400)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:618
#3  nsHTMLEditRules::WillDoAction (this=0x97587400, aSelection=0x973f8200, 
    aInfo=0xbf820c9c, aCancel=0xbf820ce4, aHandled=0xbf820ce8)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:544
#4  0xb5cb574a in InsertText (aStringToInsert=..., this=0x97585800)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/text/nsPlaintextEditor.cpp:820
#5  nsPlaintextEditor::InsertText (this=0x97585800, aStringToInsert=...)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/text/nsPlaintextEdi---Type <return> to continue, or q <return> to quit---
tor.cpp:788
#6  0xb5cb297a in nsPlaintextEditor::TypedText (this=0x97585800, aString=..., 
    aAction=0)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/text/nsPlaintextEditor.cpp:428
#7  0xb5d192dd in nsHTMLEditor::TypedText (this=0x97585800, aString=..., 
    aAction=0)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:1425
#8  0xb5d1d806 in HandleKeyPressEvent (aKeyEvent=0x97979c94, this=0x97585800)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:735
#9  nsHTMLEditor::HandleKeyPressEvent (this=0x97585800, aKeyEvent=0x97979c94)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:626
#10 0xb5cc8052 in KeyPress (aKeyEvent=0x97979c40, this=0x96fc3a20)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/base/nsEditorEventListener.cpp:357
#11 nsEditorEventListener::KeyPress (this=0x96fc3a20, aKeyEvent=0x97979c40)
    at /home/hindlet/src/hg/mozilla-central/editor/libeditor/base/nsEditorEventListener.cpp:325
#12 0xb5cc8877 in nsEditorEventListener::HandleEvent (this=0x96fc3a20, 
    aEvent=0x97979c40)
Comment 3 Tom Hindle 2011-09-02 07:01:51 PDT
Created attachment 557821 [details] [diff] [review]
Patch the removes code that causes the bug.

Added patch that removes the offending code.
Comment 4 Tom Hindle 2011-09-02 07:03:59 PDT
Rebuilding Firefox with attached patch, fixes bug.
Will now look for why this code was added.
Comment 5 Tom Hindle 2011-09-05 02:48:30 PDT
f=ehsan
Comment 6 :Ehsan Akhgari 2011-09-05 13:46:54 PDT
This is kind of tricky.  The whitespace at the end of the block is collapsed, so we should treat it as such.  I think that the desired behavior is to show "hello|" initially and then "hellok|" after pressing 'k', which should remove the trailing whitespace.  In other words, we want to keep the existing nsWSRunObject logic.

What we need to fix is the hittesting code returning the wrong offset for the selection when you click to the right of that paragraph.  nsIFrame::GetContentOffsetsFromPoint calls GetSelectionClosestFrame which returns the textframe with the frameEdge flag set.  Then, we call GetRangeForFrame which calls nsTextFrame::GetOffsets, which in turn calls nsTextFrame::GetContentEnd.  That function ignores whitespace collapsing, and just returns the length of the corresponding content textnode.  I think we should fix that, possibly by calling GetTrimmedOffsets instead of GetOffsets.  Roc, do you agree?
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-05 17:39:59 PDT
That would mean that to enter text after the trailing space, the user would have to click and then press right-arrow. And wouldn't we have the same sort of bug where the user uses right-arrow to get past the trailing space, then enters text?

Technically the trailing white-space is not collapsed (there is no whitespace for it to collapse with), but trimmed due to being at the end of the line. I think that when the line ends with a soft line break, it makes sense to treat trimmed trailing whitespace as not being there for editing purposes (especially caret movement), but when the line does not end with a soft line break, I think we should probably treat the trimmed trailing whitespace as present for editing.
Comment 8 :Ehsan Akhgari 2011-09-06 12:28:07 PDT
Right, I meant trimmed when I said collapsed.  But I don't agree with your suggestion.  We do not take this trailing space into account for layout purposes, right?  For example, it does not affect line breaking behavior.  It also doesn't show up in non-collapsed selections (although the space is copied to the clipboard for example, which is another bug I think).  In other words, the space is invisible, so I'm not sure why we should make it visible when editing.

What I propose is that for all editing intents and purposes, we should treat such trailing space characters as if they did not exist in the DOM, i.e., caret positioning and movement should be oblivious to their existence.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-06 19:11:15 PDT
If we do that, then
a) in the testcase in this bug, there will be no way for the user to insert content after the trailing space
b) if the user types some text followed by a space at the end of a contenteditable element, typing the space will not advance the caret
Comment 10 :Ehsan Akhgari 2011-09-07 09:04:45 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> If we do that, then
> a) in the testcase in this bug, there will be no way for the user to insert
> content after the trailing space

I think that's the desired behavior, because as far as the user can see, there is no trailing space to begin with (unless if they look at the DOM).

> b) if the user types some text followed by a space at the end of a
> contenteditable element, typing the space will not advance the caret

I think in that case we should insert a non-breaking space.
Comment 11 Tom Hindle 2011-09-07 09:36:28 PDT
@Ehsan, wouldn't the inserting of non-breaking spaces lead to a scenario like this:

Initial state:
"<div>hello </div>"

Click after hello:
"<div>hello| </div>"

Press space:
"<div>hello&nbsp;| </div>"

type world:
"<div>hello&nbsp;world| </div>"

Press space:
"<div>hello&nbsp;world&nbsp;| </div>"

etc. So whenever text is appended in a contenteditble block, It will not line break.

If I have understood this correctly then, from my user perspective that seems less than ideal, and I guess I would have process the content to remove &nbsp; not at end of lines.
Comment 12 :Ehsan Akhgari 2011-09-07 11:47:17 PDT
No, they can be fixed up in nsWSRunObject.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-07 19:30:30 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > If we do that, then
> > a) in the testcase in this bug, there will be no way for the user to insert
> > content after the trailing space
> 
> I think that's the desired behavior, because as far as the user can see,
> there is no trailing space to begin with (unless if they look at the DOM).

OK, but is that OK with Web authors, who certainly can see that there is a trailing space there? What does Aryeh think? This seems like something that should work the same across browsers; we don't want that space to be usable in some browsers but not others.

> > b) if the user types some text followed by a space at the end of a
> > contenteditable element, typing the space will not advance the caret
> 
> I think in that case we should insert a non-breaking space.

OK.
Comment 14 :Ehsan Akhgari 2011-09-09 16:51:15 PDT
CCing Aryeh...
Comment 15 Aryeh Gregor (:ayg) (away until October 25) 2011-09-12 13:13:26 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> OK, but is that OK with Web authors, who certainly can see that there is a
> trailing space there? What does Aryeh think? This seems like something that
> should work the same across browsers; we don't want that space to be usable
> in some browsers but not others.

I spent a whole bunch of effort on a solution that's more or less workable.  The basic idea is this:

* Whitespace that's not visible to the user (collapsed, trimmed, etc.) should never have any visible effect on editing.
* When the user inserts a space into an element that doesn't have white-space: pre or pre-wrap, convert it to an nbsp.
* When the user inserts or deletes any text (including nbsp) in an element that doesn't have white-space: pre or pre-wrap, canonicalize any adjacent space sequence so that it alternates between space and nbsp in a particular pattern that I picked.
* Hope authors will set white-space: pre-wrap to avoid the horrible mess created by the above.

Examples:

  foo[]       and type space -> foo&nbsp;[]
  foo&nbsp;[] and type space -> foo &nbsp;[]
  foo&nbsp;[] and type "a"   -> foo a[]

The upshot is that runs of user-inserted spaces get magically converted to some mix of spaces and nbsps to try getting something vaguely like what the user is expecting within the constraints of HTML.

The details are contained here:

http://aryeh.name/spec/editing/editing.html#canonical-space-sequences

The rationale is here, if you click the first "View comments" link:

http://aryeh.name/spec/editing/editing.html#the-inserttext-command

It's all very horrible, but I don't see any better solution given the constraints.


In this particular case, it looks like the spec isn't very useful.  It should really strip the invisible space before handling the text insertion.  Instead, it treats it as though it has meaning and converts it to a non-breaking space.  I filed a spec bug:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=14119

For now, I'd suggest Gecko strip the space before doing any insertions.  I intend to change the spec to do something like that when I get the time.
Comment 16 Aryeh Gregor (:ayg) (away until October 25) 2011-09-22 13:15:10 PDT
I fixed the spec bug, which included adding a pretty large number of tests:

https://dvcs.w3.org/hg/editing/rev/89fed05c54e0

You may want to take those.  Note that some of the expected results in the initial commit were wrong, and were changed in the next commit:

https://dvcs.w3.org/hg/editing/rev/e60077322f8a

(This way the diff of the actual code change clearly shows what it fixed.)

My basic solution was that before deleting or inserting text, we should get rid of any nearby collapsed whitespace.  That means removing spaces that begin or end a line, and collapsing runs of consecutive spaces into a single space.
Comment 17 Aryeh Gregor (:ayg) (away until October 25) 2011-09-22 14:34:40 PDT
I added some more tests:

https://dvcs.w3.org/hg/editing/rev/b94bc4cceb61

Currently the only tests that can run nicely in Gecko are the delete tests, since Gecko doesn't support the forwardDelete or insertText commands.  You can run the delete tests here:

http://dvcs.w3.org/hg/editing/raw-file/tip/conformancetest/splitruntest.html?delete

The relevant ones start at

  [["stylewithcss","false"],["delete",""]] "foo &nbsp;[]"

(search for that string).  There's lots of noise to wade through, though.  Also, the insertText tests might be more interesting.
Comment 18 :Ehsan Akhgari 2011-09-22 15:03:25 PDT
Thanks Aryeh for the spec fixes.  They look great!

Note You need to log in before you can comment on or make changes to this bug.