| Summary: | Add suggestionPicker to CalendarPicker | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||||
| Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | tkent, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 96085 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Keishi Hattori
2012-09-20 05:17:08 PDT
Created attachment 165069 [details]
Patch
Created attachment 165071 [details]
Patch
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. Created attachment 165102 [details]
Patch
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 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 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. Created attachment 165118 [details]
Patch
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. Created attachment 165306 [details]
Patch
Comment on attachment 165306 [details] Patch Clearing flags on attachment: 165306 Committed r129326: <http://trac.webkit.org/changeset/129326> All reviewed patches have been landed. Closing bug. |