Bug 87844 – Implement painting slider tick marks

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

[Keishi Hattori]

Implement painting slider tick marks for <datalist> for <input type=range>

[Keishi Hattori]

Created attachment 144987 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 145272 [details]
Patch

[Build Bot]

Comment on attachment 145272 [details]
Patch

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

[Build Bot]

Comment on attachment 145272 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 145272 [details]
Patch

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

[Keishi Hattori]

Created attachment 145503 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 145703 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

(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?

[Keishi Hattori]

Created attachment 145751 [details]
Patch

[Kent Tamura]

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).

[Kent Tamura]

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.

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 145991 [details]
Patch

[Keishi Hattori]

Created attachment 147306 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 147470 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 147507 [details]
Patch

[Keishi Hattori]

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.

[Gustavo Noronha (kov)]

Comment on attachment 147507 [details]
Patch

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

[Build Bot]

Comment on attachment 147507 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 147507 [details]
Patch

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

[Build Bot]

Comment on attachment 147507 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 147507 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 147507 [details]
Patch

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

[Jon Lee]

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

[Keishi Hattori]

(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!

[Kent Tamura]

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

[Keishi Hattori]

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

[Kent Tamura]

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.

[Keishi Hattori]

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

[Kent Tamura]

(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

[Keishi Hattori]

Created attachment 148234 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 148245 [details]
Patch

[Keishi Hattori]

Created attachment 152748 [details]
Patch

[Keishi Hattori]

Rebased.

[Gyuyoung Kim]

Comment on attachment 152748 [details]
Patch

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

[Keishi Hattori]

Created attachment 152912 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 152922 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 152933 [details]
Patch

[Kent Tamura]

Comment on attachment 152933 [details]
Patch

ok

[Keishi Hattori]

Created attachment 153175 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153175 [details]
Patch

Clearing flags on attachment: 153175

Committed r123072: <http://trac.webkit.org/changeset/123072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

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.