Bug 188464 – Support drag-and-drop for input[type=color]

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

This bug tracks the work required to bring drag-and-drop functionality to color inputs on macOS and iOS.

[Aditya Keerthi]

Created attachment 346886 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Aditya Keerthi]

Created attachment 346911 [details]
Patch

[EWS Watchlist]

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.

[Andy Estes]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Aditya Keerthi]

Created attachment 346938 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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.

[Daniel Bates]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Aditya Keerthi]

Created attachment 346994 [details]
Patch

[Aditya Keerthi]

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

[Aditya Keerthi]

Created attachment 347088 [details]
Patch

[Aditya Keerthi]

(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

[Daniel Bates]

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

[Aditya Keerthi]

Created attachment 347168 [details]
Patch

[Wenson Hsieh]

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.

[Aditya Keerthi]

Created attachment 347173 [details]
Patch

[Wenson Hsieh]

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.

[Aditya Keerthi]

Created attachment 347225 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347225 [details]
Patch

Clearing flags on attachment: 347225

Committed r234930: <https://trac.webkit.org/changeset/234930>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43385153>

[Ross Kirsling]

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 '{'

[Ross Kirsling]

(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