681626 - Inconsistent insertion point at end of contentediable with trailing space.

Status: NEW

(Core :: DOM: Selection, defect)

Description

[Tom Hindle]

Attachment: 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]]

Confirmed against Nightly but I'd be surprised this not been filed already.

Comment 2

[Tom Hindle]

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]

Attachment: Patch the removes code that causes the bug.

Added patch that removes the offending code.

Comment 4

[Tom Hindle]

Rebuilding Firefox with attached patch, fixes bug.
Will now look for why this code was added.

Comment 5

[Tom Hindle]

f=ehsan

Comment 6

[(no longer active)]

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)]

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

[(no longer active)]

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)]

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

[(no longer active)]

(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]

@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

[(no longer active)]

No, they can be fixed up in nsWSRunObject.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[(no longer active)]

CCing Aryeh...

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(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 []
  foo [] and type space -> foo  []
  foo [] 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) (no longer with Mozilla)]

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) (no longer with Mozilla)]

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  []"

(search for that string).  There's lots of noise to wade through, though.  Also, the insertText tests might be more interesting.

Comment 18

[(no longer active)]

Thanks Aryeh for the spec fixes.  They look great!