When using page scaling, elements with position:fixed are not actually fixed but moves as the page scrolls. This change http://trac.webkit.org/changeset/78928 was trying to address the problem of position:fixed in page scale by moving the element proportionally to document scroll. However the change only works if scale factor is greater than 1.0. The observed behavior with the change is that: 1. When page scale factor is greater than 1.0 and page is scrolled down, element moves up. 2. When page scale factor is less than 1.0 and page is scrolled down, element moves down. Moreover elements fixed to the right and bottom are incorrectly positioned for all page scale factors.
Created attachment 108403 [details] Patch
Comment on attachment 108403 [details] Patch If you do this won't fixed elements crowd the view when you zoom in? They might end up obscuring the entire page.
(In reply to comment #0) > Moreover elements fixed to the right and bottom are incorrectly positioned for all page scale factors. Can you elaborate on this? Perhaps this bug should be split into two, one about the behavior with <1 scale factors and another about whatever issues exist with the behavior of >1 scale factors.
Comment on attachment 108403 [details] Patch Attachment 108403 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9813181 New failing tests: fast/block/positioning/vertical-rl/fixed-positioning.html
(In reply to comment #3) > (In reply to comment #0) > > > Moreover elements fixed to the right and bottom are incorrectly positioned for all page scale factors. > > Can you elaborate on this? If element is fixed to the right then: 1. If scale factor < 1, element moves to the right as scrolling to the right and soon go out of sight. 2. If scale factor > 1, element is not visible all the time. The same behavior happens when it's fixed to the bottom, element moves downwards as you scroll down. > Perhaps this bug should be split into two, one about the behavior with <1 scale factors and another about whatever issues exist with the behavior of >1 scale factors. The patch submitted fixes the element to the visible content rectangle so there's no distinction between scale factor > 1 or < 1. This should also work when there's no page scale, i.e. 1.
Created attachment 108470 [details] Patch
(In reply to comment #6) > Created an attachment (id=108470) [details] > Patch There was one test broken with the previous patch. I fixed it in the latest one.
(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=108470) [details] [details] > > Patch > > There was one test broken with the previous patch. I fixed it in the latest one. This is awesome. Thanks for this! I had a similar fix locally in scrollXForFixedPosition/scrollYForFixedPosition but I hadn't gotten around to doing extensive layout testing of it yet. I'm going to try your patch on a few major sites with fixed position elements, and I'll let you know how it looks. Thanks!
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Created an attachment (id=108470) [details] [details] [details] > > > Patch > > > > There was one test broken with the previous patch. I fixed it in the latest one. > > This is awesome. Thanks for this! I had a similar fix locally in scrollXForFixedPosition/scrollYForFixedPosition but I hadn't gotten around to doing extensive layout testing of it yet. I'm going to try your patch on a few major sites with fixed position elements, and I'll let you know how it looks. Thanks! There's one issue not addressed in this patch (that I plan to do it separately): that calling scalePage() to the frame doesn't cause a re-layout. This is mostly correct but for position:fixed to the right and bottom the visible content rectangle is changed and there needs to be a movement layout for *only* position:fixed position.
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #0) > > > > > Moreover elements fixed to the right and bottom are incorrectly positioned for all page scale factors. > > > > Can you elaborate on this? > > If element is fixed to the right then: > 1. If scale factor < 1, element moves to the right as scrolling to the right and soon go out of sight. > 2. If scale factor > 1, element is not visible all the time. > > The same behavior happens when it's fixed to the bottom, element moves downwards as you scroll down. That is not what I see. For example, I navigated to this URL in Safari 5.1 in OS X Lion: data:text/html,%3Cdiv%20style=%22position:%20fixed;%20right:%200;%20width:%20100px;%20height:%20100px;%20background-color:%20blue;%22%3E%3C/div%3E I used a pinch gesture to scale the page. Then I scrolled all the way to the right and the fixed element was visible. Similarly with an element fixed to the bottom. How are you testing this? > > Perhaps this bug should be split into two, one about the behavior with <1 scale factors and another about whatever issues exist with the behavior of >1 scale factors. > > The patch submitted fixes the element to the visible content rectangle so there's no distinction between scale factor > 1 or < 1. This should also work when there's no page scale, i.e. 1. Changes to less-than-1 scale factors do not impact Safari. Changes to greater-than-1 scale factors do. That is why I proposed dealing with them separately.
(In reply to comment #10) > (In reply to comment #5) > > (In reply to comment #3) > > > (In reply to comment #0) > > > > > > > Moreover elements fixed to the right and bottom are incorrectly positioned for all page scale factors. > > > > > > Can you elaborate on this? > > > > If element is fixed to the right then: > > 1. If scale factor < 1, element moves to the right as scrolling to the right and soon go out of sight. > > 2. If scale factor > 1, element is not visible all the time. > > > > The same behavior happens when it's fixed to the bottom, element moves downwards as you scroll down. > > That is not what I see. For example, I navigated to this URL in Safari 5.1 in OS X Lion: > > data:text/html,%3Cdiv%20style=%22position:%20fixed;%20right:%200;%20width:%20100px;%20height:%20100px;%20background-color:%20blue;%22%3E%3C/div%3E > > I used a pinch gesture to scale the page. Then I scrolled all the way to the right and the fixed element was visible. > > Similarly with an element fixed to the bottom. > > How are you testing this? You're right, when page scale > 1 it behaves like expected. I was testing it with the chromium test_shell, it's like a minimal browser for testing. > > > > Perhaps this bug should be split into two, one about the behavior with <1 scale factors and another about whatever issues exist with the behavior of >1 scale factors. > > > > The patch submitted fixes the element to the visible content rectangle so there's no distinction between scale factor > 1 or < 1. This should also work when there's no page scale, i.e. 1. > > Changes to less-than-1 scale factors do not impact Safari. Changes to greater-than-1 scale factors do. That is why I proposed dealing with them separately. If we handle page scale < 1.0 differently then yes Safari won't be affected. However I would like to have a different behavior for positioned:fixed on Chromium, i.e. always fix it to the viewport. What do you suggest we proceed from here?
(In reply to comment #11) > […] I would like to have a different behavior for positioned:fixed on Chromium, i.e. always fix it to the viewport. What do you suggest we proceed from here? If a different behavior is desired, then I suggest adding a WebCore Setting for how fixed elements behave on page scale, and letting WebKit ports change its value (or expose it as WebKit API in those ports).
(In reply to comment #12) > (In reply to comment #11) > > […] I would like to have a different behavior for positioned:fixed on Chromium, i.e. always fix it to the viewport. What do you suggest we proceed from here? > > If a different behavior is desired, then I suggest adding a WebCore Setting for how fixed elements behave on page scale, and letting WebKit ports change its value (or expose it as WebKit API in those ports).
I consider this a blocker to implementing the viewport meta tag on Chromium because a lot of pages are broken on Chromium without this change once scaling is enabled. Is there an update on this?
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > […] I would like to have a different behavior for positioned:fixed on Chromium, i.e. always fix it to the viewport. What do you suggest we proceed from here? > > > > If a different behavior is desired, then I suggest adding a WebCore Setting for how fixed elements behave on page scale, and letting WebKit ports change its value (or expose it as WebKit API in those ports). This is already implemented by RIM and the iOS 5. The iOS5 source has also recently been dumped as far as I heard. This is implemented using a render layer.
(In reply to comment #14) > I consider this a blocker to implementing the viewport meta tag on Chromium because a lot of pages are broken on Chromium without this change once scaling is enabled. Is there an update on this? I haven't got a chance to complete this actually. What's missing here is to allow settings to change behavior for position:fixed. The logic is there already, I'll go over this again next week, otherwise it's free for someone else to pick it up.
(In reply to comment #16) > (In reply to comment #14) > > I consider this a blocker to implementing the viewport meta tag on Chromium because a lot of pages are broken on Chromium without this change once scaling is enabled. Is there an update on this? > > I haven't got a chance to complete this actually. What's missing here is to allow settings to change behavior for position:fixed. The logic is there already, I'll go over this again next week, otherwise it's free for someone else to pick it up. Please email me your work in progress and I'll take over if you don't have the cycles. Thanks.
It's just the latest patch up there.
Created attachment 113006 [details] Patch
Comment on attachment 113006 [details] Patch Attachment 113006 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10242630
Comment on attachment 113006 [details] Patch Attachment 113006 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10242629
Created attachment 113050 [details] Patch
Comment on attachment 113050 [details] Patch Attachment 113050 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10242766
Comment on attachment 113050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113050&action=review > Source/WebCore/page/FrameView.h:198 > + bool fixedPositionElementsRelativeToFrame() const { return m_fixedPositionElementsRelativeToFrame; } The name feels like it's missing a verb. fixedPositionElementsAreRelativeToFrame? layoutFixedElementsRelativeToFrame? > Source/WebCore/rendering/RenderBox.cpp:2333 > + if (style() && style()->position() == FixedPosition && view() && view()->frame() && view()->frameView() && view()->frameView()->fixedPositionElementsRelativeToFrame()) > + return (view()->isHorizontalWritingMode() ? view()->frameView()->visibleWidth() : view()->frameView()->visibleHeight()) / view()->frame()->frameScaleFactor(); Ugh, very hard to read. Maybe stash the Frame and FrameView in locals? > Source/WebCore/rendering/RenderBox.cpp:2389 > + if (style() && style()->position() == FixedPosition && view() && view()->frame() && view()->frameView() && view()->frameView()->fixedPositionElementsRelativeToFrame()) > + return (view()->isHorizontalWritingMode() ? view()->frameView()->visibleHeight() : view()->frameView()->visibleWidth()) / view()->frame()->frameScaleFactor(); > + Ditto.
Created attachment 113065 [details] Patch
Comment on attachment 113050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113050&action=review Uploaded new patch. I still need to make sure it builds on all the major platforms. I'll let the review bots help me out with that. >> Source/WebCore/page/FrameView.h:198 >> + bool fixedPositionElementsRelativeToFrame() const { return m_fixedPositionElementsRelativeToFrame; } > > The name feels like it's missing a verb. fixedPositionElementsAreRelativeToFrame? layoutFixedElementsRelativeToFrame? Done. >> Source/WebCore/rendering/RenderBox.cpp:2333 >> + return (view()->isHorizontalWritingMode() ? view()->frameView()->visibleWidth() : view()->frameView()->visibleHeight()) / view()->frame()->frameScaleFactor(); > > Ugh, very hard to read. Maybe stash the Frame and FrameView in locals? Done. >> Source/WebCore/rendering/RenderBox.cpp:2389 >> + > > Ditto. Done.
Comment on attachment 113065 [details] Patch Attachment 113065 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10241785
Comment on attachment 113065 [details] Patch Attachment 113065 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10249047
Created attachment 113076 [details] Patch
(In reply to comment #29) > Created an attachment (id=113076) [details] > Patch This is probably still broken on gtk and mac builds. Making use of Review bots to get symbols. Please review, and I will fix all builds before committing.
Comment on attachment 113076 [details] Patch Attachment 113076 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10249064
Comment on attachment 113076 [details] Patch Attachment 113076 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10242905
Created attachment 113102 [details] Patch
Comment on attachment 113102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113102&action=review > Source/WebCore/page/FrameView.cpp:1431 > +void FrameView::setLayoutFixedElementsRelativeToFrame(bool layoutFixedElementsRelativeToFrame) This still doesn't read well. It should be setFixedElementsLaidOutRelativeToFrame (or ..Positioned...). Or you could put a "should" in front. > Source/WebCore/rendering/RenderBox.cpp:2335 > + if (style() && style()->position() == FixedPosition && frame && frameView && frameView->layoutFixedElementsRelativeToFrame()) > + return (view()->isHorizontalWritingMode() ? frameView->visibleWidth() : frameView->visibleHeight()) / frame->frameScaleFactor(); You can't just look for position:fixed here, because transforms act as fixed position containers. See discussion in bug 69796.
Created attachment 113120 [details] Patch
Comment on attachment 113102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113102&action=review >> Source/WebCore/page/FrameView.cpp:1431 >> +void FrameView::setLayoutFixedElementsRelativeToFrame(bool layoutFixedElementsRelativeToFrame) > > This still doesn't read well. It should be setFixedElementsLaidOutRelativeToFrame (or ..Positioned...). > > Or you could put a "should" in front. Added should. Thanks. >> Source/WebCore/rendering/RenderBox.cpp:2335 >> + return (view()->isHorizontalWritingMode() ? frameView->visibleWidth() : frameView->visibleHeight()) / frame->frameScaleFactor(); > > You can't just look for position:fixed here, because transforms act as fixed position containers. See discussion in bug 69796. Checking that the container is a RenderView now.
Comment on attachment 113120 [details] Patch Attachment 113120 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10250172
Created attachment 113486 [details] Patch
Comment on attachment 113486 [details] Patch You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position.
(In reply to comment #39) > (From update of attachment 113486 [details]) > You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position. I haven't had any luck producing a layout test for this as of yet: http://happyworm.com/temp/full-screen/Video-Play-button-all-video.htm doesn't seem to have any problems.
(In reply to comment #39) > (From update of attachment 113486 [details]) > You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position. I'm able to repo that particular code path usage (and the bug) if I used a replaced element (e.g. image) with a percentage height.
(In reply to comment #41) > (In reply to comment #39) > > (From update of attachment 113486 [details] [details]) > > You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position. > > I'm able to repo that particular code path usage (and the bug) if I used a replaced element (e.g. image) with a percentage height. Another problem probably also exists with perpendicular containing blocks. I'll look into that too.
(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #39) > > > (From update of attachment 113486 [details] [details] [details]) > > > You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position. > > > > I'm able to repo that particular code path usage (and the bug) if I used a replaced element (e.g. image) with a percentage height. > > Another problem probably also exists with perpendicular containing blocks. I'll look into that too. I was able to produce a layout test for this issue as well. I will be refreshing this patch shortly.
Comment on attachment 113486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113486&action=review > Source/WebCore/page/FrameView.h:415 > + bool m_shouldLayoutFixedElementsRelativeToFrame; Please group this with other bools to optimize packing. > Source/WebCore/rendering/RenderBox.cpp:2334 > + Frame* frame = view() ? view()->frame(): 0; > + FrameView* frameView = view() ? view()->frameView() : 0; > + if (style() && style()->position() == FixedPosition && container()->isRenderView() && frame && frameView && frameView->shouldLayoutFixedElementsRelativeToFrame()) There has to be a way to make this (duplicated twice) code less ugly. Maybe move it to an inline function?
Created attachment 117454 [details] Patch
Created attachment 117661 [details] Patch for landing
Comment on attachment 117661 [details] Patch for landing Attachment 117661 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10688437
Comment on attachment 117661 [details] Patch for landing gclient sync failed on cr-linux, retrying.
Comment on attachment 117661 [details] Patch for landing Clearing flags on attachment: 117661 Committed r101875: <http://trac.webkit.org/changeset/101875>
All reviewed patches have been landed. Closing bug.
(In reply to comment #50) > All reviewed patches have been landed. Closing bug. (In reply to comment #50) > All reviewed patches have been landed. Closing bug. The age old question just popped into my head: What happens to position:fixed elements inside iframes? This is a noop for subframes whether or not shouldLayoutFixedElementsRelativeToFrame is set. I want to move this to Page. :( I'll do this in a subsequent bug.