Bug 61275 – <input type=color> Mac UI appearance

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

[Keishi Hattori]

Implement UI appearance for the mac platform.

[Keishi Hattori]

Created attachment 94406 [details]
Implements the UI appearance for Mac

This patch must be applied after the patch from Bug 61273.

[Keishi Hattori]

Created attachment 94408 [details]
Screenshot of patch

[Kent Tamura]

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

[Keishi Hattori]

Created attachment 94753 [details]
fixed patch

[Early Warning System Bot]

Comment on attachment 94753 [details]
fixed patch

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

[Gyuyoung Kim]

Comment on attachment 94753 [details]
fixed patch

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

[Keishi Hattori]

Comment on attachment 94753 [details]
fixed patch

I didn't update all the build files.

[Kent Tamura]

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.

[Kent Tamura]

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

[WebKit Review Bot]

Comment on attachment 94753 [details]
fixed patch

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

[WebKit Review Bot]

Comment on attachment 94753 [details]
fixed patch

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

[WebKit Review Bot]

Comment on attachment 94753 [details]
fixed patch

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

[Collabora GTK+ EWS bot]

Comment on attachment 94753 [details]
fixed patch

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

[Keishi Hattori]

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

[Keishi Hattori]

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

[WebKit Review Bot]

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

[Keishi Hattori]

Comment on attachment 94902 [details]
Removed color-swatch -webkit-appearance

oops. forgot to add valueChanged to header file

[Kent Tamura]

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?

[Keishi Hattori]

Created attachment 94919 [details]
fixed header, css

[Keishi Hattori]

Created attachment 94922 [details]
fixed release

[Keishi Hattori]

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

[Kent Tamura]

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;">...

[Keishi Hattori]

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

[Keishi Hattori]

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

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 94945 [details]
removed virtual

[Kent Tamura]

Comment on attachment 94945 [details]
removed virtual

ok

[WebKit Commit Bot]

Comment on attachment 94945 [details]
removed virtual

Clearing flags on attachment: 94945

Committed r87372: <http://trac.webkit.org/changeset/87372>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexis Menard (darktears)]

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

[Kent Tamura]

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