RESOLVED FIXED 50453
[Qt] QtWebKit does not seem to follow -webkit-slider-thumb 
https://bugs.webkit.org/show_bug.cgi?id=50453
Summary [Qt] QtWebKit does not seem to follow -webkit-slider-thumb
Benjamin Poulain
Reported 2010-12-03 04:47:07 PST
WebKit selectors include "-webkit-slider-thumb". This does not seem to do anything on QtWebKit.
Attachments
Patch (4.54 KB, patch)
2011-08-18 18:30 PDT, Tatiana Meshkova 		no flags
DetailsFormatted DiffDiff
Patch (6.70 KB, patch)
2011-08-23 13:55 PDT, Tatiana Meshkova 		no flags
DetailsFormatted DiffDiff
Patch (7.14 KB, patch)
2011-08-23 16:31 PDT, Tatiana Meshkova 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tatiana Meshkova
Comment 1 2011-08-18 18:30:08 PDT
Created attachment 104433 [details] Patch
Benjamin Poulain
Comment 2 2011-08-19 02:43:59 PDT
Comment on attachment 104433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104433&action=review > Source/WebCore/ChangeLog:8 > + Get rid of upcasting, that prevents related crashes. > + Allow to display SliderTrack and SliderThumb without <input type=range>. The changelog should explain the changes, it is not just a quick summary of what you have done. When the patch is trivial it is ok to have a simple changelog, but in general you need a good explanation. What is the crash you are fixing? Why haven't you included a layout test for it?
Tatiana Meshkova
Comment 3 2011-08-22 18:13:02 PDT
(In reply to comment #2) I've filed RenderThemeQt::adjustSliderThumbSize changes separately, bug 66732 > What is the crash you are fixing? Why haven't you included a layout test for it? The test case is: <html><body><p style="-webkit-appearance: slider-vertical; height: 15px"></p></body></html> Making layout test
Benjamin Poulain
Comment 4 2011-08-23 04:28:41 PDT
Comment on attachment 104433 [details] Patch > Making layout test -> Clearing review flag in the meantime.
Tatiana Meshkova
Comment 5 2011-08-23 13:55:52 PDT
Created attachment 104909 [details] Patch
Kenneth Rohde Christiansen
Comment 6 2011-08-23 14:23:10 PDT
Comment on attachment 104909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104909&action=review > LayoutTests/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). > + A short desccription would be nice > Source/WebCore/ChangeLog:11 > + Get rid of upcasting. Allow to display SliderTrack and SliderThumb > + without <input type=range>. > + > + Reviewed by NOBODY (OOPS!). > + I think it is more common to have the Reviewed by right after the bugzilla links. > Source/WebCore/platform/qt/RenderThemeQt.cpp:-922 > - RenderSlider* renderSlider = toRenderSlider(o); > - HTMLInputElement* input = renderSlider->node()->toInputElement(); > - IntRect thumbRect = sliderThumbElementOf(input)->getRect(); > - You seem to remove a lot of code without the ChangeLog telling why that was possible > Source/WebCore/platform/qt/RenderThemeQt.cpp:953 > + option.orientation = Qt::Vertical; So it is initialized to Horizontal by default? Maybe add a comment or just add the else clause
Tatiana Meshkova
Comment 7 2011-08-23 16:22:57 PDT
(In reply to comment #6) > > Source/WebCore/platform/qt/RenderThemeQt.cpp:953 > > + option.orientation = Qt::Vertical; > > So it is initialized to Horizontal by default? Maybe add a comment or just add the else clause yep, http://doc.qt.nokia.com/qstyleoptionslider.html#orientation-var Same code already used in RenderThemeQt twice with no comments. I don't think it's necessary to comment it now. I'll fix the rest.
Tatiana Meshkova
Comment 8 2011-08-23 16:31:39 PDT
Created attachment 104927 [details] Patch
WebKit Review Bot
Comment 9 2011-08-24 03:51:36 PDT
Comment on attachment 104927 [details] Patch Clearing flags on attachment: 104927 Committed r93702: <http://trac.webkit.org/changeset/93702>
WebKit Review Bot
Comment 10 2011-08-24 03:51:40 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.