[webkit-reviews] review denied: [Bug 107507] Adjust design of the Calendar Picker : [Attachment 183900] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 107507: Adjust design of the Calendar Picker
https://bugs.webkit.org/show_bug.cgi?id=107507

Attachment 183900: Patch
https://bugs.webkit.org/attachment.cgi?id=183900&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183900&action=review


> Source/WebCore/Resources/pagepopups/calendarPicker.css:166
> +    height: 24px;

Absolute height is dangerous because -webkit-control font depends on local
computer setting.

> Source/WebCore/Resources/pagepopups/calendarPicker.css:175
> +    line-height: 18px;

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:898
> +    disclosureTriangle.innerHTML = "<svg width=\"7\" height=\"5\"><polygon
points=\"0,1 7,1 3.5,5\" style=\"fill:#000000;\" /></svg>";

nit: You can avoid ugly escaping by using single quotes in the svg string.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:945
> +    this._left2.innerHTML = "<svg width=\"9\" height=\"7\"><polygon
points=\"0,3.5 4,7 4,0\" style=\"fill:#6e6e6e;\" /><polygon points=\"5,3.5 9,7
9,0\" style=\"fill:#6e6e6e;\" /></svg>";

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:950
> +    this._left1.innerHTML = "<svg width=\"4\" height=\"7\"><polygon
points=\"0,3.5 4,7 4,0\" style=\"fill:#6e6e6e;\" /></svg>";

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:962
> +    this._right1.innerHTML = "<svg width=\"4\" height=\"7\"><polygon
points=\"0,7 0,0, 4,3.5\" style=\"fill:#6e6e6e;\" /></svg>";

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:967
> +    this._right2.innerHTML = "<svg width=\"9\" height=\"7\"><polygon
points=\"4,3.5 0,7 0,0\" style=\"fill:#6e6e6e;\" /><polygon points=\"9,3.5 5,7
5,0\" style=\"fill:#6e6e6e;\" /></svg>";

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:-1018
> -    this._monthPopup.style.left = this._month.offsetLeft +
(this._month.offsetWidth - this._monthPopup.offsetWidth) / 2 + "px";
> -    this._monthPopup.style.top = this._month.offsetTop +
this._month.offsetHeight + "px";
> +    var bounds = this.picker.daysTableBounds();
> +    this._monthPopup.style.left = bounds.x + "px";
> +    this._monthPopup.style.top = bounds.y + "px";
> +    this._monthPopup.style.width = bounds.width + "px";
> +    this._monthPopup.style.height = bounds.height + "px";
>  
>      this._wall.style.display = "block";
>      this._wall.style.zIndex = "999"; // This should be smaller than the
z-index of monthPopup.
>  
> -    var popupHeight = this._monthPopup.clientHeight;
> -    var fullHeight = this._monthPopupContents.clientHeight;
> -    if (fullHeight > popupHeight) {
> -	   var selected = this._getSelection();
> -	   if (selected) {
> -	      var bottom = selected.offsetTop + selected.clientHeight;
> -	      if (bottom > popupHeight)
> -		  this._monthPopup.scrollTop = bottom - popupHeight;
> -	   }
> -	   this._monthPopup.style.webkitPaddingEnd = getScrollbarWidth() +
'px';
> -    }

Should we do this change now?  I think this change affects UX, and would be out
of scope of this bug (appearance change).

> Source/WebCore/Resources/pagepopups/chromium/calendarPickerChromium.css:10
> +.days-area-container:focus {
> +  -webkit-transition: border-color 200ms;

We use 4-space indentation.

> Source/WebCore/Resources/pagepopups/chromium/pickerCommonChromium.css:9
> +    inset 0 1px 2px rgba(255, 255, 255, 0.75);

Need additional indentation for this line.

> Source/WebCore/Resources/pagepopups/chromium/pickerCommonChromium.css:21
> +:enabled:hover:-webkit-any(button, input[type='button']) {
> +  background-image: -webkit-linear-gradient(#f0f0f0, #f0f0f0 38%, #e0e0e0);

We use 4-space indentation.

> Source/WebCore/Resources/pagepopups/chromium/pickerCommonChromium.css:35
> +:disabled:-webkit-any(button, input[type='button']) {
> +  background-image: -webkit-linear-gradient(#f1f1f1, #f1f1f1 38%, #e6e6e6);

ditto.

> Source/WebCore/Resources/pagepopups/chromium/pickerCommonChromium.css:43
> +:enabled:focus:-webkit-any(button, input[type='button']) {
> +  -webkit-transition: border-color 200ms;

ditto.

> Source/WebCore/WebCore.gyp/WebCore.gyp:-1350
> -	   ['OS=="mac"', {
> -	     'include_dirs': [
> -	       '<(chromium_src_dir)/third_party/apple_webkit',
> -	     ],

You shouldn't remove this block.