Bug 61275 - <input type=color> Mac UI appearance
Summary: <input type=color> Mac UI appearance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on: 61430
Blocks: 29358 61276
  Show dependency treegraph
 
Reported: 2011-05-23 05:42 PDT by Keishi Hattori
Modified: 2011-06-16 17:04 PDT (History)
10 users (show)

See Also:


Attachments
Implements the UI appearance for Mac (56.77 KB, patch)
2011-05-23 05:45 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Screenshot of patch (48.55 KB, image/png)
2011-05-23 06:00 PDT, Keishi Hattori
no flags Details
fixed patch (74.85 KB, patch)
2011-05-25 01:36 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Removed color-swatch -webkit-appearance (73.24 KB, patch)
2011-05-25 19:15 PDT, Keishi Hattori
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fixed header, css (73.84 KB, patch)
2011-05-25 22:53 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed release (72.66 KB, patch)
2011-05-25 23:16 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
added arbitrary size test. rearranged test to fit in pixel test. Changed ColorInputType to inherit InputType instead of BaseButtonInputType. (57.75 KB, patch)
2011-05-26 00:45 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
added expect color test to fail for chromium (58.57 KB, patch)
2011-05-26 01:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
removed virtual (58.56 KB, patch)
2011-05-26 02:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.