Bug 61275

Summary: <input type=color> Mac UI appearance
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, gns, gustavo.noronha, joepeck, menard, mjs, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61430    
Bug Blocks: 29358, 61276    
Attachments:
Description Flags
Implements the UI appearance for Mac
none
Screenshot of patch
none
fixed patch
none
Removed color-swatch -webkit-appearance
webkit.review.bot: commit-queue-
fixed header, css
none
fixed release
none
added arbitrary size test. rearranged test to fit in pixel test. Changed ColorInputType to inherit InputType instead of BaseButtonInputType.
none
added expect color test to fail for chromium
none
removed virtual none

Description Keishi Hattori 2011-05-23 05:42:57 PDT
Implement UI appearance for the mac platform.
Comment 1 Keishi Hattori 2011-05-23 05:45:00 PDT
Created attachment 94406 [details]
Implements the UI appearance for Mac

This patch must be applied after the patch from Bug 61273.
Comment 2 Keishi Hattori 2011-05-23 06:00:50 PDT
Created attachment 94408 [details]
Screenshot of patch
Comment 3 Kent Tamura 2011-05-23 18:25:27 PDT
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'.
Comment 4 Keishi Hattori 2011-05-25 01:36:17 PDT
Created attachment 94753 [details]
fixed patch
Comment 5 Early Warning System Bot 2011-05-25 01:54:25 PDT
Comment on attachment 94753 [details]
fixed patch

Attachment 94753 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8727888
Comment 6 Gyuyoung Kim 2011-05-25 01:56:12 PDT
Comment on attachment 94753 [details]
fixed patch

Attachment 94753 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8734373
Comment 7 Keishi Hattori 2011-05-25 02:03:01 PDT
Comment on attachment 94753 [details]
fixed patch

I didn't update all the build files.
Comment 8 Kent Tamura 2011-05-25 02:22:00 PDT
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.
Comment 9 Kent Tamura 2011-05-25 02:23:39 PDT
(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 10 WebKit Review Bot 2011-05-25 03:02:45 PDT
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 11 WebKit Review Bot 2011-05-25 03:10:27 PDT
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 12 WebKit Review Bot 2011-05-25 03:43:54 PDT
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 13 Collabora GTK+ EWS bot 2011-05-25 05:14:09 PDT
Comment on attachment 94753 [details]
fixed patch

Attachment 94753 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8732711
Comment 14 Keishi Hattori 2011-05-25 19:15:24 PDT
Created attachment 94902 [details]
Removed color-swatch -webkit-appearance
Comment 15 Keishi Hattori 2011-05-25 19:20:35 PDT
> > 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 16 WebKit Review Bot 2011-05-25 20:09:17 PDT
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 17 Keishi Hattori 2011-05-25 20:15:20 PDT
Comment on attachment 94902 [details]
Removed color-swatch -webkit-appearance

oops. forgot to add valueChanged to header file
Comment 18 Kent Tamura 2011-05-25 21:57:34 PDT
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?
Comment 19 Keishi Hattori 2011-05-25 22:53:26 PDT
Created attachment 94919 [details]
fixed header, css
Comment 20 Keishi Hattori 2011-05-25 23:16:24 PDT
Created attachment 94922 [details]
fixed release
Comment 21 Keishi Hattori 2011-05-25 23:19:05 PDT
(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 22 Kent Tamura 2011-05-25 23:48:54 PDT
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;">...
Comment 23 Keishi Hattori 2011-05-26 00:45:56 PDT
Created attachment 94930 [details]
added arbitrary size test. rearranged test to fit in pixel test. Changed ColorInputType to inherit InputType instead of BaseButtonInputType.
Comment 24 Keishi Hattori 2011-05-26 01:03:05 PDT
Created attachment 94935 [details]
added expect color test to fail for chromium
Comment 25 Kent Tamura 2011-05-26 01:19:22 PDT
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.
Comment 26 Keishi Hattori 2011-05-26 02:03:09 PDT
Created attachment 94945 [details]
removed virtual
Comment 27 Kent Tamura 2011-05-26 02:06:54 PDT
Comment on attachment 94945 [details]
removed virtual

ok
Comment 28 WebKit Commit Bot 2011-05-26 02:58:41 PDT
Comment on attachment 94945 [details]
removed virtual

Clearing flags on attachment: 94945

Committed r87372: <http://trac.webkit.org/changeset/87372>
Comment 29 WebKit Commit Bot 2011-05-26 02:58:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Alexis Menard (darktears) 2011-06-16 10:30:32 PDT
(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.
Comment 31 Kent Tamura 2011-06-16 17:04:27 PDT
(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.