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

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>
 wrote:

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

editing/deleting/5032066.html
    <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
delete
    <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)

editing/deleting/5206311-2.html
    <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:

editing/deleting/5026848-2.html
editing/deleting/5026848-3.html
    moving content into table cell doesn't seem to be the point of these
tests, just a side-effect

editing/deleting/5115601.html
    mentions table cell, but point of the test doesn't seem to be
table-specific - moving content into table cell seems therefore a side
effect

editing/unsupported-content/table-delete-001.html
    current result image contradicts the expectation described in the test
header, fix would create intended result

editing/unsupported-content/table-delete-001.html
    current result seems to contain extraneous <br>, fix would remove it

editing/inserting/12882.html
    unexpected image diff in editing region halo (not sure why)

editing/execCommand/5432254-2.html
    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>
 wrote:

>
> 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 (?).

Cheers,

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