[webkit-dev] Rich Text Editing Questions, Refactoring of Position Classes

Roland Steiner rolandsteiner at google.com
Wed Apr 7 00:11:47 PDT 2010

Thanks for the comments! Please find my replies inline:

On Tue, Apr 6, 2010 at 2:51 PM, Maciej Stachowiak <mjs at apple.com> wrote:

> On Apr 1, 2010, at 10:43 PM, Roland Steiner wrote:
> .) When a selection that starts in a table and ends outside it is deleted,
> the current code drags the adjacent outside content into the table. To me
> this is counter-intuitive (text can be "dragged" in, but not between cells,
> and not back outside), and it's also contrary to the behavior of other
> editors (FireFox, TextEdit, Word, etc.). The behavior is, however, enshrined
> in various layout tests, so I wonder if there was/is a reason to implement
> it this way. As this behavior also complicates fixing other bugs I wanted to
> see whether there would be much opposition to changing it (i.e., to content
> outside of a table staying outside on a delete operation).
> Which layout tests? Do they reference bugs? We can study the bugs to see if
> they truly ask for the behavior being tested.

On Tue, Apr 6, 2010 at 3:39 PM, Justin Garcia <justin.garcia at apple.com>

> I think you're right except that if the table doesn't look like a table
> (like if it doesn't have any cell borders) its content can just look like
> any other set of paragraphs.  Content should be merged in those cases I
> think.  Off the top of my head I believe some Mail behavior depended on
> this.  You could check and see when those layout tests were checked in and
> bug titles from the corresponding ChangeLog entries might shed some light on
> this.

The following layout tests explicitly require outside content to be moved
into table cells:

    <rdar://problem/5032066> <Delete> should work between To Dos

editing/deleting/delete-block-table.html  (updated multiple times, the
change marked with * is the same as for the above layout test)
    <rdar://problem/4622763> Deleting from beginning of paragraph following
a table deletes rather than selects the table
    Setup for <rdar://problem/4344550> Misspellings aren't marked after undo
    <rdar://problem/4922367> WebView selectLine: followed by deleteBackward:
deletes TABLE element of following line
*   <rdar://problem/5032066> <Delete> should work between To Dos
    <rdar://problem/5107422> TOT REGRESSION: Delete key fails to delete
text, and cursor disappears in Mail.app
    REGRESSION (r19595): WebViewDidBeginEditingNotification not posted when
focusing with the mouse
    updated some test results that were affected by Hyatt's fix
for <rdar://problem/5208440> REGRESSION: Raw text needs to be pulled outside
of tables (13753)

    <rdar://problem/5206311> Whitespace can't be removed when editing text
pasted into from web page

A quick and dirty "fix" also affected the following layout tests:

    moving content into table cell doesn't seem to be the point of these
tests, just a side-effect

    mentions table cell, but point of the test doesn't seem to be
table-specific - moving content into table cell seems therefore a side

    current result image contradicts the expectation described in the test
header, fix would create intended result

    current result seems to contain extraneous <br>, fix would remove it

    unexpected image diff in editing region halo (not sure why)

    ASSERTs (placeholder <br> seems to get deleted/pruned)

Hard to comment on this idea from such a high level view. I don't understand
> how EditingPosition is meant to be different from VisiblePosition. Is
> EditingPosition just a VisiblePosition that's also a place where you can
> edit? I don't understand how DOMPosition is different in intent from the
> current Position. I'm not sure you want the low-level class to based on
> RangeBoundaryPoint, since the latter has the ability to adjust in response
> to DOM changes, which I am not totally sure we want in this case.

The basic idea would mainly be to combine PositionIterator and Position into
one class "EditingPosition" (or just "Position"), focusing on performance,
and to move renderer-specific code into VisualPosition (which could be
(re-)named "RenderedPosition" for clarity). Apart from an IMHO clearer code
separation this should also help improving the handling of :before/:after
content renderers, which currently is buggy.

DOMPosition/RangeBoundaryPoint would continue to be just the bridge to
JavaScript code and not used in (most) editing code.

Non-contiguous selections suck as UI, except for special cases like
> selecting tables by column. I don't think they are a good way to highlight
> find results. I don't think you want to end up with a non-contiguous
> selection highlighting all results when you do a find.

I don't understand: Safari and Chrome both do basically exactly that (albeit
in a somewhat souped-up fashion) - why is this capability good in the
browser, but not for user scripts?

Also, FWIW, I admit column selection was one of the usage scenarios I
thought of, but that could in theory also be simulated by selecting a (real
or virtual) <col> element, so I'm not sure it's really required for that.

On Wed, Apr 7, 2010 at 1:47 AM, Darin Adler <darin at apple.com> wrote:

> If we have an <img> element, there are three valid DOM positions adjacent
> to it:
>    A: [ parent-of-<img>, child-offset-of-<img> ]: before the image
>    B: [ <img>, 0 ]: inside the image
>    C: [ parent-of-<img>, child-offset-of-<img> + 1 ]: after the image
> WebKit’s HTML editing code also makes use of an invalid DOM position:
>    D: [ <img>, 1 ]: inside the image, but used to represent the position
> after the image
> Using D is never a good idea because it’s not a valid DOM position, so we
> can’t use it to construct a DOM range object. Having positions like D in our
> code at all leads to complexity and confusion.
> Using B to represent the position before the image is also unconventional;
> the position is clearly “inside the image element”, whatever that means.
> Given that, it seems that editing-related code that starts with a position
> such as B should quickly convert it to either A or C as appropriate.
> But computing A or C given a pointer to an image element is costly because
> it involves walking the parent's child nodes. In a large, flat document the
> cost is proportional to the size of the document. The same goes if we start
> with A or C and want to find the image element just before or just after the
> position.
> There are other issues when the document is being changed with positions
> extant. If we add a child element to the <img> element’s parent before the
> <img> element, both A and C end up pointing to a different place in the
> document, but B and D will still point where they did before. So if we
> convert editing code that currently uses a position like B to instead use a
> position like A, it’s easy to change behavior without realizing it.

PositionIterator addresses all of these concerns, AFAICT, which is why I
would propose to use it as basis for editing code, unless that's unfeasible

On Wed, Apr 7, 2010 at 3:53 AM, Justin Garcia <justin.garcia at apple.com>

> For example:
> <div><img style="display:block"><img style="display:block"></div>
> [img1, 1] and [img2, 0] are different visually but would both be normalized
> to the same position under the above proposal.

I think this is a great example and shows that normalizing [img, 0] to
[parent-of-img, index-of-img] can probably only happen once styles and
renderers are taken into account, which doesn't strictly contradict Darin's
point though (?).


- Roland
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100407/c739605e/attachment.html>

More information about the webkit-dev mailing list