Bug 72006 - Implement combobox appearance
Summary: Implement combobox appearance
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 27247
  Show dependency treegraph
 
Reported: 2011-11-10 01:25 PST by Keishi Hattori
Modified: 2015-03-29 18:37 PDT (History)
6 users (show)

See Also:


Attachments
preliminary patch (7.56 KB, patch)
2011-11-10 01:45 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2011-11-10 18:16 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
added test (8.60 KB, patch)
2011-11-10 19:44 PST, Keishi Hattori
tkent: review-
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-11-10 01:25:51 PST
We need to add a new -webkit-appearance combobox to implement datalist for Mac (Bug 27247). It should draw a NSComboBoxCell.
Comment 1 Keishi Hattori 2011-11-10 01:45:17 PST
Created attachment 114453 [details]
preliminary patch
Comment 2 Keishi Hattori 2011-11-10 01:51:11 PST
I have a couple of issues.

1. How do you figure out the comboBoxMargins? I tweaked it but it still sometimes gets clipped.
2. There seems to be no way to achieve the push state on the combo box button. Will using Carbon API HIComboBox solve it?
Is there some way to set the highlightBy on the NSComboBoxButtonCell inside the NSComboBoxCell?
Comment 3 Kent Tamura 2011-11-10 02:10:47 PST
(In reply to comment #2)
> 2. There seems to be no way to achieve the push state on the combo box button. Will using Carbon API HIComboBox solve it?
> Is there some way to set the highlightBy on the NSComboBoxButtonCell inside the NSComboBoxCell?

NSStepperCell for <input type=number> had similar problems, and so we are using HITheme API. See paintStepper() in ThemeMac.mm and Appearance.h.
Comment 4 Keishi Hattori 2011-11-10 02:32:10 PST
kThemeComboBox seems to work!
Comment 5 Keishi Hattori 2011-11-10 18:16:31 PST
Created attachment 114613 [details]
Patch
Comment 6 Keishi Hattori 2011-11-10 18:18:42 PST
I wrote a test but I couldn't find any examples where the test was testing just the -webkit-appearance so I didn't include it in the patch.

I will add a test when applying this style to input element.
Comment 7 Kent Tamura 2011-11-10 18:22:23 PST
(In reply to comment #6)
> I wrote a test but I couldn't find any examples where the test was testing just the -webkit-appearance so I didn't include it in the patch.
> 
> I will add a test when applying this style to input element.

We should have a test.  fast/forms/range/thumbslider-no-parent-slider.html is an example of -webkit-appearance testing.
Comment 8 Keishi Hattori 2011-11-10 19:44:27 PST
Created attachment 114616 [details]
added test
Comment 9 Keishi Hattori 2011-11-10 19:51:46 PST
Comment on attachment 114613 [details]
Patch

I noticed a mistake.
Comment 10 WebKit Review Bot 2011-11-10 19:52:17 PST
Attachment 114616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderTheme.cpp:103:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Kent Tamura 2011-11-10 19:53:47 PST
Comment on attachment 114616 [details]
added test

View in context: https://bugs.webkit.org/attachment.cgi?id=114616&action=review

> Source/WebCore/platform/mac/ThemeMac.mm:508
> +    HIThemeButtonDrawInfo drawInfo;

The code looks very similar to paintStepper().  Can you share the code with it?

> Source/WebCore/platform/mac/ThemeMac.mm:532
> +    // Center the stepper rectangle in the specified area.

stepper?

> LayoutTests/fast/forms/datalist/combobox-appearance.html:4
> +<script src="../../fast/js/resources/js-test-pre.js"></script>

js-test-pre.js and js-test-post.js are not needed.

> LayoutTests/fast/forms/datalist/combobox-appearance.html:13
> +<p>Test combobox -webkit-appearance</p>

Do not show the unnecessary text.

> LayoutTests/fast/forms/datalist/combobox-appearance.html:20
> +</html>

Please add an expectation image too.