Bug 97201 – Add suggestionPicker to CalendarPicker

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

[Keishi Hattori]

Add suggestionPicker to CalendarPicker

[Keishi Hattori]

Created attachment 165069 [details]
Patch

[Keishi Hattori]

Created attachment 165071 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 165102 [details]
Patch

[Keishi Hattori]

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

[Kent Tamura]

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.

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 165118 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 165306 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165306 [details]
Patch

Clearing flags on attachment: 165306

Committed r129326: <http://trac.webkit.org/changeset/129326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.