Bug 10338 – When contentEditable, cursor doesn't change to hand

• Component: HTML Editing
• Status: RESOLVED FIXED

[Dan Wood]

The somewhat recent change that Xenon put in that makes a contentEditable div NOT show the hand cursor and act as live links (unless the shift key is held down) is confusing Sandvox users, who think that their links are broken.

I think that there needs to be a WebPreference or some other mechanism to be able to control this behavior.  Some possible behaviors could be:

- Links are live no matter what (Released Safari WebKit)
- Links not live when contentEditable (current TOT)
- Links are not live when contentEditable div is focused; but live when div is NOT focused (THIS IS THE BEHAVIOR THAT I WOULD LIKE FOR SANDVOX)

[Graham Dennis]

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?

[Darin Adler]

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.

[Graham Dennis]

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.

[Timothy Hatcher]

This patch looks good. We do want to have a WebPreferences pref to affect this.

[Graham Dennis]

(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?

[Graham Dennis]

Comment on attachment 10556 [details]
patch 2

Removing r? until I add in a WebPref

[Timothy Hatcher]

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.

[Graham Dennis]

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.

[Dan Wood]

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?

[Timothy Hatcher]

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.

[Graham Dennis]

(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)?

[Timothy Hatcher]

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.

[Graham Dennis]

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.

[Timothy Hatcher]

Comment on attachment 10833 [details]
patch 4

r=me

[Mark Rowe (bdash)]

Landed in r16760.