Bug 87844

Created attachment 144987 [details] Patch

Comment on attachment 144987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144987&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Tests will be added when turned on. This patch affect platforms with ENABLE_DATALIST, right? So, we need tests now. > Source/WebCore/rendering/RenderThemeChromiumLinux.h:63 > + virtual Color sliderTickColor() const; > + virtual IntSize sliderTickSize() const; > + virtual IntSize sliderThumbSize() const; > + virtual int sliderTickOffsetFromTrackCenter() const; Need OVERRIDE.

Created attachment 145272 [details] Patch

Comment on attachment 145272 [details] Patch Attachment 145272 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12870439

Comment on attachment 145272 [details] Patch Attachment 145272 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12868491

Comment on attachment 145272 [details] Patch Attachment 145272 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12861601

Created attachment 145503 [details] Patch

Comment on attachment 145503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145503&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:163 > + style()->setHeight(Length(trackHeight, Fixed)); > + } else { Don't we need to set trackRender's height? > Source/WebCore/rendering/RenderTheme.cpp:1014 > + i.context->setFillColor(sliderTickColor(), ColorSpaceDeviceRGB); How about using o->style()->color() here and put the default color to html.css? We can remove sliderTickColor(), and page authors can override the color. > LayoutTests/fast/forms/datalist/input-appearance-range-with-datalist.html:1 > +<input type=range list=foo /> - Please add a vertical slider too. - We had better have another slider of which width is different in order to see tick marks are correctly positioned. - Tick marks are not shown in renderer dumps. So we may call layoutTestController.dumpAsText(true) to merge text results into one.

Created attachment 145703 [details] Patch

Comment on attachment 145703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145703&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:154 > + if (trackRenderer) > + trackRenderer->style()->setHeight(thumbRenderer->style()->height()); > +#if ENABLE(DATALIST) I'd like to change this to Length height = thumbRenderer->style()->height(); if (trackRenderr) trackRenderer->style()->setHeight(height); and > Source/WebCore/html/shadow/SliderThumbElement.cpp:167 > + style()->setHeight(Length(trackHeight, Fixed)); > + } else > +#endif > + style()->setHeight(thumbRenderer->style()->height()); height = Length(trackHeight, Fixed); } style()->setHeight(height); > Source/WebCore/rendering/RenderTheme.h:217 > + virtual IntSize sliderTickSize() const; Unclear if this is for horizontal or vertical. > Source/WebCore/rendering/RenderTheme.h:218 > + virtual IntSize sliderThumbSize() const; This should be integrated with RenderTheme::adjustSliderThumbSize(). > Source/WebCore/rendering/RenderTheme.h:219 > + virtual int sliderTickOffsetFromTrackCenter() const; We have an assumption that abs(sliderTickOffsetFromTrackCenter) * 2 > height-of-slider-thumb, right? We should add a comment.

(In reply to comment #10) > (From update of attachment 145703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145703&action=review > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:154 > > + if (trackRenderer) > > + trackRenderer->style()->setHeight(thumbRenderer->style()->height()); > > +#if ENABLE(DATALIST) > > I'd like to change this to > > Length height = thumbRenderer->style()->height(); > if (trackRenderr) > trackRenderer->style()->setHeight(height); > > and > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:167 > > + style()->setHeight(Length(trackHeight, Fixed)); > > + } else > > +#endif > > + style()->setHeight(thumbRenderer->style()->height()); > > height = Length(trackHeight, Fixed); > } > style()->setHeight(height); Done. > > Source/WebCore/rendering/RenderTheme.h:217 > > + virtual IntSize sliderTickSize() const; > > Unclear if this is for horizontal or vertical. Renamed to horizontalSliderTickSize and horizontalSliderThumbSize. > > Source/WebCore/rendering/RenderTheme.h:218 > > + virtual IntSize sliderThumbSize() const; > > This should be integrated with RenderTheme::adjustSliderThumbSize(). Done. > > Source/WebCore/rendering/RenderTheme.h:219 > > + virtual int sliderTickOffsetFromTrackCenter() const; > > We have an assumption that abs(sliderTickOffsetFromTrackCenter) * 2 > height-of-slider-thumb, right? We should add a comment. Yes it would be true as long as the length of the thumb on the tick side of the track is longer than the other side. Does this assumption need to be in the comment?

Created attachment 145751 [details] Patch

Comment on attachment 145751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145751&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:168 > + style()->setHeight(Length(trackHeight, Fixed)); > + } else > +#endif > + style()->setHeight(height); height = Length(trachHeight, Fixed)); } style()->setHeight(height); > Source/WebCore/rendering/RenderTheme.cpp:992 > + int tickRegionMargin = (thumbSize.width() - tickSize.width()) / 2.0; You assume horizontal thumb width == vertical thumb height. I don't think we should introduce such restriction. We should have RenderThem::sliderThmbSize(ControlPart) or we should obtain the thumb size from an existing renderer. > Source/WebCore/rendering/RenderTheme.cpp:994 > + int tickRegionSideMargin = rect.x() + tickRegionMargin; > + int tickRegionWidth = rect.width() - tickRegionMargin * 2 - tickSize.width(); The initial values are not used. > Source/WebCore/rendering/RenderTheme.cpp:1014 > + double value = parseToDoubleForNumberType(input->sanitizeValue(optionElement->value()), std::numeric_limits<double>::quiet_NaN()); You can omit the last argument of parseToDoubleForNumberType(). Also, you have to check isfinite(value).

Comment on attachment 145751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145751&action=review >> Source/WebCore/rendering/RenderTheme.cpp:1014 >> + double value = parseToDoubleForNumberType(input->sanitizeValue(optionElement->value()), std::numeric_limits<double>::quiet_NaN()); > > You can omit the last argument of parseToDoubleForNumberType(). > Also, you have to check isfinite(value). Also, you need to reject value<min and value>max. HTMLInputElement::isValidValue() might be useful.

Comment on attachment 145751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145751&action=review > LayoutTests/fast/forms/datalist/input-appearance-range-with-datalist.html:17 > +<datalist id=foo> > + <option>0</option>, > + <option>10</option> > + <option>20</option> > + <option>40</option> > + <option>80</option> > + <option>100</option> > +</datalist> Please add <option>-10</option> <option>120</option> <option>50.1</option> <option>Infinity</option> etc.

Created attachment 145991 [details] Patch

Created attachment 147306 [details] Patch

Comment on attachment 147306 [details] Patch Attachment 147306 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12960006 New failing tests: fast/repaint/slider-thumb-float.html fast/dom/HTMLInputElement/input-slider-update.html fast/dom/HTMLInputElement/input-slider-update-styled.html fast/repaint/slider-thumb-drag-release.html

Created attachment 147429 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 147470 [details] Patch

Comment on attachment 147470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147470&action=review > Source/WebCore/css/html.css:573 > + color: #909090; Let's make a separated patch to update html.css, themeChromiumLinux.css, and themeWin.css. > Source/WebCore/rendering/RenderTheme.cpp:972 > +IntSize RenderTheme::sliderTickSize() const > +{ > + return IntSize(1, 1); > +} > + > +int RenderTheme::sliderTickOffsetFromTrackCenter() const > +{ > + return 0; > +} Remove them. These default implementations are useless. > Source/WebCore/rendering/RenderTheme.cpp:993 > + RenderObject* thumbRenderer = input->shadow()->oldestShadowRoot()->firstChild()->firstChild()->firstChild()->renderer(); We had better have a helper function to get a thumb element. > Source/WebCore/rendering/RenderTheme.h:220 > + // Returns size of one slider tick mark for a horizontal track. > + virtual IntSize sliderTickSize() const; > + // Returns the distance of slider tick origin from the slider track center. > + virtual int sliderTickOffsetFromTrackCenter() const; Make them pure virtual. Also, we need a comment for sliderTickSize about rotation for vertical sliders. > Source/WebCore/rendering/RenderTheme.h:221 > + virtual void paintSliderTicks(RenderObject*, const PaintInfo&, const IntRect&); This can be non-virtual.

Created attachment 147507 [details] Patch

Comment on attachment 147470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147470&action=review >> Source/WebCore/css/html.css:573 >> + color: #909090; > > Let's make a separated patch to update html.css, themeChromiumLinux.css, and themeWin.css. Done. Bug 89067 >> Source/WebCore/rendering/RenderTheme.cpp:972 >> +} > > Remove them. These default implementations are useless. Done. >> Source/WebCore/rendering/RenderTheme.cpp:993 >> + RenderObject* thumbRenderer = input->shadow()->oldestShadowRoot()->firstChild()->firstChild()->firstChild()->renderer(); > > We had better have a helper function to get a thumb element. Done. Added HTMLInputElement::sliderThumbElement() >> Source/WebCore/rendering/RenderTheme.h:220 >> + virtual int sliderTickOffsetFromTrackCenter() const; > > Make them pure virtual. > Also, we need a comment for sliderTickSize about rotation for vertical sliders. Done. >> Source/WebCore/rendering/RenderTheme.h:221 >> + virtual void paintSliderTicks(RenderObject*, const PaintInfo&, const IntRect&); > > This can be non-virtual. Done.

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12958147

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12961103

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12965075

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12955315

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12949898

Comment on attachment 147507 [details] Patch Attachment 147507 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12956258

As an aside: are there no proposals for CSS attributes that allow for specifying which side the ticks appear?

(In reply to comment #30) > As an aside: are there no proposals for CSS attributes that allow for specifying which side the ticks appear? I'm afraid not. If you have plans to use it, we have been thinking about ideas for doing that kind of non standard customizations for form controls. I would love to discuss!

Comment on attachment 147507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147507&action=review r- because we need rebase. > Source/WebCore/html/shadow/SliderThumbElement.cpp:150 > + RenderObject* thumbRenderer = input->shadow()->oldestShadowRoot()->firstChild()->firstChild()->firstChild()->renderer(); We had better use sliderThumbElement().

Created attachment 148078 [details] Reabsed, used HTMLInputElement::sliderThumbElement, fixed Qt build.

Comment on attachment 148078 [details] Reabsed, used HTMLInputElement::sliderThumbElement, fixed Qt build. View in context: https://bugs.webkit.org/attachment.cgi?id=148078&action=review The code looks good. But the patch looks broken. r- because of purple EWS > LayoutTests/fast/forms/range/slider-zoomed-expected.txt:2 > -PASS: slider value set to 72 > +FAIL: expected slider value 72, got 71 Is this related to this bug? > LayoutTests/platform/chromium-linux/fast/dom/HTMLInputElement/input-slider-update-expected.txt:-1 > -layer at (0,0) size 800x600 ditto. > LayoutTests/platform/chromium-linux/fast/dom/HTMLInputElement/input-slider-update-styled-expected.txt:-1 > -layer at (0,0) size 800x600 ditto. > LayoutTests/platform/chromium-linux/fast/repaint/slider-thumb-drag-release-expected.txt:-2 > - RenderView at (0,0) size 800x600 ditto. > LayoutTests/platform/chromium-linux/fast/repaint/slider-thumb-float-expected.txt:-1 > -layer at (0,0) size 800x600 ditto. > LayoutTests/platform/chromium-win/fast/dom/HTMLInputElement/input-slider-update-expected.txt:6 > - RenderSlider {INPUT} at (2,2) size 129x21 [color=#C4C4C4] [bgcolor=#FFFFFF] > + RenderSlider {INPUT} at (2,2) size 129x21 [bgcolor=#FFFFFF] ditto. > LayoutTests/platform/chromium-win/fast/repaint/slider-thumb-drag-release-expected.txt:6 > - RenderSlider {INPUT} at (2,2) size 129x21 [color=#C4C4C4] [bgcolor=#FFFFFF] > + RenderSlider {INPUT} at (2,2) size 129x21 [bgcolor=#FFFFFF] ditto. > Source/WebCore/rendering/RenderTheme.cpp:1023 > + if (!o->style()->isLeftToRightDirection()) > + tickPosition = 1.0 - tickPosition; wrong indentation > Source/WebCore/rendering/RenderThemeChromiumWin.cpp:475 > + PlatformSupport::paintTrackbar(painter.context(), > + themeData.m_part, > + themeData.m_state, > + themeData.m_classicState, > + painter.drawRect()); inconsistent indentation.

Created attachment 148226 [details] Removed unrelated test expectation changes.

(In reply to comment #35) > Created an attachment (id=148226) [details] > Removed unrelated test expectation changes. The patch doesn't contain binary diff. That's the reason of purple EWS. +</datalist> diff --git a/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-expected.png b/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-expected.png new file mode 100644 index 0000000..25b1824 Binary files /dev/null and b/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-expected.png differ diff --git a/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png b/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png new file mode 100644 index 0000000..b9a5eb2 Binary files /dev/null and b/LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png differ diff --git a/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-expected.png b/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-expected.png new file mode 100644 index 0000000..ad48c70 Binary files /dev/null and b/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-expected.png differ diff --git a/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png b/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png new file mode 100644 index 0000000..7a9c091 Binary files /dev/null and b/LayoutTests/platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png differ diff --git a/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-expected.png b/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-expected.png new file mode 100644 index 0000000..74f650d Binary files /dev/null and b/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-expected.png differ diff --git a/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png b/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png new file mode 100644 index 0000000..d1ce0fa Binary files /dev/null and b/LayoutTests/platform/chromium-win/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png differ diff --git a/LayoutTests/platform/chromium/fast/forms/datalist/input-list-expected.txt b/LayoutTests/platform/chromium/fast/forms/datalist/input-list-expected.txt index 4981a80..5556966 100644 --- a/LayoutTests/platform/chromium/fast/forms/datalist/input-list-expected.txt

Created attachment 148234 [details] Patch

Comment on attachment 148234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148234&action=review Almost ok. But please do not commit this patch until resolving the <datalist> update issue. > Source/WebCore/platform/qt/RenderThemeQt.cpp:412 > +IntSize RenderThemeQt::sliderTickSize() const > +{ > + return IntSize(0, 0); > +} > + > +int RenderThemeQt::sliderTickOffsetFromTrackCenter() const > +{ > + return 0; > +} > + Need to wrap by #if ENABLE(DATALIST) - #endif. Need to add FIXME comments.

Created attachment 148245 [details] Patch

Created attachment 152748 [details] Patch

Rebased.

Comment on attachment 152748 [details] Patch Attachment 152748 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13278142

Created attachment 152912 [details] Patch

Comment on attachment 152912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152912&action=review > Source/WebCore/ChangeLog:9 > + > + This implements painting slider tick marks for <datalist> support for input type=range. > + Please add a sentence about a defect that this doesn't support automatic-update by list target changes and a following patch will fix it. > LayoutTests/ChangeLog:17 > + * platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-expected.png: Added. > + * platform/chromium-mac/fast/forms/datalist/input-appearance-range-with-datalist-zoomed-expected.png: Added. Probably these pixel results won't work on chromium-mac-snowleopard. Please add a line to platform/chromium/TestExpectations.

Created attachment 152922 [details] Patch

Comment on attachment 152922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152922&action=review > LayoutTests/platform/chromium/TestExpectations:3762 > +BUG87844 REBASELINE SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist-rtl.html = IMAGE+TEXT > +BUG87844 REBASELINE SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist-zoomed.html = IMAGE+TEXT > +BUG87844 REBASELINE SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist.html = IMAGE+TEXT Do not add REBASELINE. It will confuse a WebKit gardener. IMAGE+TEXT looks incorrect. The text result doesn't depend on platforms. I think it should be IMAGE.

Created attachment 152933 [details] Patch

Comment on attachment 152933 [details] Patch ok

Created attachment 153175 [details] Patch

Comment on attachment 153175 [details] Patch Clearing flags on attachment: 153175 Committed r123072: <http://trac.webkit.org/changeset/123072>

All reviewed patches have been landed. Closing bug.

Comment on attachment 153175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153175&action=review > LayoutTests/platform/chromium/TestExpectations:3709 > +BUG87844 SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist-rtl.html = IMAGE > +BUG87844 SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist-zoomed.html = IMAGE > +BUG87844 SNOWLEOPARD : fast/forms/datalist/input-appearance-range-with-datalist.html = IMAGE This was failing new-run-webkit-tests --lint-test-files because it was missing the WK. Fixed in r123109.