Bug 61275

Created attachment 94406 [details] Implements the UI appearance for Mac This patch must be applied after the patch from Bug 61273.

Created attachment 94408 [details] Screenshot of patch

Comment on attachment 94406 [details] Implements the UI appearance for Mac View in context: https://bugs.webkit.org/attachment.cgi?id=94406&action=review > LayoutTests/fast/forms/color/input-appearance-color.html:7 > +<h2>Different Font Sizes</h2> > +<input type="color" list value="#FF0000" class="big-font"> > +<input type="color" list value="#FF0000" class="middle-font"> > +<input type="color" list value="#FF0000" class="small-font"> These test make no sense because there are no class definitions for big-font, middle-font, and small-font. You should write what appearance is correct. e.g. The appearance should be identical for various font sizes. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22837 > E1BE512E0CF6C512002EA959 /* XSLTUnicodeSort.h in Headers */, > 977E2E0F12F0FC9C00C13379 /* XSSFilter.h in Headers */, > FD537353137B651800008DCE /* ZeroPole.h in Headers */, > + C3EAEBB6138A2E7A00FB055F /* ColorSwatchElement.h in Headers */, Should be sorted. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25512 > E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp in Sources */, > 977E2E0E12F0FC9C00C13379 /* XSSFilter.cpp in Sources */, > FD537352137B651800008DCE /* ZeroPole.cpp in Sources */, > + C3EAEBB7138A2E7A00FB055F /* ColorSwatchElement.cpp in Sources */, ditto. > Source/WebCore/css/html.css:598 > +input[type="color"][list] { > + -webkit-appearance: menulist; > + width: 88px; > + height: 23px; > +} Should be wrapped with ENABLE_DATALIST > Source/WebCore/css/html.css:619 > +input[type="color"][list]::-webkit-color-swatch { > + border: 1px solid #000000; > + left: 8px; > + right: 24px; > +} ditto. > Source/WebCore/html/ColorInputType.cpp:98 > +RenderObject* ColorInputType::createRenderer(RenderArena* arena, RenderStyle*) const > +{ > + return new (arena) RenderBlock(element()); > +} Do you need to override this? InputType::createRenderer() is not enough? > Source/WebCore/html/shadow/ColorSwatchElement.h:44 > +class ColorSwatchElement : public HTMLDivElement { Will you add a lot of code to ColorSwatchElement and ColorSwatchWrapperElement? If so, we should use separated source fiels. If no, we should move ElementWithPseudoId in html/ValidationMessage.cpp to html/shadow/, and use it. > Source/WebCore/rendering/RenderThemeMac.mm:1369 > + HTMLInputElement* inputElement = static_cast<HTMLInputElement*> (o->node()->parentNode()->parentNode()->shadowHost()); Remove a space between '>' and '('. This line causes a crash if non-shadow element has the webkit-appearance. <p style="-webkit-appearance:color-swatch"></p> Also, you should use o->node()->shadowAncestorNode(). Don't use one-letter variables such as 'o' 'r'.

Created attachment 94753 [details] fixed patch

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8727888

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8734373

Comment on attachment 94753 [details] fixed patch I didn't update all the build files.

Comment on attachment 94753 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94753&action=review > LayoutTests/fast/forms/color/input-appearance-color.html:12 > +Regular color controls are identical for various font sizes.<br> > +<input type="color" value="#00FF00" style="font-size:18px"> > +<input type="color" value="#00FF00" class="font-size:11px"> > +<input type="color" value="#00FF00" class="font-size:5px"> > +List color controls have different sizes depending on font sizes.<br> > +<input type="color" list value="#FF0000" style="font-size:18px"> > +<input type="color" list value="#FF0000" class="font-size:11px"> > +<input type="color" list value="#FF0000" class="font-size:5px"> The test is still wrong. - You need to change class="font-size... to style="font-size... - Probably you want to add <br> before "List color...", to use <h3> for texts. - The appearance for input[type=color][list] works only if ENABLE_DATALIST. You should mention it in the test. > Source/WebCore/ChangeLog:28 > + * html/shadow/ElementWithPseudoId.cpp: > + (WebCore::ElementWithPseudoId::shadowPseudoId): > + * html/shadow/ElementWithPseudoId.h: Used for plain shadow nodes that need to be styled. > + (WebCore::ElementWithPseudoId::create): > + (WebCore::ElementWithPseudoId::ElementWithPseudoId): You can split the patch into two: Introducing ElementWithPseudoId.{cpp,h} and others. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:13533 > + C3EAEBB4138A2E7A00FB055F /* ElementWithPseudoId.h */, > + C3EAEBB5138A2E7A00FB055F /* ElementWithPseudoId.cpp */, > A7C9ABF61357A3BF00F5503F /* DetailsMarkerControl.cpp */, > A7C9ABF71357A3BF00F5503F /* DetailsMarkerControl.h */, You should sort lines by Tools/Scripts/sort-Xcode-project-file. > Source/WebCore/css/html.css:595 > + position: relative; Why ? > Source/WebCore/css/html.css:606 > + position: absolute; > + top: 4px; > + left: 2px; > + bottom: 4px; > + right: 2px; Why absolute position? Doesn't CSS margin work? > Source/WebCore/css/html.css:620 > + left: 8px; > + right: 24px; ditto. > Source/WebCore/rendering/RenderThemeMac.mm:1376 > + Node* shadowAncestorNode = node->shadowAncestorNode(); > + ASSERT(shadowAncestorNode); > + if (!shadowAncestorNode->hasTagName(HTMLNames::inputTag)) > + return true; I want something is drawn even if non-input element has -webkit-appearance:color-swatch.

(In reply to comment #8) > - Probably you want to add <br> before "List color...", to use <h3> for texts. to add <br> ..., or to use <h3>

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8727905

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8730790

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8734401

Comment on attachment 94753 [details] fixed patch Attachment 94753 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8732711

Created attachment 94902 [details] Removed color-swatch -webkit-appearance

> > Source/WebCore/css/html.css:595 > > + position: relative; > > Why ? > > > Source/WebCore/css/html.css:606 > > + position: absolute; > > + top: 4px; > > + left: 2px; > > + bottom: 4px; > > + right: 2px; > > Why absolute position? Doesn't CSS margin work? I can only get CSS margins to work when I set the width and height of the swatch element. I couldn't get it to work when width and height are set on the input element.

Comment on attachment 94902 [details] Removed color-swatch -webkit-appearance Attachment 94902 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8731913

Comment on attachment 94902 [details] Removed color-swatch -webkit-appearance oops. forgot to add valueChanged to header file

Comment on attachment 94902 [details] Removed color-swatch -webkit-appearance View in context: https://bugs.webkit.org/attachment.cgi?id=94902&action=review > Source/WebCore/html/ColorInputType.cpp:102 > + RefPtr<HTMLElement> wrapperElement = ElementWithPseudoId::create(document, "-webkit-color-swatch-wrapper"); > + RefPtr<HTMLElement> swatchElement = ElementWithPseudoId::create(document, "-webkit-color-swatch"); > + ExceptionCode ec = 0; > + wrapperElement->appendChild(swatchElement, ec); > + element()->ensureShadowRoot()->appendChild(wrapperElement, ec); * swatchElement is used only once. we can remove the local variable. * Passing RefPtr to appendChild() is not good in this case. You should write appendChild(wrapperElement.release(), ec) to avoid a temporal +1 of the reference counter. > Source/WebCore/html/ColorInputType.cpp:116 > +HTMLElement* ColorInputType::shadowColorSwatch() const > +{ Is this function used?

Created attachment 94919 [details] fixed header, css

Created attachment 94922 [details] fixed release

(In reply to comment #18) > > Source/WebCore/html/ColorInputType.cpp:116 > > +HTMLElement* ColorInputType::shadowColorSwatch() const > > +{ > > Is this function used? It is used one time in updateColorSwatch. Should it not be a function?

Comment on attachment 94922 [details] fixed release View in context: https://bugs.webkit.org/attachment.cgi?id=94922&action=review > LayoutTests/fast/forms/color/input-appearance-color.html:1 > +<h2>Default Appearance</h2> We had better test appearances in case that the element has size properties. e.g. <input type=color style="width: 100px; height: 6px;">...

Created attachment 94930 [details] added arbitrary size test. rearranged test to fit in pixel test. Changed ColorInputType to inherit InputType instead of BaseButtonInputType.

Created attachment 94935 [details] added expect color test to fail for chromium

Comment on attachment 94935 [details] added expect color test to fail for chromium View in context: https://bugs.webkit.org/attachment.cgi?id=94935&action=review > Source/WebCore/html/ColorInputType.h:52 > + virtual void updateColorSwatch(); I don't think this needs to be virtual for now.

Created attachment 94945 [details] removed virtual

Comment on attachment 94945 [details] removed virtual ok

Comment on attachment 94945 [details] removed virtual Clearing flags on attachment: 94945 Committed r87372: <http://trac.webkit.org/changeset/87372>

All reviewed patches have been landed. Closing bug.

(In reply to comment #29) > All reviewed patches have been landed. Closing bug. It's me or the RenderThemeMac changes and the ColorSwatchElement.* files are missing in the patch landed? I wanted to give a try to implement the feature for the Qt port.

(In reply to comment #30) > It's me or the RenderThemeMac changes and the ColorSwatchElement.* files are missing in the patch landed? They are unnecessary because we use the standard DOM+CSS rendering. I think we need to land a patch for Bug 62619 before starting port-dependent parts.