68617 – When page scaling is in use position:fixed has incorrect results

RESOLVED FIXED 68617

When page scaling is in use position:fixed has incorrect results

https://bugs.webkit.org/show_bug.cgi?id=68617

Summary When page scaling is in use position:fixed has incorrect results

Hin-Chung Lam

Reported 2011-09-22 07:01:41 PDT

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.

Attachments
Patch (15.46 KB, patch) 2011-09-22 14:34 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (15.72 KB, patch) 2011-09-23 06:35 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (20.64 KB, patch) 2011-10-30 18:14 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (24.87 KB, patch) 2011-10-31 07:56 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (21.38 KB, patch) 2011-10-31 10:04 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (22.86 KB, patch) 2011-10-31 13:09 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (24.72 KB, patch) 2011-10-31 16:24 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (24.95 KB, patch) 2011-10-31 18:30 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (24.93 KB, patch) 2011-11-03 07:22 PDT, Fady Samuel	no flags	Details Formatted Diff Diff
Patch (25.95 KB, patch) 2011-12-01 12:09 PST, Fady Samuel	no flags	Details Formatted Diff Diff
Patch for landing (25.89 KB, patch) 2011-12-02 11:39 PST, Fady Samuel	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Hin-Chung Lam

Comment 1 2011-09-22 14:34:44 PDT

Created attachment 108403 [details] Patch

Simon Fraser (smfr)

Comment 2 2011-09-22 16:05:13 PDT

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.

mitz

Comment 3 2011-09-22 16:07:50 PDT

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

WebKit Review Bot

Comment 4 2011-09-23 02:54:56 PDT

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

Hin-Chung Lam

Comment 5 2011-09-23 04:44:19 PDT

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

Hin-Chung Lam

Comment 6 2011-09-23 06:35:55 PDT

Created attachment 108470 [details] Patch

Hin-Chung Lam

Comment 7 2011-09-23 06:36:47 PDT

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

Fady Samuel

Comment 8 2011-09-23 08:00:15 PDT

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

Hin-Chung Lam

Comment 9 2011-09-23 08:05:36 PDT

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

mitz

Comment 10 2011-09-23 09:03:14 PDT

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

Hin-Chung Lam

Comment 11 2011-09-26 03:53:35 PDT

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

mitz

Comment 12 2011-09-26 10:36:28 PDT

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

Fady Samuel

Comment 13 2011-10-25 12:08:23 PDT

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

Fady Samuel

Comment 14 2011-10-25 12:09:23 PDT

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?

Kenneth Rohde Christiansen

Comment 15 2011-10-25 15:38:45 PDT

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

Hin-Chung Lam

Comment 16 2011-10-28 05:50:58 PDT

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

Fady Samuel

Comment 17 2011-10-28 08:17:01 PDT

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

Hin-Chung Lam

Comment 18 2011-10-28 08:27:18 PDT

It's just the latest patch up there.

Fady Samuel

Comment 19 2011-10-30 18:14:19 PDT

Created attachment 113006 [details] Patch

Collabora GTK+ EWS bot

Comment 20 2011-10-30 18:41:35 PDT

Comment on attachment 113006 [details] Patch Attachment 113006 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10242630

Daniel Bates

Comment 21 2011-10-30 18:55:44 PDT

Comment on attachment 113006 [details] Patch Attachment 113006 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10242629

Fady Samuel

Comment 22 2011-10-31 07:56:04 PDT

Created attachment 113050 [details] Patch

Gustavo Noronha (kov)

Comment 23 2011-10-31 08:09:28 PDT

Comment on attachment 113050 [details] Patch Attachment 113050 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10242766

Simon Fraser (smfr)

Comment 24 2011-10-31 08:54:47 PDT

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.

Fady Samuel

Comment 25 2011-10-31 10:04:37 PDT

Created attachment 113065 [details] Patch

Fady Samuel

Comment 26 2011-10-31 10:05:53 PDT

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.

Gustavo Noronha (kov)

Comment 27 2011-10-31 10:19:04 PDT

Comment on attachment 113065 [details] Patch Attachment 113065 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10241785

Daniel Bates

Comment 28 2011-10-31 12:57:43 PDT

Comment on attachment 113065 [details] Patch Attachment 113065 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10249047

Fady Samuel

Comment 29 2011-10-31 13:09:39 PDT

Created attachment 113076 [details] Patch

Fady Samuel

Comment 30 2011-10-31 13:11:09 PDT

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

Gustavo Noronha (kov)

Comment 31 2011-10-31 13:13:14 PDT

Comment on attachment 113076 [details] Patch Attachment 113076 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10249064

Daniel Bates

Comment 32 2011-10-31 14:50:37 PDT

Comment on attachment 113076 [details] Patch Attachment 113076 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10242905

Fady Samuel

Comment 33 2011-10-31 16:24:36 PDT

Created attachment 113102 [details] Patch

Simon Fraser (smfr)

Comment 34 2011-10-31 16:29:21 PDT

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.

Fady Samuel

Comment 35 2011-10-31 18:30:22 PDT

Created attachment 113120 [details] Patch

Fady Samuel

Comment 36 2011-10-31 18:34:20 PDT

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.

Daniel Bates

Comment 37 2011-11-01 01:34:24 PDT

Comment on attachment 113120 [details] Patch Attachment 113120 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10250172

Fady Samuel

Comment 38 2011-11-03 07:22:59 PDT

Created attachment 113486 [details] Patch

Simon Fraser (smfr)

Comment 39 2011-11-14 13:52:18 PST

Comment on attachment 113486 [details] Patch You also need to change availableLogicalHeightUsing() to deal with relative position inside percentage-height fixed position.

Fady Samuel

Comment 40 2011-11-16 15:38:47 PST

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

Fady Samuel

Comment 41 2011-11-30 22:54:08 PST

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

Fady Samuel

Comment 42 2011-11-30 22:58:58 PST

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

Fady Samuel

Comment 43 2011-12-01 09:20:52 PST

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

Simon Fraser (smfr)

Comment 44 2011-12-01 10:43:59 PST

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?

Fady Samuel

Comment 45 2011-12-01 12:09:55 PST

Created attachment 117454 [details] Patch

Fady Samuel

Comment 46 2011-12-02 11:39:52 PST

Created attachment 117661 [details] Patch for landing

WebKit Review Bot

Comment 47 2011-12-02 14:01:30 PST

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

Fady Samuel

Comment 48 2011-12-02 14:04:44 PST

Comment on attachment 117661 [details] Patch for landing gclient sync failed on cr-linux, retrying.

WebKit Review Bot

Comment 49 2011-12-02 16:05:05 PST

Comment on attachment 117661 [details] Patch for landing Clearing flags on attachment: 117661 Committed r101875: <http://trac.webkit.org/changeset/101875>

WebKit Review Bot

Comment 50 2011-12-02 16:05:14 PST

All reviewed patches have been landed. Closing bug.

Fady Samuel

Comment 51 2011-12-03 11:56:52 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Fady Samuel

Reported

2011-09-22 07:01 PDT

Modified

2011-12-03 11:56 PST History

CC List

18 users Show

URL

Keywords

Depends on

Blocks

68075 70559 71198

Dependencies

tree graph