Bug 23793 - WebKit needs separate VisibleSelection and Selection data storage
Summary: WebKit needs separate VisibleSelection and Selection data storage
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on: 23852
Blocks: 6350 17697
  Show dependency treegraph
Reported: 2009-02-06 11:39 PST by Eric Seidel (no email)
Modified: 2011-08-19 12:32 PDT (History)
7 users (show)

See Also:

modified test case from chromium bug (3.60 KB, text/html)
2009-02-06 11:41 PST, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-02-06 11:39:32 PST
WebKit changes the Selection on setting with addRange, FF does not

This is sorta a dupe of bug 6350 and bug 17697, certainly fixing this more general bug would fix both of those.

Basically Selection::validate() is flawed, at least from the DOM's perspective.  JS should be able to set the selection to anything and get it back w/o having it be mutated in the process.  The Editing code wants to be able to get at the "validated" selection.  We may just want to store things double, not sure yet.
Comment 1 Eric Seidel (no email) 2009-02-06 11:40:26 PST
This bug was prompted by http://code.google.com/p/chromium/issues/detail?id=2874, I'm about to attach a modified test case from chromium's 2874
Comment 2 Eric Seidel (no email) 2009-02-06 11:41:21 PST
Created attachment 27404 [details]
modified test case from chromium bug
Comment 3 Eric Seidel (no email) 2009-02-06 12:01:24 PST
I think the "validation" invariant which Selection currently tries to hold, is a wrong one.  In at least I don't think Selection should assume that both of it's endpoints are valid visible positions.  Maybe we need a separate VisibleSelection class which holds that invariant, similar to how we have VisiblePosition and Position split classes.
Comment 4 Eric Seidel (no email) 2009-02-06 12:07:00 PST
In the above proposal, Selection would just be an array of ranges.  VisibleSelection would probably still support only one Range (like it does today), slowly editing commands could be transitioned off of VisibleSelection and onto Selection?  Maybe adding multiple-range selection support in the process?
Comment 5 Justin Garcia 2009-02-06 12:24:29 PST
(In reply to comment #3)
> Maybe we need a separate VisibleSelection class which holds that invariant, 
> similar to how we have VisiblePosition and Position split classes.

Yea, that seems like the way to go.
Comment 6 Eric Seidel (no email) 2009-02-06 13:27:36 PST
Ok, so SelectionController will need a VisibleSelection member (the current m_selection), and a new Selection member.

What the user sees and interacts with is the VisibleSelection.  What JavaScript and (eventually) the editing commands interact with will be Selection (a Vector of Range objects).

VisibleSelection and Selection will both be copyable, datastorage types.  Editing, and any other section of the code which wants to keep around multiple copies can do so.  SelectionController maintain the official copy. :)

In order to handle keeping the stored Selection and VisibleSelection in sync, SelectionController will need to change to return a copy of a VisibleSelection via visibleSelection() instead of a reference like it does now.  There will be a new setVisibleSelection method which can take a VisibleSelection and knows to set m_selection from VisibleSelection::toSelection() or similar.

Likewise there will need to be a setSelection which takes a Selection object and knows to create a VisibleSelection from the Selection and store it in m_visibleSelection.

Initially all of our code will use VisibleSelection.  Over time, we will move more and more code to use Selection instead.

To add multi-range selection (which we'll eventually need for table editing), Selection will just store a Vector of Range objects.  VisibleSelection can hold whatever data it needs to represent the selection, but it will probably be a Vector of VisiblePosition pairs (or maybe VisibleRanges?).

Thoughts on this proposal are most welcome!