Bug 97201 - Add suggestionPicker to CalendarPicker
Summary: Add suggestionPicker to CalendarPicker
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:
Blocks: 96085
  Show dependency treegraph
 
Reported: 2012-09-20 05:17 PDT by Keishi Hattori
Modified: 2012-09-23 21:38 PDT (History)
2 users (show)

See Also:


Attachments
Patch (27.32 KB, patch)
2012-09-21 00:34 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (27.11 KB, patch)
2012-09-21 00:48 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (27.70 KB, patch)
2012-09-21 03:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (29.29 KB, patch)
2012-09-21 04:43 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (29.35 KB, patch)
2012-09-23 20:47 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 2012-09-20 05:17:08 PDT
Add suggestionPicker to CalendarPicker
Comment 1 Keishi Hattori 2012-09-21 00:34:00 PDT
Created attachment 165069 [details]
Patch
Comment 2 Keishi Hattori 2012-09-21 00:48:52 PDT
Created attachment 165071 [details]
Patch
Comment 3 Kent Tamura 2012-09-21 01:27:37 PDT
Comment on attachment 165071 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/pickerCommon.js:80
> + * @return {!Element}

This is nullable.

> Source/WebCore/Resources/pagepopups/pickerCommon.js:82
> +Node.prototype.enclosingNodeOrSelfWithClass = function(className)

Polluting the Node prototype is not good in general.
Adding this function to the Node prototype looks no benefit.

> Source/WebCore/Resources/pagepopups/suggestionPicker.css:20
> +    background-color: #3875d7;

How did you decide this color?

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:63
> + * @param {!String} title

The typename should be "string", not "String"

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:90
> +    var entryElement = createElement("div", "suggestion-entry");

The class name should be "suggestion-entry-or-action" or something.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:102
> +    this._containerElement = createElement("div", "suggestion-list");

nit: because it's a list, how about using <ul> and <li>?

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:104
> +    for (var i = 0; i < this._config.suggestionValues.length; i++) {

We prefer ++i

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:113
> +        var otherEntry = this._createActionEntryElement(this._config.otherDateLabel, "openCalendarPicker");

We had better introduce a const variable for the action name.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:118
> +    // To measure the required width, we first set the class to "measuring-width" which

We had better move the code below to its own function(s).

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:133
> +    for (var i = 0; i < this._containerElement.childNodes.length; i++) {

We prefer ++i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:180
> +    for (var i = 0; i < childNodes.length; i++) {

We prefer ++i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:196
> +    for (var i = childNodes.length - 1; i >= 0; i--){

We prefer --i.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:217
> +            for (var node = document.activeElement.previousElementSibling; node && node; node = node.previousElementSibling) {

'node && node' is redundant.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:230
> +            for (var node = document.activeElement.nextElementSibling; node && node; node = node.nextElementSibling) {

ditto.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:277
> +    if (!document.activeElement.classList.contains("suggestion-entry"))

This class name appears multiple times.  Please consider introducing const variable for it like calendarPicker.js does.
Comment 4 Keishi Hattori 2012-09-21 03:03:11 PDT
Created attachment 165102 [details]
Patch
Comment 5 Keishi Hattori 2012-09-21 03:03:54 PDT
Comment on attachment 165071 [details]
Patch

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

>> Source/WebCore/Resources/pagepopups/pickerCommon.js:80
>> + * @return {!Element}
> 
> This is nullable.

Done.

>> Source/WebCore/Resources/pagepopups/pickerCommon.js:82
>> +Node.prototype.enclosingNodeOrSelfWithClass = function(className)
> 
> Polluting the Node prototype is not good in general.
> Adding this function to the Node prototype looks no benefit.

I brought it from the web inspector utilities but I agree that its not good even inside a page popup. Changed to function.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.css:20
>> +    background-color: #3875d7;
> 
> How did you decide this color?

Yes this color is not right. We need to expose activeListBoxSelectionBackground/Foreground as CSS colors. I have to fix that.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:63
>> + * @param {!String} title
> 
> The typename should be "string", not "String"

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:90
>> +    var entryElement = createElement("div", "suggestion-entry");
> 
> The class name should be "suggestion-entry-or-action" or something.

Changed to suggestion-list-entry

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:102
>> +    this._containerElement = createElement("div", "suggestion-list");
> 
> nit: because it's a list, how about using <ul> and <li>?

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:104
>> +    for (var i = 0; i < this._config.suggestionValues.length; i++) {
> 
> We prefer ++i

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:113
>> +        var otherEntry = this._createActionEntryElement(this._config.otherDateLabel, "openCalendarPicker");
> 
> We had better introduce a const variable for the action name.

Introduced SuggestionPicker.ActionNames.OpenCalendarPicker

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:118
>> +    // To measure the required width, we first set the class to "measuring-width" which
> 
> We had better move the code below to its own function(s).

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:133
>> +    for (var i = 0; i < this._containerElement.childNodes.length; i++) {
> 
> We prefer ++i.

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:180
>> +    for (var i = 0; i < childNodes.length; i++) {
> 
> We prefer ++i.

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:196
>> +    for (var i = childNodes.length - 1; i >= 0; i--){
> 
> We prefer --i.

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:217
>> +            for (var node = document.activeElement.previousElementSibling; node && node; node = node.previousElementSibling) {
> 
> 'node && node' is redundant.

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:230
>> +            for (var node = document.activeElement.nextElementSibling; node && node; node = node.nextElementSibling) {
> 
> ditto.

Done.

>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:277
>> +    if (!document.activeElement.classList.contains("suggestion-entry"))
> 
> This class name appears multiple times.  Please consider introducing const variable for it like calendarPicker.js does.

I use this class inside querySelector so I worry it might get confusing.
this._element.querySelector(".suggestion-list-entry:first-child").focus();
Comment 6 Kent Tamura 2012-09-21 03:25:56 PDT
Comment on attachment 165071 [details]
Patch

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

>>> Source/WebCore/Resources/pagepopups/suggestionPicker.js:277
>>> +    if (!document.activeElement.classList.contains("suggestion-entry"))
>> 
>> This class name appears multiple times.  Please consider introducing const variable for it like calendarPicker.js does.
> 
> I use this class inside querySelector so I worry it might get confusing.
> this._element.querySelector(".suggestion-list-entry:first-child").focus();

You can concatenate strings like
    "." + ClassEntry + ":first-child"

We easily make a typo in a string literal, and it's hard to find such bugs. We had better use constant variables for literals as possible.
Comment 7 Kent Tamura 2012-09-21 03:34:44 PDT
Comment on attachment 165071 [details]
Patch

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

>>> Source/WebCore/Resources/pagepopups/suggestionPicker.css:20
>>> +    background-color: #3875d7;
>> 
>> How did you decide this color?
> 
> Yes this color is not right. We need to expose activeListBoxSelectionBackground/Foreground as CSS colors. I have to fix that.

I don't think adding activeListBoxSelection* as CSS system colors is reasonable.
 - We should not add an extension to general CSS processing just for the suggestion picker.
 - CSS system color itself is deprecated in CSS3.

You can pass the color value via the config object from C++ to JavaScript.
Comment 8 Keishi Hattori 2012-09-21 04:43:29 PDT
Created attachment 165118 [details]
Patch
Comment 9 Kent Tamura 2012-09-21 05:11:16 PDT
Comment on attachment 165118 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/pickerCommon.js:86
> +    for (var node = selfNode; node && node !== selfNode.ownerDocument; node = node.parentNode)
> +        if (node.nodeType === Node.ELEMENT_NODE && node.classList.contains(className))
> +            return node;

should wrap with { } because the content of 'for' has two lines.

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:71
> +    var text = ".suggestion-list-entry:focus {\

should use ListEntryClass

> Source/WebCore/Resources/pagepopups/suggestionPicker.js:74
> +    text += ".suggestion-list-entry:focus .label { color: " + this._config.suggestionHighlightTextColor + "; }";

ditto.
Comment 10 Keishi Hattori 2012-09-23 20:47:32 PDT
Created attachment 165306 [details]
Patch
Comment 11 WebKit Review Bot 2012-09-23 21:38:50 PDT
Comment on attachment 165306 [details]
Patch

Clearing flags on attachment: 165306

Committed r129326: <http://trac.webkit.org/changeset/129326>
Comment 12 WebKit Review Bot 2012-09-23 21:38:53 PDT
All reviewed patches have been landed.  Closing bug.