Bug 52920 - Use CSS machinery to position slider thumb.
Summary: Use CSS machinery to position slider thumb.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 61621 62195 62196 62207 62208
Blocks: 59866 62096 62535 60353 62683 62685
  Show dependency treegraph
 
Reported: 2011-01-21 14:07 PST by Dimitri Glazkov (Google)
Modified: 2011-06-15 07:26 PDT (History)
9 users (show)

See Also:


Attachments
=WIP: Mostly working, moar cleening needed. (7.58 KB, patch)
2011-01-21 14:08 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP by tkent (45.24 KB, patch)
2011-05-30 04:37 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Try (37.45 KB, patch)
2011-06-09 01:45 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (534.04 KB, patch)
2011-06-10 01:13 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (2.28 MB, application/zip)
2011-06-10 01:32 PDT, WebKit Review Bot
no flags Details
Patch 2 (501.67 KB, patch)
2011-06-13 02:11 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.60 MB, application/zip)
2011-06-13 02:37 PDT, WebKit Review Bot
no flags Details
Patch 3 (502.04 KB, patch)
2011-06-13 02:51 PDT, Kent Tamura
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-01-21 14:07:24 PST
Use CSS machinery to position slider thumb.
Comment 1 Dimitri Glazkov (Google) 2011-01-21 14:08:07 PST
Created attachment 79790 [details]
=WIP: Mostly working, moar cleening needed.
Comment 2 Dimitri Glazkov (Google) 2011-01-24 15:34:56 PST
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.
Comment 3 Dimitri Glazkov (Google) 2011-04-30 09:27:32 PDT
This is useful if we want to animate the slider thumb.
Comment 4 Kent Tamura 2011-05-27 03:33:01 PDT
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().
Comment 5 Dimitri Glazkov (Google) 2011-05-27 08:55:18 PDT
(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.
Comment 6 Kent Tamura 2011-05-29 23:26:57 PDT
(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?
Comment 7 Kent Tamura 2011-05-30 03:21:56 PDT
(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.
Comment 8 Kent Tamura 2011-05-30 04:37:54 PDT
Created attachment 95336 [details]
WIP by tkent
Comment 9 Dimitri Glazkov (Google) 2011-05-31 09:11:24 PDT
Comment on attachment 95336 [details]
WIP by tkent

I like where this is going.
Comment 10 Kent Tamura 2011-06-09 01:45:36 PDT
Created attachment 96560 [details]
Try

This is an almost-complete patch without ChangeLog and test result updates
Comment 11 Kent Tamura 2011-06-09 02:55:28 PDT
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 12 Dimitri Glazkov (Google) 2011-06-09 09:07:03 PDT
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 13 Kent Tamura 2011-06-09 23:58:48 PDT
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>.
Comment 14 Kent Tamura 2011-06-10 01:13:22 PDT
Created attachment 96710 [details]
Patch
Comment 15 Kent Tamura 2011-06-10 01:14:59 PDT
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.
Comment 16 Kent Tamura 2011-06-10 01:29:38 PDT
(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 17 WebKit Review Bot 2011-06-10 01:32:49 PDT
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
Comment 18 WebKit Review Bot 2011-06-10 01:32:56 PDT
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 19 Dimitri Glazkov (Google) 2011-06-10 09:50:31 PDT
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.
Comment 20 Kent Tamura 2011-06-13 02:11:05 PDT
Created attachment 96935 [details]
Patch 2

Add comments and Chromium expectation.
Comment 21 Kent Tamura 2011-06-13 02:13:01 PDT
(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 22 Kent Tamura 2011-06-13 02:23:35 PDT
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 23 WebKit Review Bot 2011-06-13 02:37:22 PDT
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
Comment 24 WebKit Review Bot 2011-06-13 02:37:27 PDT
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
Comment 25 Kent Tamura 2011-06-13 02:51:04 PDT
Created attachment 96941 [details]
Patch 3

More Chromium failures.
Comment 26 Dimitri Glazkov (Google) 2011-06-13 09:26:38 PDT
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 27 Kent Tamura 2011-06-13 20:31:11 PDT
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.
Comment 28 Kent Tamura 2011-06-13 20:46:46 PDT
Committed r88757: <http://trac.webkit.org/changeset/88757>
Comment 29 Kent Tamura 2011-06-14 03:26:34 PDT
(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.
Comment 30 James Robinson 2011-06-14 11:51:00 PDT
media/video-controls-rendering.html is broken on the GPU waterfall.  Can you land appropriate changes to test_expectations.txt while you investigate?
Comment 31 Ryosuke Niwa 2011-06-14 17:13:17 PDT
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.