Bug 4582

Summary:

Editing delegates need more selection state

Product:

WebKit

Reporter:

Duncan Wilcox <duncan>

Component:

HTML Editing

Assignee:

Duncan Wilcox <duncan>

Status:

RESOLVED LATER

Severity:

Enhancement

CC:

ahmad.saleem792, ap, darin, ian, rniwa

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
patch	darin: review-
patch	darin: review-
patch	none
patch	duncan: review-
patch	darin: review-
rename patch	none

Duncan Wilcox

Reported 2005-08-22 03:54:56 PDT

khtml::Selection can represent both selection range and selection direction, through m_start/m_end and m_base/m_extent positions. DOMRange instead only represents a range, and not a direction. This is an issue when during a text selection done with a mouse drag, in response to a webView:shouldChangeSelectedDOMRange:toDOMRange:affinity:stillSelecting:, a delegate programmatically sets a new selection with -[WebView setSelectedDOMRange:affinity:]. If the mouse selection is backwards, the selection direction isn't preserved in its DOMRange incarnation, and the mouse selection basically doesn't work. This is easy enough to test, create a cocoa app in xcode, replace main.m with the following code and add the WebKit.framework to the project: ---------- #import <Cocoa/Cocoa.h> #import <WebKit/WebKit.h> @interface Test : NSObject @end @implementation Test - (BOOL)webView:(WebView *)webView shouldChangeSelectedDOMRange:(DOMRange *)currentRange toDOMRange:(DOMRange *)proposedRange affinity:(NSSelectionAffinity)selectionAffinity stillSelecting: (BOOL)flag { NSLog(@"shouldChangeSelectedDOMRange: %ld-%ld", [proposedRange startOffset], [proposedRange endOffset]); [webView setSelectedDOMRange:proposedRange affinity:selectionAffinity]; return NO; } @end int main(int argc, char *argv[]) { NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; Test *t = [[Test alloc] init]; [NSApplication sharedApplication]; NSRect contentRect = NSMakeRect(200, 180, 300, 300); NSRect wvRect = NSMakeRect(0, 0, 300, 300); NSWindow *w = [[NSWindow alloc] initWithContentRect:contentRect styleMask:NSMiniaturizableWindowMask | NSClosableWindowMask | NSTitledWindowMask backing:NSBackingStoreBuffered defer:NO]; WebView *wv = [[WebView alloc] initWithFrame:wvRect]; [[w contentView] addSubview:wv]; [wv setEditable:YES]; [wv setEditingDelegate:t]; [[wv mainFrame] loadHTMLString:@"<html><body><p>select me backwards! select me backwards! select me backwards!</p></body></html>" baseURL:nil]; [w makeKeyAndOrderFront:nil]; [NSApp run]; return 0; } ---------- With this app select text backwards and you'll see the selection sort of vanishing while you drag. Also, with a word/line/paragraph granularity, m_start/m_end are expanded to the actual selection, while m_base/m_extent represent what the mouse actually dragged over. Not preserving this information through the delegate has other selection side effects. The attached patch adds a WebSelection class that carries over all needed information, delegates methods are also replaced with new ones (the old ones are called for compatibility).

Attachments
patch (44.26 KB, patch) 2005-08-22 04:00 PDT, Duncan Wilcox	darin: review-	Details Formatted Diff Diff
patch (63.83 KB, patch) 2005-08-24 13:54 PDT, Duncan Wilcox	darin: review-	Details Formatted Diff Diff
patch (90.91 KB, patch) 2005-08-28 16:18 PDT, Duncan Wilcox	no flags	Details Formatted Diff Diff
patch (92.60 KB, patch) 2005-08-28 16:53 PDT, Duncan Wilcox	duncan: review-	Details Formatted Diff Diff
patch (88.02 KB, patch) 2005-09-04 17:06 PDT, Duncan Wilcox	darin: review-	Details Formatted Diff Diff
rename patch (149.00 KB, patch) 2005-09-12 14:52 PDT, Duncan Wilcox	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Duncan Wilcox

Comment 1 2005-08-22 04:00:29 PDT

Created attachment 3513 [details] patch At this time I'm submitting this patch for review to solicit comments, things to look into: - a few new APIs, do they look ok? - I'm not sure of the location of files (should WebSelection.* go in WebCore/kwq/?) - the other editing delegate callbacks might use WebSelection - Darin mentioned that classes should be a thin Obj-C wrapper over a C++ class, I suppose I can refactor that, but where would the C++ class go in the tree? - would the C++ class contain a khtml::Selection? This would perhaps let us improve the WebSelection API by making expandedRange read-only, update automatically when changing baseRange and granularity (via the underlying khtml::Selection)

Darin Adler

Comment 2 2005-08-22 08:17:25 PDT

Comment on attachment 3513 [details] patch Looks like a good direction, and pretty good patch. Here is a list of comments based on my read-through of the patch: 1) Since "Web" is the prefix, analogous to "NS", the functions to create WebSelection can just be called, e.g., "selectionWithBaseRange" rather than "webSelectionWIthBaseRange". 2) I'm not sure comfortable with the "expanded range" terminology -- "expand using granularity" was an internal name until this point, and not an API concept. How is this notion handled in NSTextView? Is there a better name for this? Can this just be called the "range" without an adjective? Or will that be even more confusing? 3) webSelectionWithBaseRange needs to autorelease its result. 4) The setBaseRange: method should use the "retain before release" idiom rather that doing the "!=" comparison. Same for the other setter. 5) Please don't include commented-out printf statements. 6) I think the direction() method of khtml::Selection should be a boolean with a descriptive name rather than having a special meaning for 0 vs. 1. Given the name direction() it's not clear, for example, whether it would be 0 vs. 1 or -1 vs. 1 or something else. 7) The import of "WebViewPrivate.h" should not be right up against the declaration that follows it (needs a blank line). 8) The WebEditingDelegatePendingPublic should be in a WebEditingDelegateInternal.h header. The current patch has it appearing in two places. 9) The big ? : expression in shouldChangeSelectionTo would be easier to read if it was written as an if statement. Also, I'd like to see the editingDelegate in a local variable. Finally, if we're going to be doing one respondsToSelector call, I suggest we do two and don't bother with the "forwarder". 10) I don't like the idea that the WebSelection has a separate expanded range and granularity. When it's created in any way expect when making it from a khtml::Selection you can end up with an expanded range that is inconsistent with the claimed granularity, such as in the calls that use unionDOMRange. Is there something we can do to eliminate this? Or can you prove to me that it's not a problem? 11) The "shouldChangeSelectedRange" code should be shared between the bridge and WebView. In general, it's good to have the bridge call over to WebView methods and remove bridge code. 12) This comment: +// These replace setSelectedDOMRange:affinity: and selectedDOMRange, in interface WebView (WebViewEditing) is slightly misleading, since the old calls will continue to work, selectionDOMRange (at least) is still useful, and the calls also replace the ones that get/set affinity and granularity. This looks to me like it's going to be a great change.

Darin Adler

Comment 3 2005-08-22 08:56:55 PDT

Duncan, I didn't see your questions in the bug, so let me answer those now: > a few new APIs, do they look ok? Seem fine to me. > I'm not sure of the location of files (should WebSelection.* go in WebCore/kwq/?) WebSelection.* should be in WebView.subproj and should be in WebKit, not WebCore. But this does beg the question of how we can get selection information across the bridge, since we can't use WebSelection. Maybe WebSelection needs to be handled like the DOM*.h headers; in WebCore, but then copied by WebKit. The other option is to start using C++ classes in the bridge API and add some C++ code on the WebKit side. This is more forward-looking since we want to move code from WebKit to WebCore over time and have minimal Objective-C in WebCore. This will require a bit of thought. > the other editing delegate callbacks might use WebSelection Yes, I think we should do new versions of all APIs involving selections. > Darin mentioned that classes should be a thin Obj-C wrapper over a C++ class, I suppose I can refactor that, but where would the C++ class go in the tree? The C++ class would go somewhere in KHTML. What I don't know is how we'd handle this when bridging. Usually we have to figure out some Objective-C way of getting something over the bridge, since we don't like to use C++ for the interface between WebCore and WebKit. I think this is something we're going to have to change, and the issue is not specific to this patch. > Would the C++ class contain a khtml::Selection? Probably not. > This would perhaps let us improve the WebSelection API by making expandedRange read-only, update automatically when changing baseRange and granularity (via the underlying khtml::Selection) I can see how this is desirable, but khtml::Selection comes with additional unwanted stuff. I think we'd need to do some refactoring if we wanted to go in this direction. Sorry that some of these is tentative; there are some tricky, albeit minor, design issues here.

Duncan Wilcox

Comment 4 2005-08-23 00:49:13 PDT

I think I have addressed most issues in my tree. I think the following is the best way to address the API consistency issues you raise: - extract the code that expands granularity in khtml::Selection::validate, into WebCore/khtml/editing/ granularity.cpp|h - create a C++ class that would mirror WebSelection in WebCore/khtml/editing/webselection.cpp|h - have -[WebSelection setBaseRange] and -[WebSelection setGranularity] call into the granularity expansion code, so that the now read-only 'range' property (previously expandedRange) correctly reflects selection and granularity The only problem that's left is bridging, so I'm stuck with your comment: > we don't like to use C++ for the interface between WebCore > and WebKit. I think this is something we're going to have to > change, and the issue is not specific to this patch. Is avoiding C++ in WebKit a build/link/deployment/runtime issue? Should I build some bridging stuff in WebCore/kwq in the mean time?

Darin Adler

Comment 5 2005-08-23 09:52:59 PDT

I like the suggested plan re: granularity. One flaw in this is that as the document changes, an old WebSelection object may become invalid. For example, paragraph boundaries could change. I don't know how we should define this. We could make it so that it was illegal to use a WebSelection object after the document changed, but that would probably be too heavy handed. Or we could try to define exactly what happens to an outstanding WebSelection object when the document changes. Avoiding C++ in WebKit was primarily an "SPI stability" issue. We used to use a lot of C++ in WebKit, and we had problems with Xcode's header dependency support. It was a great improvement when the interface became Objective-C-only. But that was in the distant past so it may no longer apply. The new rule going forward should probably be something like "WebKit gets code that's about bridging to Objective-C WebKit API, and WebCore gets all the rest of the code".

Duncan Wilcox

Comment 6 2005-08-24 13:54:09 PDT

Created attachment 3553 [details] patch Regarding WebSelection becoming invalid, the same happens for a DOMRange, with the difference that you might get a DOM exception with DOMRange only when using against a WebView, whereas you might get a DOM exception with a WebSelection simply by calling its range method (that combines baseRange, granularity with the current document on the fly). Is this reasonable? Regarding C++, I could either call C++ from WebKit or export WebSelection.h from WebCore like DOM headers. I have chosen the latter because it seems simpler until WebCore/kwq gets somehow refactored into WebKit, but I can change it around if necessary. I have held off extracting the khtml::Selection::validate code because there doesn't seem to be much other code in khtml::Selection, so I'm concerned that I might be already using what you consider unwanted stuff, and khtml::Selection would become sort of an empty shell. So I'm submitting this for some more feedback, I still need to create new WebEditingDelegate methods that use WebSelection and I want to refactor [WebCoreBridge setSelection:closeTyping:] that's duplicating work done in SelectionData::range.

Darin Adler

Comment 7 2005-08-26 19:46:25 PDT

Comment on attachment 3553 [details] patch Looking good. Here's another round of comments: A) Code inside the khtml namespace needn't specify khtml:: when using other things in the same namespace, so remove all those khtml:: prefixes in the .cpp files and the .h files. B) The baseRange parameter should be: RangeImpl *baseRange We use SharedPtr for member variables and return values, but not for parameters. C) It's more efficient to initialize variables with the right value than to initialize them to false and then set them, so I'd prefer that the constructors not use the set functions, and instead initialize. D) When checking a SharedPtr for null, it's better to use isNull() or notNull() rather than get(). E) The formatting should not be: if(! xxx instead it should be: if (!xxx F) Should do "using DOM::" for identifiers you use at the top of the .cpp file for any type you are using, and not use the DOM:: prefix at each use later on. This does not apply to .h files. G) In selectionWithBaseRange: + id sel = [[WebSelection alloc] init]; that variable should be of type WebSelection *, not id. Same for the other creation function. Also, there's no need to check for nil because: 1) init won't ever return nil in this case, since there's no error case, and 2) dispatching against nil safely does nothing for all the methods you're calling. H) The selectionWithXXX methods should be covers for initWithXXX methods that do the actual work. I) Besides the dealloc method, you also need a finalize method that does the same thing, since WebKit works in both GC and non-GC modes. This is only needed when there are non-ObjC deallocations, such as in the WebSelectionPrivate class; the finalize methods don't need to make any calls to "release". J) I slightly prefer to leave out the parentheses on lines like this one: + data = new khtml::SelectionData(); K) For things internal to WebKit, we should use the term "Internal" rather than "Private". "Private" basically means "private to the OS, not for use by applications". Thus the WebSelectionPrivate.h header should be named WebSelectionInternal. Also, the WebSelectionPrivate class, whatever its name, should not be in the header because there's no use of it outside the implementation file. L) In Selection::baseRange, the code gets the extent and base and then checks == with m_end. I think instead it should just get m_start and m_end, no? M) I'm a little disappointed that this patch adds copied and pasted code. I'd like to see this changed around so that the code is shared with the place it was copied from. N) m_baseIsStart ? true : false is pretty strange. Should just be "m_baseIsStart". O) The use of "deprecate" is incorrect in this comment: +// The following deprecate setSelectedDOMRange:affinity: and complement selectedDOMRange and +// selectionAffinity, in interface WebView (WebViewEditing) It's true that the new methods supplant the old ones, but it's incorrect grammar to say that a method "deprecates" another method. You should reword the comment (and I'm not really suggesting you use the word "supplant" :-). P) Perhaps we should add a selectionDirection accessor to complement selectionAffinity. It seems like the odd man out at this point.

Duncan Wilcox

Comment 8 2005-08-28 16:18:53 PDT

Created attachment 3628 [details] patch The WebSelectionInternal class (previously WebSelectionPrivate) is in the header file because it is used in a few places to initialize a WebSelection from a SelectionData (KWQKHTMLPart.mm, WebCoreBridge.mm). The code in baseRange needs m_base/m_extent and not m_start/m_end, as the latter are granularity-expanded (and toRange already returns those). m_base/m_extent however might be backwards and need to be flipped. The check against m_end was in fact incorrect, I replaced it with a check with isForwardDirection(). In addition to selectionDirection I added selectionGranularity, which was also missing. I have also added the other editing delegate WebSelection-based methods and refactored some code as mentioned. Hopefully this patch addresses all the outstanding issues.

Duncan Wilcox

Comment 9 2005-08-28 16:53:02 PDT

Created attachment 3629 [details] patch Sorry, I hadn't correctly update the webcore xcode project after an update conflict.

Duncan Wilcox

Comment 10 2005-08-29 02:29:09 PDT

Comment on attachment 3629 [details] patch Found a few problems I need to address.

Duncan Wilcox

Comment 11 2005-09-04 17:06:06 PDT

Created attachment 3766 [details] patch This patch addresses all the outstanding issues.

Darin Adler

Comment 12 2005-09-05 09:32:24 PDT

Comment on attachment 3766 [details] patch Looks great; this is going to be an excellent improvement to the API. I have a lot of comments. --- Semantics A) I'm not sure I understand the rule about m_extended and the concept of how SelectionData handles the extending. Why does a newly constructed SelectionData have m_extended set to true? Is the base range passed in actually an "already-extended" range? Is the fact that the extended range is computed at the time you get the range a feature of the class, and if so, why do we ever store the extended range, since that gives a different semantic? Can we do away with m_extended entirely? B) Reading the SelectionData code closely, it looks like the class will behave differently if you pass in a "detached" range than if you pass in NULL for the range. If you pass in a detached range, you'll actually get assertion failures later in toSelection(). I think it's worth a bit more thought to make this edge case more well defined. Are detached ranges allowed as parameters or not? If not, lets assert when they are passed in. If so, then what's the behavior when we have one. C) The SelectionData constructors that take a RangeImpl * adopt that range. That's different from what setBaseRange does. I'm not sure the difference is a good idea. Also, because of this subtlety, if we aren't going to change it, I suggest marking the SelectionData constructor that takes just a RangeImpl * explicit so it can't be used implicitly for type conversion. D) Since SelectionData has to implement its own copy constructor, then it either needs an implementation of the assignment operator, or to declare it and not implement it. To be specific, the assignment operator would result in two SelectionData objects that both hold a pointer to the same base range, while the copy constructor would copy the base range. E) I don't understand how the "getDocument != NULL" assertion inside SelectionData::toSelection is guaranteed. It seems to me that we could easily run into a situation where a selection object outlasts the document it was made from, and thus we need defined behavior. We should only use ASSERT for things we know will be true for some reason, for example because it's a caller responsibility or an impossible situation prevented by the structure of the code. Otherwise we have to consider the exceptional case and decide what our behavior will be. And I think this is a case like that. F) There's a difference between a function that returns a range that the caller owns and can modify and a function that returns a range that you need to copy if you want to modify it. We need to document which of these functions has which semantic. This applies to both SelectionData and WebSelection because of the nature of the DOMRange class. --- Naming G) It seems that khtml::SelectionData should really be named khtml::Selection for the same reason that WebSelection has its name; we should look for another name for the current khtml::Selection class and at least propose it at the time of this patch. Actually doing the renaming could be postponed, but having a planned new name for the classes might influence the names of some functions H) As long as we're adding new source files, we should follow the new standard of naming files after the classes in them in mixed case. I know the old code doesn't match that, but we're planning on renaming the old files soonish. So it should be "SelectionData.h" and "SelectionData.cpp". I) Do we really need the word "Text" in WebTextSelectionDirection and WebTextSelectionGranularity? I think that these concepts apply equally well for non-text selections in a WebView so we can probably just call them WebSelectionDirection and WebSelectionGranularity. --- Factoring and organization J) Leaving the EDirection enum inside the Selection class creates a situation where the SelectionData's header file has to include Selection's header file. That's really unfortunate, since Selection is a more-complex class that has to include many other headers. I think we'd be way better off if we made a global ESelectionDirection and put it in its own header file as with affinity. If this is done, then the SelectionData header can get away with just forward-declaring the Selection class. In fact, the only reason it needs to declare it at all is the "toSelection()" function, and I don't think that needs to be a member function since it can be implemented entirely with SelectionData's public interface. K) I don't think the setSelectionInPart function makes logical sense as a member function of SelectionData. It makes sense to have the function, but perhaps it should be either a free-standing function or a member of another class. Same comment for WebSelection. In this case, we should let the method on the bridge be the one that pulls the SelectionData out of the WebSelection and calls the appropriate C++ and omit the -[WebSelection _setSelectionInPart:closeTyping:] method. --- Other code details L) Since SelectionData is in the khtml namespace, it shouldn't use the khtml prefix in the declaration of khtml::Selection for the return type of toSelection(). M) I don't see the point in having a getter for SelectionData::extended() if it's private. The class should just use the m_extended variable directly. N) I don't understand the purpose of the line of code that says: class DOM::RangeImpl; in the selectiondata.h header. I believe that this syntax does not work to forward-delare a class. So either we're including a header that forward-declares the class already and don't need this line, or we need to use the syntax that does work for forward declaration which is this: namespace DOM { class RangeImpl; } Same comment about "class khtml::Selection" inside selectiondata.cpp. O) The include guard macro in selectiondata.h has the name WEBSELECTION_H in it; needs to be fixed. P) There's no need for selectiondata.h to include "text_granularity.h" if it's also including "selection.h". In general we'd like the list of includes to be minimal. Q) This line of code: m_baseRange = baseRange ? SharedPtr<RangeImpl>(baseRange->cloneRange(exceptioncode)) : SharedPtr<RangeImpl>(); can be written as: m_baseRange = baseRange ? baseRange->cloneRange(exceptioncode) : NULL; and I think it reads a little better. Don't you think? Similarly, this: if (m_baseRange.isNull()) return SharedPtr<RangeImpl>(); can be written as this: if (!m_baseRange) return NULL; In general, the "isNull()" function is deprecated these days. Although if you think it makes the code read better, then it's fine to keep using it. R) We usually leave out the space in lines like this one: if (! rangeCollapsed) and write them like this: if (!rangeCollapsed) S) The WebSelection class needs a "designated initializer". See the guidelines at <http://developer.apple.com/documentation/Cocoa/Conceptual/CocoaObjects/Article s/ObjectCreation.html>. I'd suggest that all the other initializers call initWithBaseRange:affinity:direction:granularity: and that one call [super init] rather than [self init]. T) -[WebSelectionInternal init] should probably call [super init]; U) -[WebSelectionInternal finalize] needs to call [super finalize]. WebSelectionInternal.h should forward-declare khtml::SelectionData rather than including the header. V) This: SharedPtr<RangeImpl> i = makeRange(m_start, m_end); return i; reads better as: return makeRange(m_start, m_end); This: SelectionData sel(rangeOfContents(root).get()); return part()->shouldEndEditing(sel); reads better as: return part()->shouldEndEditing(rangeOfContents(root).get()); This: khtml::SelectionData sd = _part->selectionData(); return [WebSelection _selectionWithSelectionData:&sd]; reads better as: return [WebSelection _selectionWithSelectionData:&_part->selectionData()]; W) Since the WebSelection private API is all Objective-C++, maybe the SelectionData parameters should be const & instead of const * typed to help make it clearer that it's copying the data and not keeping a pointer. X) We don't need to use WebDefaultEditingDelegate if we're doing explicit respondsToSelector: checks. Instead it's fine to just have the callers return YES. The reason WebDefaultEditingDelegate exists is for the _editingDelegateForwarder technique, and if we aren't using that technique with a particular method, then it's best to leave it out of the default delegate. I think we will retire the forwarder technique soon; it doesn't save much code or read particularly nicely and it's considerably slower than an explicit check for the common case of a delegate that does not implement a particular method.

Duncan Wilcox

Comment 13 2005-09-05 10:14:11 PDT

A) There are two different patterns of use of selections: programmatic for example via javascript, and with user interaction. Remember that the need to carry over state was needed for mouse-initiated selections being approved by the delegate. Now, the mouse handling needs the non-extended selection to work, while the javascript only seems to expose extended selection. I don't have a very deep understanding of visible positions and <br> handling related bugs, but re-extending a selection doesn't work well, even if the granularity is forced to character. In retrospect I suppose the m_extended flag should be explicitly set by the user, though the current implementation matches the usage in the codebase (bad excuse, I realize). B) I think the behaviour should match the current implementation, so detached ranges shouldn't be allowed I) Another bad excuse, WebSelectionDirection and WebSelectionGranularity are already taken, and have slightly different definitions, those need renaming too I suppose. I'll work on the rest.

Duncan Wilcox

Comment 14 2005-09-12 14:52:19 PDT

Created attachment 3881 [details] rename patch First step: rename Selection to SelectionController, WebSelectionDirection to WebBridgeSelectionDirection and WebSelectionGranularity to WebBridgeSelectionGranularity. The patch file doesn't mention that WebCore/khtml/editing/selection.* and WebCore/ForwardingHeaders/editing/selection.h should be removed.

Darin Adler

Comment 15 2005-09-23 08:56:13 PDT

Comment on attachment 3881 [details] rename patch To avoid losing history, we probably want to copy the file on the server side before landing this patch. Otherwise, looks good. r=me

Darin Adler

Comment 16 2005-09-23 18:16:14 PDT

Comment on attachment 3881 [details] rename patch Removing the review+ flag from this patch now that it's landed so it doesn't show up as "needed but not yet landed".

David Harrison

Comment 17 2005-10-30 07:38:40 PST

Please do re-think how extended vs non-extended is specified and used. The current interface relies too heavily on the SelectionController creators knowing implicit differences between the constructors that accept baseRange, and the constructor and setBaseRange that do not. Some more explicit control is appropriate. How about removing m_extended and renaming things so that callers can ask for the range() or the extendedRange()? i.e. both are available, and the caller just needs to know which kind it wants. Or is that knowledge only available to the code that constructs or sets up the SelectionController? current name new name ----------- ---------- m_baseRange m_range -- member variable for RangeImpl * setBaseRange() setRange() -- sets m_range as specified, unmodified baseRange() range() -- returns m_range unmodified baseRange range -- param to constructors and setter range() extendedRange() -- return extended range m_extended n/a -- remove. call range() or extendedRange() instead At the very least, the names baseRange, m_baseRange, and setBaseRange are a bit confusing because of the basePosition that's already part of the SelectionController. Also, note that setBaseRange() ensures that range() always returns the unextended range, so it's a little misleading not to call it setRange() to begin with. Also, do you have more comments on Darin's questions? Thanks.

Duncan Wilcox

Comment 18 2005-10-30 09:10:39 PST

David, thank you for your comments. I haven't looked at this patch for a long time, a fresh look makes me realize that the "m_extended" flag really means "don't touch the selection". Let me explain. Extending a selection basically mean applying granularity to the current selection. A naive implementation might consider no extension equivalent to applying character granularity to a selection. This would make matters really simple, no need for m_extended trickery. Unfortunately quite a few layout tests fail (or failed when I did this in an inital test implementation), because sometimes ranges are modified by normalization. I haven't quite spotted what the issue is or how it might be solved. In accordance with Darin's comments, the patch I have has a "range()" accessor that will return an extended range, while a "baseRange()" accessor will return the base range. Most of the time base range and extended range are exactly the same thing, they're only different when there's mouse interaction. Finally, if you are referring to Darin's comments in #12 I have a patch against an old tree that addresses all the issues that I didn't answer right away. I need to update the patch (perhaps rebuild it from scratch). I hope to be able to work on it soon.

Ahmad Saleem

Comment 19 2022-11-17 16:43:49 PST

I am not sure these KHTML era patches will apply and bring any progression but I am going to tag few so they can confirm if something can be salvaged from these or we can mark it as "RESOLVED WONTFIX". Thanks!

Note You need to log in before you can comment on or make changes to this bug.