Thanks for the comments! Please find my replies inline: On Tue, Apr 6, 2010 at 2:51 PM, Maciej Stachowiak <m...@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.gar...@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 <da...@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.gar...@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
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev