Deleting contenteditable text to the left of a non-contenteditable span inserts unnecessary placeholder <br/>

[Benjamin (Ben) Kalman]

Deleting contenteditable text to the left of a non-contenteditable span inserts unnecessary placeholder <br/>

[Benjamin (Ben) Kalman]

Created attachment 72367 [details]
Patch

[Benjamin (Ben) Kalman]

Created attachment 72368 [details]
Simple repro

[Benjamin (Ben) Kalman]

Repro attached -- highlight "hello " (i.e. including space) and delete.

[Benjamin (Ben) Kalman]

Comment on attachment 72367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72367&action=review

> WebCore/WebCore.exp.in:420
> +__ZN7WebCore16isEndOfParagraphERKNS_15VisiblePositionENS_8Position27EditingBoundaryCrossingRuleE

fyi I tried deleting these but linking fails with

Undefined symbols:
  "__ZN7WebCore16isEndOfParagraphERKNS_15VisiblePositionENS_8Position27EditingBoundaryCrossingRuleE", referenced from:
      -[WebFrame(WebPrivate) _smartInsertForString:replacingRange:beforeString:afterString:] in WebFrame.o
  "__ZN7WebCore18isStartOfParagraphERKNS_15VisiblePositionENS_8Position27EditingBoundaryCrossingRuleE", referenced from:
      -[WebFrame(WebPrivate) _smartInsertForString:replacingRange:beforeString:afterString:] in WebFrame.o

[Benjamin (Ben) Kalman]

Created attachment 72371 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 72371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72371&action=review

LGTM.

> LayoutTests/editing/deleting/5390681-expected.txt:-2
>  This tests for a bug where expansion for smart delete would not consider editable boundaries.  Only 'foo' should be deleted.  You should see ' bar'.  There is a bug: while the non-editable space isn't deleted, deletion inserts a placeholder when it shouldn't.
> -| <br>

You need to update the comment here.

[Benjamin (Ben) Kalman]

Created attachment 72383 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 72383 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 72383 [details]
Patch

Clearing flags on attachment: 72383

Committed r70932: <http://trac.webkit.org/changeset/70932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Normally to share an enum like this we’d move the enum into a header instead of including "Position.h" in a header like "visible_units.h".

[Benjamin (Ben) Kalman]

Ok, happy to do that.  What is the standard practice here?

[Darin Adler]

(In reply to comment #12)
> Ok, happy to do that.  What is the standard practice here?

Create the new header, add it to the build systems, put the enum in there, add includes to the appropriate source files.

[Benjamin (Ben) Kalman]

(In reply to comment #13)
> (In reply to comment #12)
> > Ok, happy to do that.  What is the standard practice here?
> 
> Create the new header, add it to the build systems, put the enum in there, add includes to the appropriate source files.

https://bugs.webkit.org/show_bug.cgi?id=49630

[Philippe Normand]

editing/pasteboard/paste-text-events.html
editing/pasteboard/copy-backslash-with-euc.html

Started to fail on GTK since this patch landed it seems.

[Philippe Normand]

(In reply to comment #15)
> editing/pasteboard/paste-text-events.html
> editing/pasteboard/copy-backslash-with-euc.html
> 
> Started to fail on GTK since this patch landed it seems.

Heh. Nevermind!