Summary: | When contentEditable, cursor doesn't change to hand | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Wood <dwood> | ||||||||||
Component: | HTML Editing | Assignee: | Graham Dennis <Graham.Dennis> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | Graham.Dennis, justin.garcia, timothy | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Dan Wood
2006-08-10 16:24:59 PDT
Created attachment 10416 [details]
patch for comment
I've had a stab at this, but I think my method is a bit hackish, and I'm looking for comments. As such no changelog or testcase is included in this patch.
To see why this is a bit hackish, consider the following sequence of events:
Say your current selection is either non-existent or outside of the editable div with the link. Now you mouse down on the link. At this stage, the selection is still what it was. Some time later (perhaps a really short time) you release the mouse. At this point, the selection is now inside the editable div with the link, and I'm not sure how to determine whether or not this link should be followed based on information available at this point.
To work around this, I've created an extra private variable in HTMLAnchorElement, m_rootEditableElementForSelectionOnMouseDown, which is the root editable element of the selection at the time of the mousedown event. This variable is cleared on either a mouseout or a mouseclick event. Using the value of this variable, during the mouseclick event, it is then possible to determine whether or not the link should be followed.
Does anyone have any better ideas?
Also, this patch does not use WebPreferences as it is still an initial hack. Do we want a preference for this, or is Dan's suggested behaviour OK to be default?
Comment on attachment 10416 [details]
patch for comment
This needs a test case and a change log.
Also, I think the patch has rotted because of changes to Frame::selectionController().
The actual code looks fine, although it's complicated enough that it might need some comments to explain what's going on. In particular, the extra code in selectCursor is verbose and all on one line. I'd like to see it made clearer, perhaps by writing an inline helper function with a suitable name and an appropriate comment.
As you say, I'm not sure the new behavior is OK for all WebKit clients. I'd like to hear opinions about that from Tim Hatcher and Justin Garcia on this point.
In the HTMLAnchorElement functions, are we guaranteed that the document will have a non-nil frame? In particular I'm concerned about the default event handler code, because I think events can be dispatched to a node after a document is no longer in a browser window. The new code may not be the only code in the file that's making the incorrect assumption that the document will have a non-0 frame.
Created attachment 10556 [details] patch 2 New and improved patch. This patch fixes the patch rot, adds comments, cleans up the selectCursor function and checks for frames that are 0. This patch also includes a ChangeLog and modifies the comments in the testcase for bug #7156 (a manual test) to mention the changes in behaviour made by this patch. This patch does not use WebPreferences, but I can add that if it is decided that that is desired. This patch looks good. We do want to have a WebPreferences pref to affect this. (In reply to comment #4) > This patch looks good. We do want to have a WebPreferences pref to affect this. > Any suggestions for a suitable name? editableLinksLiveWhenNotFocused? Something a bit more concise? Comment on attachment 10556 [details]
patch 2
Removing r? until I add in a WebPref
That name seems OK. Though we really need a tri-state per Dan's original request. This name would be fine for the current support, but not as good for a future thris state that restores Safai 2.0 behavior. Created attachment 10722 [details] patch 3 Newer and even more improved patch! This patch includes a quad-state WebPreference for editable link behaviour called WebKitEditableLinkBehaviour. The four states are: * AlwaysLive (Safari 2 behaviour) * OnlyLiveWithShiftKey (Previous WebKit-ToT behaviour) * LiveWhenNotFocused (Dan's behaviour, links are live only when the editable block containing them isn't focused) * DefaultBehaviour (This is the same as OnlyLiveWithShiftKey) I'm not certain that I've done the WebPreference correctly, but I've included ChangeLog entries anyway. No automatic testcase is included as I don't see how it is possible, however I have modified the manual testcase for bug 7156. I think that the default should be determined by a good UI review team -- the current TOT behavior seemed kind of an arbitrary change and it "breaks" existing applications. Maybe it's just me, but it seems that "LiveWhenNotFocused" is maybe a better default, or perhaps AlwaysLive. What do some of the Apple UI gurus (e.g. John Geylense) think? Comment on attachment 10722 [details]
patch 3
I think AlwaysLive would be the best default, since this will match Tiger's behavior.
This is the first WebPreference we have added like this... I think they should be enums in WebKit also. Using a string is OK, but the enum makes more sense I think. If we did use strings we would need to add constansts for all the values that can be used in an app, like the key.
Also you will need to make your change to WebPreferencesPrivate.h, since this is new API we will need to stage it there first until it can be approved inside Apple for API.
The WebCore part seems fine. You will need to make the WebCore and WebKit enums match, then you can just cast them back and forth in the bridge.
(In reply to comment #10) > (From update of attachment 10722 [details] [edit]) > I think AlwaysLive would be the best default, since this will match Tiger's > behavior. This would effectively un-fix bug #7156 for the default configuration... Is this intended? As I thought the change was made to match Firefox/WinIE behaviour. So is your suggestion that the preference should be stored in user defaults as an NSNumber instead of an NSString? As my intention was to make that preference human-readable, but I suppose it doesn't matter as that is technically private anyway. So for the WebKit enum, should the constants be called: WebKitEditableLinksDefaultBehaviour, WebKitEditableLinksAlwaysLive etc, as this would then be in the global namespace (I think)? For bug 7156, we can make Safari set preference to a preference that matches WinIE and Firefox. But I think the default for all WebKit clients should be AlwaysLive. Those enum names are good, and yes as a NSNumber. Created attachment 10833 [details]
patch 4
This patch uses an enum instead of an NSString in WebKit for the preference. I have also changed 'behaviour' to 'behavior' everywhere I used it in code as I figure patches should be in American English not Australian English. This patch passes the layout tests, but doesn't have a new layout test for reasons stated earlier.
Comment on attachment 10833 [details]
patch 4
r=me
Landed in r16760. |