[Webkit-unassigned] [Bug 61594] REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span

Mon Jun 6 12:33:59 PDT 2011

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

--- Comment #10 from Annie Sullivan <sullivan at chromium.org>  2011-06-06 12:33:59 PST ---
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6) 
> > > This logic is likely wrong as it doesn't take invisible nodes into account.
> > 
> > You're right that I didn't take invisible nodes into account at all when writing this code. By invisible, do you mean display:none or something else? Is there a case I should add as a layout test?
> 
> Yes. display: none and text nodes with collapsible whitespace, etc... In general, we need to be consistent in how we deal with these invisible nodes and the best way to do is to rely on VisiblePosition.

I wanted to add a layout test where this doesn't work, so I could understand the problem better before trying to fix it. So I experimented with a few cases, and I wasn't able to find one where I was sure my function splits in the wrong place. Using "|" for cursor, I tried:

<span>Line1|    <br>Line2</span>
(extra whitespace after cursor removed when pressing enter)

<span>Line1|<span style="display:none">Invisible</span><br>Line2</span>
(invisible span moved to next line)

I must be misunderstanding the problem--do you have a more specific example of a case where this code is likely to break, so I can add a layout test for it?

When I do change the code to use VisiblePosition, is the correct approach to call Position::next instead of Node::nextSibling()?

> > > It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.
> > 
> > At the same time, complicating the arguments and adding one-off functionality to a simple, well-understood function used in many places seems quite dangerous, too. When I modify the function to take a position, I'll still need to answer this question about whether the position should be split, and make sure I don't introduce bugs with invisible nodes or anything else. Am I missing a less complicated way to fix this bug?
> 
> By splitting the position, do you mean splitting a text node which is the anchor node of a Position (where AnchorType is PositionIsOffsetInAnchor)?  We should be doing that iff we can come up with a test case where that is necessary.  As far as I know, we deal with that in callers.  But maybe it's better to encapsulate all of that in splitTreeToNode instead of imposing implicit pre-condition like that.

Yes, by splitting the position, I mean if the position is in the middle of a text node, it seems logical that splitTreeToPosition() would split the text node so that the tree is split exactly at the given position. Right now, all the callers do the splitting themselves, since the function used to only split to node boundaries. I agree that it's not a great idea to add code if no one uses it yet. But I think that since the new function will take a position, it will be confusing to callers who aren't familiar with the history of the editing code why callers are expected to do any actual splitting of text nodes at the position. Not sure how this type of problem is usually handled--a comment above the function?

Also, I'm still not clear on what the declaration of the new function should be:
Should it be renamed to splitTreeToPosition?
Should it take Position or a VisiblePosition?
Its arguments should be a start position and and end node?

> > Also, it took me a bit of extra time to understand what splitTreeToNode() is supposed to do because there wasn't a unit testing framework that allowed me to pass in arbitrary nodes/dom trees and look at the output. Do you think it would be worth trying to add common editing methods like this to window.internals so that we can test them independently? (Or maybe I'm missing some existing way to do this?)
> 
> I'm not so sure because that'll require us adding a custom editing command for splitTreeToNode since you can't call splitTreeToNode without instantiating a CompositeEditCommand.

My understanding is that CompositeEditingCommand exists to allow shared code between various editing commands. I know in some codebases, making a UnitTestEditingCommand that derives from CompositeEditingCommand that we could use to write unit tests for all those functions would be a way to do this. Not sure about WebKit.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.