Summary: | WebKit editing throws exception when monochrome color dragged onto text | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Jalkut <jalkut> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Major | CC: | ap, darin, enrica, rakuco, rniwa, webkit.review.bot, zoltan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac (Intel) | ||||||||||
OS: | OS X 10.7 | ||||||||||
Attachments: |
|
Description
Daniel Jalkut
2011-12-16 18:12:17 PST
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. 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 ;) 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.
(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. 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.
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. Test looks great, just waiting to hear if we should just remove the if statement condition entirely. 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.
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 }. (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. (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. (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. (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. (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. 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.
The patch looks great to me. Thanks for the fix. (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 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> All reviewed patches have been landed. Closing bug. |