Bug 98666

Created attachment 168501 [details] Patch

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

Created attachment 168739 [details] Patch

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

Created attachment 168753 [details] Patch for landing

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

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.

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

Reverted r131367 for reason: crashes on Apple Mac Committed r131378: <http://trac.webkit.org/changeset/131378>

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

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

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

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

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

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

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