Bug 52920

Created attachment 79790 [details] =WIP: Mostly working, moar cleening needed.

I am abandoning this idea for now, because of the sheer amount of fiddling with styles and minimizing style recalc is hard. However, we should take this as a use case of how this could be implemented in Javascript when we have the component model (see bug 52962), without sacrificing performance.

This is useful if we want to animate the slider thumb.

Yes, setting the thumb position with CSS properties is really difficult. The thumb position should be updated when the track size is changed, and we must not update CSS properties during RenderSlider::layout().

(In reply to comment #4) > Yes, setting the thumb position with CSS properties is really difficult. The thumb position should be updated when the track size is changed, and we must not update CSS properties during RenderSlider::layout(). I don't think it's that difficult. If we step back and look at the context of the change -- why did the track size changed? -- we can trace it down to the DOM change and the point where we should adjust CSS style and dirty style/layout bits. In case of resize, we should rely on CSS to position using percentages.

(In reply to comment #5) > I don't think it's that difficult. If we step back and look at the context of the change -- why did the track size changed? -- we can trace it down to the DOM change and the point where we should adjust CSS style and dirty style/layout bits. In case of resize, we should rely on CSS to position using percentages. Using percentage position is hard. Unlike <meter>, the slider thumb has its width. So we need to do something like... ::-webkit-slider-container-horizontal { display: -webkit-box; -webkit-box-orient: horizontal; -webkit-box-align: center; width: 100%; height: 100%; } ::-webkit-slider-runnable-track { display: block; -webkit-box-flex: 1; } ::-webkit-slider-track-goal-horizontal { -webkit-appearrance: sliderthumb-horizontal; display: block; -webkit-box-flex: 0; visibility: hidden; } ::-webkit-slider-thumb { position: relative; left: NN%; } <input type=range> (shadow) <div ::-webkit-slider-container-horizontal> <div ::-webkit-slider-runnable-track> <div ::-webkit-slider-thumb></div> </div> <div ::-webkit-slider-track-goal-horizontal></div> </div> Then, we'll have a problem similar to one <meter> had. ::-webkit-slider-container-horizontal and ::-webkit-slider-track-goal-horizontal are depend on orientation. A page author will need to specify additional style declarations to use vertical sliders. .vertical-range { -webkit-appearance: slider-vertical; } .vertical-range::-webkit-slider-slider-container-horizontal { -webkit-box-orient: vertical; } .vertical-range::-webkit-slider-track-goal-horizontal { -webkit-appearance: sliderthumb-vertical; } .vertical-range::-webkit-slider-thumb { -webkit-appearance: sliderthumb-vertical; } Is there better way to solve this?

(In reply to comment #6) > Then, we'll have a problem similar to one <meter> had. ::-webkit-slider-container-horizontal and ::-webkit-slider-track-goal-horizontal are depend on orientation. A page author will need to specify additional style declarations to use vertical sliders. I found SliderThumbElement::layout() handles such case. The page author don't need to specify orientation properties for shadow elements. So, it's ok to use percentage positions in type=range.

Created attachment 95336 [details] WIP by tkent

Comment on attachment 95336 [details] WIP by tkent I like where this is going.

Created attachment 96560 [details] Try This is an almost-complete patch without ChangeLog and test result updates

Comment on attachment 96560 [details] Try View in context: https://bugs.webkit.org/attachment.cgi?id=96560&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:84 > + SliderThumbElement* thumb = static_cast<SliderThumbElement*>(node()); I'll fix this wrong cast. This renderer is used by TrackGoalElement too. > Source/WebCore/html/shadow/SliderThumbElement.cpp:289 > +// FIXME: Should move to own files? Should we move them to new files? They are small and I hesitate to move. > Source/WebCore/html/shadow/SliderThumbElement.cpp:291 > +inline TrackGoalElement::TrackGoalElement(Document* document) I don't think this is a good name.

Comment on attachment 96560 [details] Try View in context: https://bugs.webkit.org/attachment.cgi?id=96560&action=review This is a very good shot. I am annoyed at the vertical/horizontal code, but I can't think of a better way to fix this. > Source/WebCore/html/RangeInputType.cpp:212 > + container->appendChild(TrackGoalElement::create(document), ec); This looks good for now, but with CSS3 calc, should we be able to get rid of the TrackGoalElement? left: calc(100% - 10px); > Source/WebCore/html/shadow/SliderThumbElement.cpp:99 > + if (parentStyle->appearance() == SliderVerticalPart) { > style()->setAppearance(SliderThumbVerticalPart); > - else if (parentStyle->appearance() == SliderHorizontalPart) > + isVertical = true; > + } else if (parentStyle->appearance() == SliderHorizontalPart) > style()->setAppearance(SliderThumbHorizontalPart); > else if (parentStyle->appearance() == MediaSliderPart) > style()->setAppearance(MediaSliderThumbPart); > - else if (parentStyle->appearance() == MediaVolumeSliderPart) > + else if (parentStyle->appearance() == MediaVolumeSliderPart) { > style()->setAppearance(MediaVolumeSliderThumbPart); > - > + isVertical = true; > + } I keep wracking my brains trying to figure out how to get rid of this. >> Source/WebCore/html/shadow/SliderThumbElement.cpp:289 >> +// FIXME: Should move to own files? > > Should we move them to new files? > They are small and I hesitate to move. I agree. small classes like this are ok to keep in one file. >> Source/WebCore/html/shadow/SliderThumbElement.cpp:291 >> +inline TrackGoalElement::TrackGoalElement(Document* document) > > I don't think this is a good name. Let's see if we can brainstorm this.. What is this element for? It's a stub at the end of the track, whose purpose is to limit the movement of the thumb... Right? Maybe it's a limiter of some sort?

Comment on attachment 96560 [details] Try View in context: https://bugs.webkit.org/attachment.cgi?id=96560&action=review Thank you for the pre-review. >> Source/WebCore/html/RangeInputType.cpp:212 >> + container->appendChild(TrackGoalElement::create(document), ec); > > This looks good for now, but with CSS3 calc, should we be able to get rid of the TrackGoalElement? > > left: calc(100% - 10px); It sounds cool. But what we want is (<parent width> - <thumb width>) * percentage. This example looks <parent width> * percentage - <thumb width>.

Created attachment 96710 [details] Patch

Comment on attachment 96710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96710&action=review > LayoutTests/ChangeLog:45 > + * platform/mac/media/media-document-audio-repaint-expected.png: I'm not sure why the vertical position of the media control is changed and -expected.txt is not changed.

(In reply to comment #14) > Created an attachment (id=96710) [details] > Patch The patch is more complex than my expectation. That's because I have no idea to realize the following rules by pure CSS: * The flex box container must occupy the whole area of the <input>. It must not overflow from the <input> * The height of the flex box container and the <input> must be same as the height of the thumb if the <input> has 'auto' height. * We can't change the 'display' property of the <input> from 'inline-block' to '-webkit-box'. RenderTextControlSingleLine has similar rules.

Comment on attachment 96710 [details] Patch Attachment 96710 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8827254 New failing tests: fast/forms/input-appearance-range.html media/controls-strict.html fast/forms/thumbslider-no-parent-slider.html media/controls-styling.html fast/forms/slider-onchange-event.html media/video-display-toggle.html media/audio-repaint.html fast/forms/slider-thumb-stylability.html media/audio-controls-rendering.html fast/forms/slider-thumb-shared-style.html fast/forms/slider-padding.html media/video-controls-rendering.html media/controls-without-preload.html fast/forms/box-shadow-override.html fast/forms/range-thumb-height-percentage.html fast/forms/input-appearance-height.html fast/forms/slider-mouse-events.html fast/repaint/slider-thumb-drag-release.html fast/forms/slider-delete-while-dragging-thumb.html media/controls-after-reload.html

Created attachment 96713 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 96710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96710&action=review This looks gloriously awesome. The drawback of this conversion is fairly painful though: we are adding 2 more DOM elements for each slider. That seems super-sad. I think we should file bugs now and aim to fix this at some point. It's bizarre that the little dimples on the slider thumb are gone. Why, I wonder? Is this acceptable by Mac HIG? > Source/WebCore/html/RangeInputType.cpp:156 > + if (event->button() != LeftButton || !targetNode || (targetNode != element() && !targetNode->isDescendantOf(element()->shadowRoot()))) Interesting. What else could a target node be in this case? > Source/WebCore/html/shadow/SliderThumbElement.cpp:163 > + // Percentage 'top' for the thumb doesn't work if the parent style has > + // no concrete height. > + Node* track = node()->firstChild(); > + if (track && track->renderer()->isBox()) { > + RenderBox* trackBox = track->renderBox(); > + trackBox->style()->setHeight(Length(trackBox->height() - trackBox->borderAndPaddingHeight(), Fixed)); > + } I feel like we should be solving this problem with CSS. I have no concrete suggestions though... > Source/WebCore/html/shadow/SliderThumbElement.cpp:362 > + return new (arena) RenderSliderContainer(this); I think this guy needs a FIXME an a bug filed, blocking bug 62096. And RenderSliderThumb too. We should aim to remove all custom renderer objects.

Created attachment 96935 [details] Patch 2 Add comments and Chromium expectation.

(In reply to comment #15) > (From update of attachment 96710 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96710&action=review > > > LayoutTests/ChangeLog:45 > > + * platform/mac/media/media-document-audio-repaint-expected.png: > > I'm not sure why the vertical position of the media control is changed and -expected.txt is not changed. I confirmed the vertical position was different even without the patch. So I removed the -expected.png from the patch.

Comment on attachment 96710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96710&action=review > It's bizarre that the little dimples on the slider thumb are gone. Why, I wonder? Is this acceptable by Mac HIG? This patch won't change any appearance. The patch includes some -expected.png updates because the current expectations are out of date. I think there should not be the dimples. The dimples are very short slider tracks drawn by NSSliderCell in RenderThemeMac::paintSliderThumb(). I don't know what change removed the dimples. Safari 5 and Chrome 11 stable have the dimples, but Chrome 13 dev has no dimples. >> Source/WebCore/html/RangeInputType.cpp:156 >> + if (event->button() != LeftButton || !targetNode || (targetNode != element() && !targetNode->isDescendantOf(element()->shadowRoot()))) > > Interesting. What else could a target node be in this case? Other shadow nodes in the slider. We need this change in order to support clicking on the track but not on the thumb. >> Source/WebCore/html/shadow/SliderThumbElement.cpp:362 >> + return new (arena) RenderSliderContainer(this); > > I think this guy needs a FIXME an a bug filed, blocking bug 62096. And RenderSliderThumb too. We should aim to remove all custom renderer objects. I filed Bug 62535 and added a FIXME comment.

Comment on attachment 96935 [details] Patch 2 Attachment 96935 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8837075 New failing tests: media/video-no-audio.html fast/multicol/client-rects.html media/video-volume-slider.html media/video-zoom-controls.html fast/dom/HTMLInputElement/input-slider-update.html fast/layers/video-layer.html media/video-empty-source.html

Created attachment 96940 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 96941 [details] Patch 3 More Chromium failures.

Comment on attachment 96941 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=96941&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:135 > + virtual void layout() Can you move the body out of the declaration here?

Comment on attachment 96941 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=96941&action=review >> Source/WebCore/html/shadow/SliderThumbElement.cpp:135 >> + virtual void layout() > > Can you move the body out of the declaration here? Yes, I'll do.

Committed r88757: <http://trac.webkit.org/changeset/88757>

(In reply to comment #28) > Committed r88757: <http://trac.webkit.org/changeset/88757> This made two regressions (probably only on Chromium). - Some event tests for sliders fail. - Media control slider thumb position is wrong. Unfortunately it's too late to revert r88757 because I landed many rebaselines. I'll investigate them soon.

media/video-controls-rendering.html is broken on the GPU waterfall. Can you land appropriate changes to test_expectations.txt while you investigate?

Landed Qt rebaseline in http://trac.webkit.org/changeset/88879 per http://build.webkit.org/results/Qt%20Linux%20Release/r88869%20(34130)/results.html. fast/forms/slider-onchange-event.html has two extra lines in actual result so it might be legitimately failing.