Bug 98666 - input[type=range] as a flex item renders thumb at wrong position
Summary: input[type=range] as a flex item renders thumb at wrong position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords: HasReduction
Depends on:
Blocks: 62048 102059
  Show dependency treegraph
 
Reported: 2012-10-08 10:50 PDT by Steve Orvell
Modified: 2012-11-13 01:25 PST (History)
17 users (show)

See Also:


Attachments
reduction showing incorrect rendering of input type=range when it's a flexitem (690 bytes, text/html)
2012-10-08 10:50 PDT, Steve Orvell
no flags Details
Patch (120.12 KB, patch)
2012-10-12 15:50 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (114.87 KB, patch)
2012-10-15 10:40 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (114.90 KB, patch)
2012-10-15 12:30 PDT, Tony Chang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Orvell 2012-10-08 10:50:10 PDT
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).
Comment 1 Tony Chang 2012-10-12 15:50:16 PDT
Created attachment 168501 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-12 23:42:51 PDT
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
Comment 3 Kent Tamura 2012-10-14 21:23:21 PDT
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 ?
Comment 4 Tony Chang 2012-10-15 10:40:47 PDT
Created attachment 168739 [details]
Patch
Comment 5 Tony Chang 2012-10-15 10:49:04 PDT
(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";
}
Comment 6 Ojan Vafai 2012-10-15 11:27:49 PDT
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?
Comment 7 Tony Chang 2012-10-15 12:30:50 PDT
Created attachment 168753 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-10-15 14:42:59 PDT
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
Comment 9 Eric Seidel (no email) 2012-10-15 14:47:57 PDT
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.
Comment 10 Tony Chang 2012-10-15 15:02:27 PDT
Committed r131367: <http://trac.webkit.org/changeset/131367>
Comment 11 Tony Chang 2012-10-15 16:17:43 PDT
Reverted r131367 for reason:

crashes on Apple Mac

Committed r131378: <http://trac.webkit.org/changeset/131378>
Comment 12 Tony Chang 2012-10-16 14:00:09 PDT
Committed r131497: <http://trac.webkit.org/changeset/131497>
Comment 13 Roger Fong 2012-10-16 14:26:59 PDT
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
Comment 14 Tony Chang 2012-10-16 14:30:26 PDT
(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 . . .
Comment 15 Zoltan Arvai 2012-10-17 00:50:53 PDT
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
Comment 16 Zan Dobersek 2012-10-17 03:14:30 PDT
(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?
Comment 17 Tony Chang 2012-10-17 10:20:29 PDT
(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.
Comment 18 Zan Dobersek 2012-10-17 11:02:03 PDT
(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.