Bug 74775 – WebKit editing throws exception when monochrome color dragged onto text

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Jalkut]

Note: this was reproduced with WebKit nightly as of two days ago. 

In any app that supports WebKit editing, if a color is dragged onto text, typically the dragged color is interpreted to mean that the text should be changed to that color.

If the color being dragged happens to be monochrome, e.g. of the NSCalibratedWhiteColorSpace, then WebKit throws an exception because it fails to get the expected RGB components from the dragged color.

This is an issue for any WebKit editing app that allows users to display a standard color panel, and then to drag a color swatch onto the edited text. It's an especially heinous issues in apps, like my own, that treat uncaught exceptions as crash-worthy events ;)

To reproduce in Safari or WebKit nightly:

1. Open the Snippet editor.
2. Enter this HTML content:

<div contentEditable="true">Hello - try dragging a black/white colorspace color to me.</div>

3. Select the editable section in the rendering pane of the snippet editor.
4. From another app, e.g. TextEdit, open the standard Color panel.
5. Switch to the "Sliders" color panel pane, and choose "Gray Scale Slider" from the popup. 
6. Drag whatever color you have selected from the large color preview rectangle next to the magnifying glass, while cmd-tabbing back to Safari to your snippet content.
7. Drag it to the text and release.

Result: nothing happens, and Safari logs to the console:

2011-12-16 21:04:10.792 Safari[40273:1d07] *** Canceling drag because exception 'NSInvalidArgumentException' (reason '*** -redComponent not valid for the NSColor NSCalibratedWhiteColorSpace 0.375912 1; need to first convert colorspace.') was raised during a dragging session

More typically, in an app that supports the Color panel natively, the exception is thrown with a full backtrace. For example, I interrupted Safari with gdb and called:

call (void) [[NSColorPanel sharedColorPanel] makeKeyAndOrderFront:nil]

Then I dragged a color to the text and received this in gdb:

2011-12-16 21:02:39.357 Safari[40273:1d07] *** -redComponent not valid for the NSColor NSCalibratedWhiteColorSpace 0.754032 1; need to first convert colorspace.
2011-12-16 21:02:39.367 Safari[40273:1d07] (
	0   CoreFoundation                      0x00007fff8b04b286 __exceptionPreprocess + 198
	1   libobjc.A.dylib                     0x00007fff8f80dd5e objc_exception_throw + 43
	2   CoreFoundation                      0x00007fff8b04b0ba +[NSException raise:format:arguments:] + 106
	3   CoreFoundation                      0x00007fff8b04b044 +[NSException raise:format:] + 116
	4   AppKit                              0x00007fff8dfbd46c -[NSColor redComponent] + 65
	5   WebCore                             0x0000000100c427a7 _ZNK7WebCore8DragData7asColorEv + 55
	6   WebCore                             0x0000000100c3efcc _ZN7WebCore14DragController16concludeEditDragEPNS_8DragDataE + 236
	7   WebCore                             0x0000000100c3ed01 _ZN7WebCore14DragController11performDragEPNS_8DragDataE + 593
	8   WebKit                              0x00000001003963a7 -[WebView performDragOperation:] + 199
	9   AppKit                              0x00007fff8e05cc6a NSCoreDragReceiveMessageProc + 1462
	10  HIServices                          0x00007fff86fe760f CallReceiveMessageCollectionWithMessage + 70
	11  HIServices                          0x00007fff86fe7714 DoMultipartDropMessage + 126
	12  HIServices                          0x00007fff86fe791d DoDropMessage + 55
	13  HIServices                          0x00007fff86fe7df5 SendDropMessage + 39
	14  HIServices                          0x00007fff86feab08 DragInApplication + 717
	15  HIServices                          0x00007fff86feadaa CoreDragStartDragging + 517
	16  AppKit                              0x00007fff8e05ab4c -[NSCoreDragManager _dragUntilMouseUp:accepted:] + 885
	17  AppKit                              0x00007fff8e05a218 -[NSCoreDragManager dragImage:fromWindow:at:offset:event:pasteboard:source:slideBack:] + 1455
	18  AppKit                              0x00007fff8e34ed86 -[NSWindow(NSDrag) dragImage:at:offset:event:pasteboard:source:slideBack:] + 132
	19  AppKit                              0x00007fff8dfd1260 +[NSColorPanel dragColor:withEvent:fromView:] + 785
	20  AppKit                              0x00007fff8dfcc80e -[NSColorSwatch mouseDown:] + 547
	21  AppKit                              0x00007fff8dd520e0 -[NSWindow sendEvent:] + 6306
	22  AppKit                              0x00007fff8dcea68f -[NSApplication sendEvent:] + 5593
	23  Safari                              0x00007fff895496f8 -[BrowserApplication sendEvent:] + 822
	24  AppKit                              0x00007fff8dc80682 -[NSApplication run] + 555
	25  AppKit                              0x00007fff8deff80c NSApplicationMain + 867
	26  Safari                              0x00007fff896ffa6f SafariMain + 197
	27  Safari                              0x0000000100000f24 Safari + 3876

[Alexey Proskuryakov]

Daniel, would you be willing to make a patch <http://www.webkit.org/coding/contributing.html>? This should be easy to fix, at least as long as the expectation is to get an RGB color in HTML.

[Daniel Jalkut]

Hi Alexey - sure, I will take a crack at it. I am not currently set up for WebKit dev, so it might take a me a little while to get my environment back up to speed. Feel free to leave this with me until I either attach a patch or throw my arms up ;)

[Daniel Jalkut]

Created attachment 120185 [details]
Fix the NSColor exception thrown by converting to RGB colorspace if necessary

The attached patch addresses the problem by checking ahead of time whether the provided color uses an RGB colorspace model. If it doesn't, it converts the color to RGB colorspace before accessing its components.

I don't think this affects layout tests, and I'm not sure if there is a straightforward way to cover the bug using any other test mechanism. In IRC it was suggested that a manual test may be appropriate?

I built and tested the fix on Lion.

[Ryosuke Niwa]

(In reply to comment #3)
> I don't think this affects layout tests, and I'm not sure if there is a straightforward way to cover the bug using any other test mechanism. In IRC it was suggested that a manual test may be appropriate?

Yeah, I don't think eventSender, etc... would work here. Could you add a manual test instead? it's in trunk/ManualTests.

[Daniel Jalkut]

Created attachment 120196 [details]
Patch for bug fix and patch for manual test case

I added a manual test case to the patch. Hope that looks good! Thanks for the guidance.

[Darin Adler]

Comment on attachment 120196 [details]
Patch for bug fix and patch for manual test case

View in context: https://bugs.webkit.org/attachment.cgi?id=120196&action=review

> Source/WebCore/platform/mac/DragDataMac.mm:121
>      NSColor *color = [NSColor colorFromPasteboard:m_pasteboard.get()];
> +    
> +    // The supplied color may not be in an RGB colorspace. This commonly occurs when e.g.
> +    // a color is dragged from the NSColorPanel grayscale picker. Before we attempt to access RGB components,
> +    // ensure it's appropriately converted to an RGB colorspace.
> +    if ([[color colorSpace] colorSpaceModel] != NSRGBColorSpaceModel) {
> +        color = [color colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
> +    }

Why not call that method unconditionally? Is there a case that would behave worse? We don’t put braces around a single-line if statement body in WebKit codings style.

I suggest removing the third sentence of that comment, since it just repeats what the code does.

[Darin Adler]

Test looks great, just waiting to hear if we should just remove the if statement condition entirely.

[WebKit Review Bot]

Attachment 120196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
 + 3a244c5...0a192a1 master     -> origin/master  (forced update)
LayoutTests/inspector/extensions/extensions-api-expected.txt: needs merge
update-index --refresh: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 120196 [details]
Patch for bug fix and patch for manual test case

View in context: https://bugs.webkit.org/attachment.cgi?id=120196&action=review

r- due to various nits.

> ChangeLog:3
> +        Add manual test covering failure to accept grayscale color drags to contentEditable regions.

This line should repeat what the bug summary.

> ManualTests/drag-color-to-contenteditable.html:3
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
> +        "http://www.w3.org/TR/html4/strict.dtd">
> +<html lang="en">

Can we use HTML5 style DOCTYPE?
<!DOCTYPE html>
<html>

> ManualTests/drag-color-to-contenteditable.html:5
> +<head>
> +</head>

We don't need head.

> ManualTests/drag-color-to-contenteditable.html:11
> +<li>Open a color panel in some host app that facilitates doing so, such as TextEdit.app.</li>

I don't understand what you mean by "that facilitates doing so". I think it's better to say "Open a color panel in some app such as TextEdit.app."

> Source/WebCore/ChangeLog:4
> +        Handle non-RGB colorspace colors in the Mac platform drag manager. Fixes NSException thrown 
> +        when dragging monochrome colors to contentEditable regions.

This line should repeat what the bug summary. You can add a long description below "Reviewed by" line followed by a blank line.

> Source/WebCore/platform/mac/DragDataMac.mm:121
> +    if ([[color colorSpace] colorSpaceModel] != NSRGBColorSpaceModel) {
> +        color = [color colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
> +    }

WebKit style is not to wrap single statement with { and }.

[Daniel Jalkut]

(In reply to comment #6)
> (From update of attachment 120196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120196&action=review
> 
> > Source/WebCore/platform/mac/DragDataMac.mm:121
> >      NSColor *color = [NSColor colorFromPasteboard:m_pasteboard.get()];
> > +    
> > +    // The supplied color may not be in an RGB colorspace. This commonly occurs when e.g.
> > +    // a color is dragged from the NSColorPanel grayscale picker. Before we attempt to access RGB components,
> > +    // ensure it's appropriately converted to an RGB colorspace.
> > +    if ([[color colorSpace] colorSpaceModel] != NSRGBColorSpaceModel) {
> > +        color = [color colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
> > +    }
> 
> Why not call that method unconditionally? Is there a case that would behave worse? We don’t put braces around a single-line if statement body in WebKit codings style.
> 
> I suggest removing the third sentence of that comment, since it just repeats what the code does.

My thinking for testing is that there could be some nuance of the color that a client wants to be conveyed and preserved without a potentially lossy conversion. Since I have to pick a specific RGB-model colorspace to convert to, doing so unilaterally would e.g. convert a device-RGB or custom-colorspace color to calibrated RGB colorspace when it's not called for.

[Darin Adler]

(In reply to comment #10)
> My thinking for testing is that there could be some nuance of the color that a client wants to be conveyed and preserved without a potentially lossy conversion. Since I have to pick a specific RGB-model colorspace to convert to, doing so unilaterally would e.g. convert a device-RGB or custom-colorspace color to calibrated RGB colorspace when it's not called for.

An interesting though. I wonder if this does any good in practice. Or any harm. If neither, then I suppose it does not matter how we write the code.

I suppose the conservative thing is to write the code as you have since it matches the old behavior in more cases.

[Daniel Jalkut]

(In reply to comment #9)
> (From update of attachment 120196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120196&action=review
> 
> r- due to various nits.

Thank you for the feedback. I will create a new patch.

FWIW I created the test by copying "template.html" in the ManualTests folder. So some of the HTML-level nits are from what I assumed to be the canonical starting point for tests.

[Darin Adler]

(In reply to comment #12)
> FWIW I created the test by copying "template.html" in the ManualTests folder. So some of the HTML-level nits are from what I assumed to be the canonical starting point for tests.

We should file a separate bug to track fixing that.

[Daniel Jalkut]

(In reply to comment #11)
> An interesting though. I wonder if this does any good in practice. Or any harm. If neither, then I suppose it does not matter how we write the code.
> 
> I suppose the conservative thing is to write the code as you have since it matches the old behavior in more cases.

That was my thinking. I doubt that it will do much good in practice, but as it's so unpredictable what variety of applications will use contentEditable, I thought it was worth playing it safe. The ONLY cases where the code will behavior differently under the patch are when it's avoiding an imminent exception.

[Daniel Jalkut]

Created attachment 120204 [details]
Patch for fix and test with review corrections applied

I have incorporated the suggested revisions. Let me know if I missed anything.

[Ryosuke Niwa]

The patch looks great to me. Thanks for the fix.

[Daniel Jalkut]

(In reply to comment #16)
> The patch looks great to me. Thanks for the fix.

Great, glad to hear it!

I submitted a patch for the template file changes as suggested:

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

[WebKit Review Bot]

Comment on attachment 120204 [details]
Patch for fix and test with review corrections applied

Clearing flags on attachment: 120204

Committed r103507: <http://trac.webkit.org/changeset/103507>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.