Bug 188464 - Support drag-and-drop for input[type=color]
Summary: Support drag-and-drop for input[type=color]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-09 18:51 PDT by Aditya Keerthi
Modified: 2018-08-16 15:29 PDT (History)
17 users (show)

See Also:


Attachments
Patch (71.49 KB, patch)
2018-08-09 19:31 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.14 MB, application/zip)
2018-08-09 21:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.14 MB, application/zip)
2018-08-09 23:45 PDT, EWS Watchlist
no flags Details
Patch (75.47 KB, patch)
2018-08-10 10:28 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.65 MB, application/zip)
2018-08-10 12:04 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.14 MB, application/zip)
2018-08-10 12:12 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.47 MB, application/zip)
2018-08-10 12:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.13 MB, application/zip)
2018-08-10 14:39 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.49 MB, application/zip)
2018-08-10 14:40 PDT, EWS Watchlist
no flags Details
Patch (65.70 KB, patch)
2018-08-10 16:21 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.40 MB, application/zip)
2018-08-10 17:17 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.01 MB, application/zip)
2018-08-10 18:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews205 for win-future (12.95 MB, application/zip)
2018-08-11 08:25 PDT, EWS Watchlist
no flags Details
Patch (66.51 KB, patch)
2018-08-12 18:23 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (68.44 KB, patch)
2018-08-14 10:24 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (75.49 KB, patch)
2018-08-15 09:37 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (74.86 KB, patch)
2018-08-15 10:14 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (74.83 KB, patch)
2018-08-15 16:51 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2018-08-09 18:51:52 PDT
This bug tracks the work required to bring drag-and-drop functionality to color inputs on macOS and iOS.
Comment 1 Aditya Keerthi 2018-08-09 19:31:56 PDT
Created attachment 346886 [details]
Patch
Comment 2 EWS Watchlist 2018-08-09 19:33:39 PDT
Attachment 346886 [details] did not pass style-queue:


ERROR: Source/WebCore/page/EventHandler.cpp:3705:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3707:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EWS Watchlist 2018-08-09 21:38:29 PDT
Comment on attachment 346886 [details]
Patch

Attachment 346886 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8815457

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
accessibility/roles-exposed.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
fast/selectors/read-only-read-write-input-basics.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html
editing/pasteboard/drag-and-drop-color-input.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html
Comment 4 EWS Watchlist 2018-08-09 21:38:31 PDT
Created attachment 346889 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-08-09 23:45:44 PDT
Comment on attachment 346886 [details]
Patch

Attachment 346886 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8816200

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
accessibility/roles-exposed.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
fast/selectors/read-only-read-write-input-basics.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html
editing/pasteboard/drag-and-drop-color-input.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html
Comment 6 EWS Watchlist 2018-08-09 23:45:46 PDT
Created attachment 346898 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 7 Aditya Keerthi 2018-08-10 10:28:04 PDT
Created attachment 346911 [details]
Patch
Comment 8 EWS Watchlist 2018-08-10 10:30:45 PDT
Attachment 346911 [details] did not pass style-queue:


ERROR: Source/WebCore/page/EventHandler.cpp:3705:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3707:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Andy Estes 2018-08-10 11:32:00 PDT
Comment on attachment 346911 [details]
Patch

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

> Source/WebCore/page/DragController.cpp:348
> +static bool isColorInput(Node& node)

This function is slightly misnamed, because it's also checking whether the input element is not disabled.

> Source/WebCore/platform/ios/DragImageIOS.mm:258
> +    RetainPtr<UIGraphicsImageRenderer> render = adoptNS([allocUIGraphicsImageRendererInstance() initWithSize:imageRect.size()]);

This would be a good place to use auto.

> Source/WebCore/platform/mac/DragImageMac.mm:346
> +    RetainPtr<NSImage> dragImage = adoptNS([[NSImage alloc] initWithSize:NSMakeSize(swatchWidth, swatchWidth)]);

Ditto.

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:297
> +            RetainPtr<NSString> title = (NSString *)source.linkTitle;
> +            RetainPtr<NSURL> url = (NSURL *)source.linkURL;
> +            dragItem.previewProvider = [title, url, draggingCenter] () -> UIDragPreview * {

You can avoid some retain count churn by WTFMove()ing title and url in the capture list. Even better, you could avoid these local variables entirely by writing the capture list like this: [title = retainPtr((NSString *)source.linkTitle), url = retainPtr((NSURL *)source.linkURL)].

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:307
> +            RetainPtr<UIImage> image = source.image;
> +            dragItem.previewProvider = [image] () -> UIDragPreview * {

Ditto for image.

> Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:280
> +    WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPasteboardProxy::SetPasteboardColor(pasteboardName, color), Messages::WebPasteboardProxy::SetPasteboardColor::Reply(newChangeCount), 0);

sendSync() can time out, and if so I think you'd end up with an uninitialized newChangeCount. You should either initialize newChangeCount to 0 or handle sendSync() failing.
Comment 10 EWS Watchlist 2018-08-10 12:04:45 PDT
Comment on attachment 346911 [details]
Patch

Attachment 346911 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8820766

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
accessibility/roles-exposed.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
fast/selectors/read-only-read-write-input-basics.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html
editing/pasteboard/drag-and-drop-color-input.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html
Comment 11 EWS Watchlist 2018-08-10 12:04:47 PDT
Created attachment 346915 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-08-10 12:12:40 PDT
Comment on attachment 346911 [details]
Patch

Attachment 346911 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8820779

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
accessibility/roles-exposed.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
fast/selectors/read-only-read-write-input-basics.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html
Comment 13 EWS Watchlist 2018-08-10 12:12:41 PDT
Created attachment 346917 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2018-08-10 12:42:18 PDT
Comment on attachment 346911 [details]
Patch

Attachment 346911 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8820777

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
fast/forms/color/input-appearance-color.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/color.html
imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html
Comment 15 EWS Watchlist 2018-08-10 12:42:20 PDT
Created attachment 346918 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 16 EWS Watchlist 2018-08-10 14:39:53 PDT
Comment on attachment 346911 [details]
Patch

Attachment 346911 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8821532

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
accessibility/roles-exposed.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
fast/selectors/read-only-read-write-input-basics.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/selection.html
editing/pasteboard/drag-and-drop-color-input.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-validity-badInput.html
Comment 17 EWS Watchlist 2018-08-10 14:39:55 PDT
Created attachment 346927 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 EWS Watchlist 2018-08-10 14:40:45 PDT
Comment on attachment 346911 [details]
Patch

Attachment 346911 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8821548

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application.html
imported/w3c/web-platform-tests/html/semantics/forms/constraints/form-validation-willValidate.html
fast/forms/color/input-appearance-color.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/valueMode.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/color.html
imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-elements-filter.html
imported/w3c/web-platform-tests/html/dom/reflection-forms.html
Comment 19 EWS Watchlist 2018-08-10 14:40:47 PDT
Created attachment 346928 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 20 Aditya Keerthi 2018-08-10 16:21:49 PDT
Created attachment 346938 [details]
Patch
Comment 21 EWS Watchlist 2018-08-10 17:17:52 PDT
Comment on attachment 346938 [details]
Patch

Attachment 346938 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8823661

New failing tests:
editing/pasteboard/drag-and-drop-color-input.html
Comment 22 EWS Watchlist 2018-08-10 17:17:54 PDT
Created attachment 346943 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 23 EWS Watchlist 2018-08-10 18:32:13 PDT
Comment on attachment 346938 [details]
Patch

Attachment 346938 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8824076

New failing tests:
editing/pasteboard/drag-and-drop-color-input.html
Comment 24 EWS Watchlist 2018-08-10 18:32:15 PDT
Created attachment 346947 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 25 Wenson Hsieh 2018-08-10 19:49:21 PDT
Comment on attachment 346938 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Implemented createDragImageForColor to display a particular color swatch once

Nit - these are good comments to put next to the relevant functions a bit further down.

> Source/WebCore/page/DragController.cpp:350
> +    Node* n = node.deprecatedShadowAncestorNode();

For variable names, we usually prefer full words over single letters or abbreviations (https://webkit.org/code-style-guidelines/#names-full-words)

> Source/WebCore/platform/mac/PasteboardMac.mm:230
> +    platformStrategies()->pasteboardStrategy()->setTypes(types, m_pasteboardName);

Nit - this could be made a tiny bit cleaner by using an initializer list to create the types vector.
Comment 26 Daniel Bates 2018-08-10 20:37:39 PDT
Comment on attachment 346938 [details]
Patch

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

I haven’t read through the entire patch. Here are some comment so far.

> Source/WebCore/page/DragController.cpp:353
> +        if (input.isColorControl() && !input.isDisabledFormControl())

We should not allow dragging when the control is read-only.

> Source/WebCore/page/DragController.cpp:1179
> +        Color color = input.valueAsColor();

Auto?

> Source/WebCore/page/DragController.cpp:1185
> +        dragImageOffset = IntPoint(dragImageSize(dragImage.get()));

Uniform intialization syntax?

> Source/WebCore/platform/ios/DragImageIOS.mm:256
> +    FloatRect imageRect(0, 0, elementRect.width() * pageScaleFactor, elementRect.height() * pageScaleFactor);

Uniform initialization syntax?

> Source/WebCore/platform/ios/DragImageIOS.mm:259
> +    UIImage *image = [render.get() imageWithActions:^(UIGraphicsImageRendererContext *rendererContext) {

The .get() is not needed.

> Source/WebCore/platform/ios/DragImageIOS.mm:260
> +        GraphicsContext context(rendererContext.CGContext);

Uniform initialization syntax.

> Source/WebCore/platform/ios/DragImageIOS.mm:263
> +        context.fillRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)), color);

Ditto. Also, we should cache the rounded rect in a local and then reference it here and in line 266 below as opposed to computing it twice.

> Source/WebCore/platform/ios/DragImageIOS.mm:266
> +    visiblePath.addRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)));

How did you come up with 4 here and above?

> Source/WebCore/platform/mac/DragDataMac.mm:94
> +    return String(UIColorPboardType);

Uniform initialization syntax?
Comment 27 EWS Watchlist 2018-08-11 08:25:19 PDT
Comment on attachment 346938 [details]
Patch

Attachment 346938 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8828474

New failing tests:
editing/pasteboard/drag-and-drop-color-input.html
Comment 28 EWS Watchlist 2018-08-11 08:25:31 PDT
Created attachment 346961 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 29 Aditya Keerthi 2018-08-12 18:23:35 PDT
Created attachment 346994 [details]
Patch
Comment 30 Aditya Keerthi 2018-08-12 18:25:04 PDT
(In reply to Daniel Bates from comment #26)
> Comment on attachment 346938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346938&action=review
> 
> > Source/WebCore/platform/ios/DragImageIOS.mm:266
> > +    visiblePath.addRoundedRect(FloatRoundedRect(imageRect, FloatRoundedRect::Radii(4 * pageScaleFactor)));
> 
> How did you come up with 4 here and above?

This value matches NSColorWell's drag-and-drop implementation.
Comment 31 Aditya Keerthi 2018-08-14 10:24:08 PDT
Created attachment 347088 [details]
Patch
Comment 32 Aditya Keerthi 2018-08-14 10:25:51 PDT
(In reply to Daniel Bates from comment #26)
> Comment on attachment 346938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346938&action=review
>
> > Source/WebCore/page/DragController.cpp:353
> > +        if (input.isColorControl() && !input.isDisabledFormControl())
> 
> We should not allow dragging when the control is read-only.

According to the specification, only text controls can be marked read-only. I've changed the code back to use isDisabledFormControl().

https://www.w3.org/TR/html5/sec-forms.html#the-readonly-attribute
Comment 33 Daniel Bates 2018-08-14 11:50:23 PDT
(In reply to Aditya Keerthi from comment #32)
> (In reply to Daniel Bates from comment #26)
> > Comment on attachment 346938 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=346938&action=review
> >
> > > Source/WebCore/page/DragController.cpp:353
> > > +        if (input.isColorControl() && !input.isDisabledFormControl())
> > 
> > We should not allow dragging when the control is read-only.
> 
> According to the specification, only text controls can be marked read-only.
> I've changed the code back to use isDisabledFormControl().
> 
> https://www.w3.org/TR/html5/sec-forms.html#the-readonly-attribute

OK. Please add a test.
Comment 34 Aditya Keerthi 2018-08-15 09:37:38 PDT
Created attachment 347168 [details]
Patch
Comment 35 Wenson Hsieh 2018-08-15 09:54:24 PDT
Comment on attachment 347168 [details]
Patch

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

> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:305
> +                UIDragPreviewParameters *parameters = [[[UIDragPreviewParameters alloc] initWithTextLineRects:@[[NSValue valueWithCGRect:[imageView bounds]]]] autorelease];

Nit - I think we usually put spaces around the inside of the outer square brackets in array initialized like this: `@[ … ]`.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:111
> +TEST(DragAndDropTests, ColorInputInputEvent)

I think you could consider consolidating this and ColorInputChangeEvent into a single test that checks for both events, since their setup and steps are identical.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/color-drop.html:10
> +    setTimeout(() => window.webkit.messageHandlers.testHandler.postMessage("change"), 0);

Hm...I don't see why we need to schedule these on a timer.

> Tools/TestWebKitAPI/Tests/mac/DragAndDropTestsMac.mm:62
> +TEST(DragAndDropPasteboardTests, DropColor)

Nit - s/DragAndDropPasteboardTests/DragAndDropTests/

> Tools/TestWebKitAPI/Tests/mac/DragAndDropTestsMac.mm:72
> +    auto hostWindow = adoptNS([[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 400, 400) styleMask:0 backing:NSBackingStoreBuffered defer:NO]);

I don't think we need to add the web view in this window, since TestWKWebView is hosted in an NSWindow by default.
Comment 36 Aditya Keerthi 2018-08-15 10:14:41 PDT
Created attachment 347173 [details]
Patch
Comment 37 Wenson Hsieh 2018-08-15 14:05:24 PDT
Comment on attachment 347173 [details]
Patch

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

> Source/WebCore/page/DragController.cpp:917
> +    bool isColorControl = (is<HTMLInputElement>(state.source.get()) && downcast<HTMLInputElement>(*state.source.get()).isColorControl());

Nit - I don't think the extra ()'s around the RHS are needed. (The second .get() can also be removed).

> Source/WebCore/page/DragController.cpp:1177
> +        auto& input = downcast<HTMLInputElement>(*state.source.get());

Nit - The .get() is unnecessary here.

> Source/WebCore/page/mac/DragControllerMac.mm:148
> +        supportedTypes.append(String(UIColorPboardType));

Nit - The explicit call to String() shouldn't be needed here.
Comment 38 Aditya Keerthi 2018-08-15 16:51:33 PDT
Created attachment 347225 [details]
Patch
Comment 39 WebKit Commit Bot 2018-08-16 09:44:24 PDT
Comment on attachment 347225 [details]
Patch

Clearing flags on attachment: 347225

Committed r234930: <https://trac.webkit.org/changeset/234930>
Comment 40 WebKit Commit Bot 2018-08-16 09:44:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Radar WebKit Bug Importer 2018-08-16 09:47:49 PDT
<rdar://problem/43385153>
Comment 42 Ross Kirsling 2018-08-16 13:02:43 PDT
MSVC Debug evidently does not support #ifs inside an ASSERT macro:
https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/10949/steps/compile-webkit/logs/stdio

> Source\WebCore\page/EventHandler.cpp(3707): error C2121: '#': invalid character: possibly the result of a macro expansion
> Source\WebCore\page/EventHandler.cpp(3699): error C2059: syntax error: 'if'
> Source\WebCore\page/EventHandler.cpp(3707): error C2143: syntax error: missing ';' before '{'
Comment 43 Ross Kirsling 2018-08-16 14:43:23 PDT
(In reply to Ross Kirsling from comment #42)
> MSVC Debug evidently does not support #ifs inside an ASSERT macro:
> https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> 10949/steps/compile-webkit/logs/stdio
> 
> > Source\WebCore\page/EventHandler.cpp(3707): error C2121: '#': invalid character: possibly the result of a macro expansion
> > Source\WebCore\page/EventHandler.cpp(3699): error C2059: syntax error: 'if'
> > Source\WebCore\page/EventHandler.cpp(3707): error C2143: syntax error: missing ';' before '{'

Note that Clang too will give the following warning with -Wembedded-directive:
> warning: embedding a directive within macro arguments has undefined behavior