102817 2012-11-20 09:09:42 -0800 Reset the slider thumb location before every layout of the slider container 2012-11-21 10:16:04 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 102352 1 cgarcia webkit-unassigned mifenton ojan tkent tony webkit.review.bot oldest_to_newest 771911 0 cgarcia 2012-11-20 09:09:42 -0800 The location of the slider thumb is set by the slider container assuming the render slider has been laid out. When this happens, the location of the slider thumb is reset during the layout ignoring any previous location set. If the slider thumb is not laid out, the previous value is added to the new one by the slider container. 771914 1 175231 cgarcia 2012-11-20 09:14:28 -0800 Created attachment 175231 Patch 771967 2 175231 tony 2012-11-20 10:17:33 -0800 Comment on attachment 175231 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > + // Reset the thumb location before layout. > + thumb->setLocation(LayoutPoint()); > + } When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position? 771989 3 cgarcia 2012-11-20 10:35:53 -0800 (In reply to comment #2) > (From update of attachment 175231 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review Thanks for reviewing. > > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > > + // Reset the thumb location before layout. > > + thumb->setLocation(LayoutPoint()); > > + } > > When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position? Yes, I think so, I'll try 772083 4 175231 ap 2012-11-20 12:21:11 -0800 Comment on attachment 175231 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > Source/WebCore/ChangeLog:13 > + ignoring any previous location set. If the slider thumb is not > + laid out, the previous value is added to the new one by the slider > + container. This sounds like it should be observable testable. Is it not? 772123 5 ojan 2012-11-20 13:37:05 -0800 It will be observable once https://bugs.webkit.org/show_bug.cgi?id=102352 is fixed. It's already covered by existing tests. 772667 6 cgarcia 2012-11-21 02:14:49 -0800 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 175231 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > > Thanks for reviewing. > > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > > > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > > > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > > > + // Reset the thumb location before layout. > > > + thumb->setLocation(LayoutPoint()); > > > + } > > > > When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position? > > Yes, I think so, I'll try The initial position of the thumb slider is set by the parent container in RenderBlock::layoutBlockChild() and it depends on the padding, border and margin. I don't know if the thumb can be affected by those, because after the layoutBlockChild() the thumb position is always 0. In case of being greater than 0 I guess it makes sense to add the offset, but I guess we should also use the current thumb position when the container is not laid out. 772837 7 cgarcia 2012-11-21 05:42:30 -0800 Committed r135388: <http://trac.webkit.org/changeset/135388> 772840 8 cgarcia 2012-11-21 05:45:11 -0800 I landed the patch without any modification in the end to be extra sure it doesn't break anything. 773080 9 tony 2012-11-21 10:16:04 -0800 (In reply to comment #6) > The initial position of the thumb slider is set by the parent container in RenderBlock::layoutBlockChild() and it depends on the padding, border and margin. I don't know if the thumb can be affected by those, because after the layoutBlockChild() the thumb position is always 0. In case of being greater than 0 I guess it makes sense to add the offset, but I guess we should also use the current thumb position when the container is not laid out. If there was a padding/border/margin on the thumb, after bug 102352 lands, we're no longer going to be applying them on a relayout since we will reset the location to 0, 0 then not call layout on the thumb. I would probably call thumbLocation.setX directly on the new position since it's less code, but it doesn't really matter. Conceptually, I don't think it makes sense for web authors to be able to use CSS to control the position of the thumb. 175231 2012-11-20 09:14:28 -0800 2012-11-20 12:21:11 -0800 Patch wk-slider-thumb.diff text/plain 2240 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA2YmI1ZTdjLi5mMWFjZmE4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTItMTEtMjAgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFJlc2V0IHRoZSBzbGlkZXIgdGh1bWIgbG9jYXRpb24gYmVmb3JlIGV2ZXJ5 IGxheW91dCBvZiB0aGUgc2xpZGVyIGNvbnRhaW5lcgorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyODE3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGxvY2F0aW9uIG9mIHRoZSBzbGlkZXIgdGh1bWIg aXMgc2V0IGJ5IHRoZSBzbGlkZXIgY29udGFpbmVyCisgICAgICAgIGFzc3VtaW5nIHRoZSBzbGlk ZXIgdGh1bWIgaGFzIGJlZW4gbGFpZCBvdXQuIFdoZW4gdGhpcyBoYXBwZW5zLAorICAgICAgICB0 aGUgbG9jYXRpb24gb2YgdGhlIHNsaWRlciB0aHVtYiBpcyByZXNldCBkdXJpbmcgdGhlIGxheW91 dAorICAgICAgICBpZ25vcmluZyBhbnkgcHJldmlvdXMgbG9jYXRpb24gc2V0LiBJZiB0aGUgc2xp ZGVyIHRodW1iIGlzIG5vdAorICAgICAgICBsYWlkIG91dCwgdGhlIHByZXZpb3VzIHZhbHVlIGlz IGFkZGVkIHRvIHRoZSBuZXcgb25lIGJ5IHRoZSBzbGlkZXIKKyAgICAgICAgY29udGFpbmVyLgor CisgICAgICAgICogaHRtbC9zaGFkb3cvU2xpZGVyVGh1bWJFbGVtZW50LmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbmRlclNsaWRlckNvbnRhaW5lcjo6bGF5b3V0KTogUmVzZXQgdGhlIGxvY2F0 aW9uIG9mCisgICAgICAgIHRoZSBzbGlkZXIgdGh1bWIgYmVmb3JlIGNhbGxpbmcgUmVuZGVyRmxl eGlibGVCb3g6OmxheW91dCgpLgorCiAyMDEyLTExLTE5ICBFcmlrIEFydmlkc3NvbiAgPGFydkBj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmVtb3ZlIGhpc3RvcmljYWwgZW51bXMgZnJvbSBFeGNl cHRpb25Db2RlLmgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvc2hhZG93L1NsaWRl clRodW1iRWxlbWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL3NoYWRvdy9TbGlkZXJUaHVt YkVsZW1lbnQuY3BwCmluZGV4IDhkYzBmZDIuLmU2Mzk1NGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL2h0bWwvc2hhZG93L1NsaWRlclRodW1iRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvaHRtbC9zaGFkb3cvU2xpZGVyVGh1bWJFbGVtZW50LmNwcApAQCAtMTc3LDEzICsxNzcs MTkgQEAgdm9pZCBSZW5kZXJTbGlkZXJDb250YWluZXI6OmxheW91dCgpCiAgICAgICAgIHN0eWxl KCktPnNldERpcmVjdGlvbihMVFIpOwogICAgIH0KIAorICAgIFJlbmRlckJveCogdGh1bWIgPSAw OworICAgIGlmIChpbnB1dC0+c2xpZGVyVGh1bWJFbGVtZW50KCkgJiYgaW5wdXQtPnNsaWRlclRo dW1iRWxlbWVudCgpLT5yZW5kZXJlcigpKSB7CisgICAgICAgIHRodW1iID0gdG9SZW5kZXJCb3go aW5wdXQtPnNsaWRlclRodW1iRWxlbWVudCgpLT5yZW5kZXJlcigpKTsKKyAgICAgICAgLy8gUmVz ZXQgdGhlIHRodW1iIGxvY2F0aW9uIGJlZm9yZSBsYXlvdXQuCisgICAgICAgIHRodW1iLT5zZXRM b2NhdGlvbihMYXlvdXRQb2ludCgpKTsKKyAgICB9CisKICAgICBSZW5kZXJGbGV4aWJsZUJveDo6 bGF5b3V0KCk7CiAKICAgICBzdHlsZSgpLT5zZXREaXJlY3Rpb24ob2xkVGV4dERpcmVjdGlvbik7 CiAgICAgLy8gVGhlc2Ugc2hvdWxkIGFsd2F5cyBleGlzdCwgdW5sZXNzIHNvbWVvbmUgbXV0YXRl cyB0aGUgc2hhZG93IERPTSAoZS5nLiwgaW4gdGhlIGluc3BlY3RvcikuCi0gICAgaWYgKCFpbnB1 dC0+c2xpZGVyVGh1bWJFbGVtZW50KCkgfHwgIWlucHV0LT5zbGlkZXJUaHVtYkVsZW1lbnQoKS0+ cmVuZGVyZXIoKSkKKyAgICBpZiAoIXRodW1iKQogICAgICAgICByZXR1cm47Ci0gICAgUmVuZGVy Qm94KiB0aHVtYiA9IHRvUmVuZGVyQm94KGlucHV0LT5zbGlkZXJUaHVtYkVsZW1lbnQoKS0+cmVu ZGVyZXIoKSk7CiAgICAgUmVuZGVyQm94KiB0cmFjayA9IHRvUmVuZGVyQm94KHRodW1iLT5wYXJl bnQoKSk7CiAKICAgICBkb3VibGUgcGVyY2VudGFnZU9mZnNldCA9IHNsaWRlclBvc2l0aW9uKGlu cHV0KS50b0RvdWJsZSgpOwo=