85231 2012-04-30 14:15:22 -0700 Fixed position objects that are removed from the DOM don't kick off fixed position recalculation 2012-05-01 08:08:59 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 bdakin bdakin bdakin oldest_to_newest 612941 0 bdakin 2012-04-30 14:15:22 -0700 Fixed position objects that are removed from the DOM don't kick off fixed position recalculation. It is currently a limitation of tiled scrolling that we can't go into tiled scrolling mode when there are fixed position objects. But because of this bug, some pages will be stuck in non-tiled scrolling mode even though the fixed position objects have been destroyed. <rdar://problem/11297916> Patch forthcoming. 612948 1 139516 bdakin 2012-04-30 14:22:58 -0700 Created attachment 139516 Patch 613016 2 139516 darin 2012-04-30 15:37:26 -0700 Comment on attachment 139516 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139516&action=review Is there a way to write a regression test for this? Normally we would not want to land a code change without a test. > Source/WebCore/rendering/RenderBox.cpp:259 > + if (frameView && styleToUse) { I’d suggest putting the styleToUse if before even fetching the frameView. I’d write this: if (styleToUse) { if (RenderView* view = this->view()) { if (FrameView* frameView = view->frameView()) { > Source/WebCore/rendering/RenderBox.cpp:260 > + // If this renderer is owning renderer for the frameview's custom scrollbars, I think it should be “frame view” or “frameView”, not “frameview”. 613393 3 bdakin 2012-05-01 07:53:27 -0700 (In reply to comment #2) > (From update of attachment 139516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139516&action=review > > Is there a way to write a regression test for this? Normally we would not want to land a code change without a test. > We would need the ability via WebKitTestRunner to detect whether a page is in tiled scrolling mode, and I don't think we currently have the ability to do that. I will talk to Anders about it when he gets in. > > Source/WebCore/rendering/RenderBox.cpp:259 > > + if (frameView && styleToUse) { > > I’d suggest putting the styleToUse if before even fetching the frameView. I’d write this: > > if (styleToUse) { > if (RenderView* view = this->view()) { > if (FrameView* frameView = view->frameView()) { > Good call, will fix. > > Source/WebCore/rendering/RenderBox.cpp:260 > > + // If this renderer is owning renderer for the frameview's custom scrollbars, > > I think it should be “frame view” or “frameView”, not “frameview”. I think I like FrameView, which is not actually one of your suggestions, but I suspect you would like it too? 613404 4 bdakin 2012-05-01 08:08:59 -0700 I will still talk to Anders about a test for this, but in the meantime: http://trac.webkit.org/changeset/115725 139516 2012-04-30 14:22:58 -0700 2012-04-30 15:37:26 -0700 Patch for-review.txt text/plain 2202 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExNTY2NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA0LTMwICBCZXRoIERh a2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODUyMzEKKyAgICAgICAgRml4ZWQgcG9zaXRpb24gb2JqZWN0cyB0 aGF0IGFyZSByZW1vdmVkIGZyb20gdGhlIERPTSBkb24ndCBraWNrIG9mZiAKKyAgICAgICAgZml4 ZWQgcG9zaXRpb24gcmVjYWxjdWxhdGlvbgorICAgICAgICAtYW5kIGNvcnJlc3BvbmRpbmctCisg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS8xMTI5NzkxNj4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJCb3guY3BwOgorICAg ICAgICAoV2ViQ29yZTo6UmVuZGVyQm94Ojp3aWxsQmVEZXN0cm95ZWQpOgorCiAyMDEyLTA0LTI5 ICBTYW0gV2VpbmlnICA8c2FtQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmVtb3ZlIEJsb2JCdWls ZGVyCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQm94LmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQm94LmNwcAkocmV2aXNpb24g MTE1MzM3KQorKysgU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJveC5jcHAJKHdvcmtp bmcgY29weSkKQEAgLTI1MywxMCArMjUzLDE4IEBAIHZvaWQgUmVuZGVyQm94Ojp3aWxsQmVEZXN0 cm95ZWQoKQogICAgIGlmIChzdHlsZVRvVXNlICYmIChzdHlsZVRvVXNlLT5sb2dpY2FsSGVpZ2h0 KCkuaXNQZXJjZW50KCkgfHwgc3R5bGVUb1VzZS0+bG9naWNhbE1pbkhlaWdodCgpLmlzUGVyY2Vu dCgpIHx8IHN0eWxlVG9Vc2UtPmxvZ2ljYWxNYXhIZWlnaHQoKS5pc1BlcmNlbnQoKSkpCiAgICAg ICAgIFJlbmRlckJsb2NrOjpyZW1vdmVQZXJjZW50SGVpZ2h0RGVzY2VuZGFudCh0aGlzKTsKIAot ICAgIC8vIElmIHRoaXMgcmVuZGVyZXIgaXMgb3duaW5nIHJlbmRlcmVyIGZvciB0aGUgZnJhbWV2 aWV3J3MgY3VzdG9tIHNjcm9sbGJhcnMsCi0gICAgLy8gd2UgbmVlZCB0byBjbGVhciBpdCBmcm9t IHRoZSBzY3JvbGxiYXIuIFNlZSB3ZWJraXQgYnVnIDY0NzM3LgotICAgIGlmIChzdHlsZVRvVXNl ICYmIHN0eWxlVG9Vc2UtPmhhc1BzZXVkb1N0eWxlKFNDUk9MTEJBUikgJiYgZnJhbWUoKSAmJiBm cmFtZSgpLT52aWV3KCkpCi0gICAgICAgIGZyYW1lKCktPnZpZXcoKS0+Y2xlYXJPd25pbmdSZW5k ZXJlckZvckN1c3RvbVNjcm9sbGJhcnModGhpcyk7CisgICAgUmVuZGVyVmlldyogdmlldyA9IHRo aXMtPnZpZXcoKTsKKyAgICBGcmFtZVZpZXcqIGZyYW1lVmlldyA9IHZpZXcgPyB2aWV3LT5mcmFt ZVZpZXcoKSA6IDA7CisKKyAgICBpZiAoZnJhbWVWaWV3ICYmIHN0eWxlVG9Vc2UpIHsKKyAgICAg ICAgLy8gSWYgdGhpcyByZW5kZXJlciBpcyBvd25pbmcgcmVuZGVyZXIgZm9yIHRoZSBmcmFtZXZp ZXcncyBjdXN0b20gc2Nyb2xsYmFycywKKyAgICAgICAgLy8gd2UgbmVlZCB0byBjbGVhciBpdCBm cm9tIHRoZSBzY3JvbGxiYXIuIFNlZSB3ZWJraXQgYnVnIDY0NzM3LgorICAgICAgICBpZiAoc3R5 bGVUb1VzZS0+aGFzUHNldWRvU3R5bGUoU0NST0xMQkFSKSkKKyAgICAgICAgICAgIGZyYW1lVmll dy0+Y2xlYXJPd25pbmdSZW5kZXJlckZvckN1c3RvbVNjcm9sbGJhcnModGhpcyk7CisKKyAgICAg ICAgaWYgKHN0eWxlVG9Vc2UtPnBvc2l0aW9uKCkgPT0gRml4ZWRQb3NpdGlvbikKKyAgICAgICAg ICAgIGZyYW1lVmlldy0+cmVtb3ZlRml4ZWRPYmplY3QoKTsKKyAgICB9CiAKICAgICAvLyBJZiB0 aGUgZm9sbG93aW5nIGFzc2VydGlvbiBmYWlscywgbG9naWNhbEhlaWdodCgpL2xvZ2ljYWxNaW5I ZWlnaHQoKS8KICAgICAvLyBsb2dpY2FsTWF4SGVpZ2h0KCkgdmFsdWVzIGFyZSBjaGFuZ2VkIGZy b20gYSBwZXJjZW50IHZhbHVlIHRvIGEgbm9uLXBlcmNlbnQK