input[type=range] as a flex item renders thumb at wrong position

• Component: CSS
• Status: RESOLVED FIXED

[Steve Orvell]

Created attachment 167562 [details]
reduction showing incorrect rendering of input type=range when it's a flexitem

When an input type=range is a flexitem, the thumb is rendered at an incorrect position.  The slider is displayed at the center of the rendered box while the thumb is displayed at the top (see reduction).

[Tony Chang]

Created attachment 168501 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168501 [details]
Patch

Attachment 168501 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14288165

New failing tests:
fast/forms/datalist/input-appearance-range-with-datalist-zoomed.html

[Kent Tamura]

Comment on attachment 168501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168501&action=review

Very nice change.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:181
> +    // These should always exist, unless someone mutates the shadow DOM (e.g., in the inspector).
> +    RenderBox* track = firstChildBox();
> +    if (!track)
> +        return;
> +    RenderBox* thumb = track->firstChildBox();
> +    if (!thumb)
> +        return;

Is this safe even if the container has :before or :after ?

[Tony Chang]

Created attachment 168739 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> (From update of attachment 168501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168501&action=review
> 
> Very nice change.
> 
> > Source/WebCore/html/shadow/SliderThumbElement.cpp:181
> > +    // These should always exist, unless someone mutates the shadow DOM (e.g., in the inspector).
> > +    RenderBox* track = firstChildBox();
> > +    if (!track)
> > +        return;
> > +    RenderBox* thumb = track->firstChildBox();
> > +    if (!thumb)
> > +        return;
> 
> Is this safe even if the container has :before or :after ?

Good question.  It's safe (doesn't crash), but it does grab the wrong RenderObject and causes the generated content to move instead of the thumb.  I went back to using the DOM methods then getting the renderer from that.

Here's the code to create generated content before.
input[type=range]::-webkit-slider-runnable-track:before {
  content: "hello";
}

[Ojan Vafai]

Comment on attachment 168739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168739&action=review

Big improvement!

> Source/WebCore/ChangeLog:39
> +        * rendering/RenderSlider.h: Fix indention.

s/indention/indentation

> Source/WebCore/html/shadow/SliderThumbElement.cpp:178
> +    RenderBox* thumb = toRenderBox(input->sliderThumbElement()->renderer());

sliderThumbElement can exist without having a renderer, e.g. if in the inspector you point to it with a global variable and then delete it. Not sure if this is possible in practice since it's in the shadow DOM.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:186
> +        thumbLocation.setY(thumbLocation.y() + track->contentHeight() - offset);

FWIW, I think for consistency sake, we should support RTL on vertical sliders as well to have them start at the top. You obviously don't need to fix this, but maybe add a FIXME? In an ideal world, we'd swap it so that RTL vertical sliders start at the bottom and LTR ones start at the top.

> LayoutTests/platform/chromium/TestExpectations:4012
> +webkit.org/b/98666 [ Win Mac ] fast/forms/datalist/input-appearance-range-with-datalist.html [ Failure ]

Won't this be ImageOnlyFailure?

[Tony Chang]

Created attachment 168753 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168753 [details]
Patch for landing

Rejecting attachment 168753 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Remove WTF::fastDeleteAllValues().

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14300789

[Eric Seidel (no email)]

Attachment 168753 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium-linux/fast/forms/datalist/input-appearance-range-with-datalist-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Committed r131367: <http://trac.webkit.org/changeset/131367>

[Tony Chang]

Reverted r131367 for reason:

crashes on Apple Mac

Committed r131378: <http://trac.webkit.org/changeset/131378>

[Tony Chang]

Committed r131497: <http://trac.webkit.org/changeset/131497>

[Roger Fong]

The Windows built bots bots are sad.
Looks like an exports issue:
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/56312

[Tony Chang]

(In reply to comment #13)
> The Windows built bots bots are sad.
> Looks like an exports issue:
> http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/56312

Trying to fix . . .

[Zoltan Arvai]

On QT fast/forms/range/input-appearance-range-rtl.html reftest fails after r131497. The vertical slider moved some pixel to the right.

http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r131499%20%2843908%29/results.html

[Zan Dobersek]

(In reply to comment #15)
> On QT fast/forms/range/input-appearance-range-rtl.html reftest fails after r131497. The vertical slider moved some pixel to the right.
> 
> http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r131499%20%2843908%29/results.html

Same on the GTK port. A bunch of tests were also added to pretty much every port's TestExpectations file. Should they be rebaselined?

[Tony Chang]

(In reply to comment #16)
> (In reply to comment #15)
> > On QT fast/forms/range/input-appearance-range-rtl.html reftest fails after r131497. The vertical slider moved some pixel to the right.
> > 
> > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r131499%20%2843908%29/results.html
> 
> Same on the GTK port. A bunch of tests were also added to pretty much every port's TestExpectations file. Should they be rebaselined?

I plan on rebaselining all tests on all ports today.  Sorry for the breakage of fast/forms/range/input-appearance-range-rtl.html.  I thought I had added the tests last night to TestExpectations.

[Zan Dobersek]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > On QT fast/forms/range/input-appearance-range-rtl.html reftest fails after r131497. The vertical slider moved some pixel to the right.
> > > 
> > > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r131499%20%2843908%29/results.html
> > 
> > Same on the GTK port. A bunch of tests were also added to pretty much every port's TestExpectations file. Should they be rebaselined?
> 
> I plan on rebaselining all tests on all ports today.  Sorry for the breakage of fast/forms/range/input-appearance-range-rtl.html.  I thought I had added the tests last night to TestExpectations.

You did, but it's apparently a reftest so the expectation should be ImageOnlyFailure instead of simply Failure.

Thanks for taking care of rebaselining.