Bug 68617 - When page scaling is in use position:fixed has incorrect results
Summary: When page scaling is in use position:fixed has incorrect results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks: 68075 70559 71198
  Show dependency treegraph
 
Reported: 2011-09-22 07:01 PDT by Hin-Chung Lam
Modified: 2011-12-03 11:56 PST (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 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.
Comment 1 Hin-Chung Lam 2011-09-22 14:34:44 PDT
Created attachment 108403 [details]
Patch
Comment 2 Simon Fraser (smfr) 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.
Comment 3 mitz 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.
Comment 4 WebKit Review Bot 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
Comment 5 Hin-Chung Lam 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.
Comment 6 Hin-Chung Lam 2011-09-23 06:35:55 PDT
Created attachment 108470 [details]
Patch
Comment 7 Hin-Chung Lam 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.
Comment 8 Fady Samuel 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!
Comment 9 Hin-Chung Lam 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.
Comment 10 mitz 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.
Comment 11 Hin-Chung Lam 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?
Comment 12 mitz 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).
Comment 13 Fady Samuel 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).
Comment 14 Fady Samuel 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?
Comment 15 Kenneth Rohde Christiansen 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.
Comment 16 Hin-Chung Lam 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.
Comment 17 Fady Samuel 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.
Comment 18 Hin-Chung Lam 2011-10-28 08:27:18 PDT
It's just the latest patch up there.
Comment 19 Fady Samuel 2011-10-30 18:14:19 PDT
Created attachment 113006 [details]
Patch
Comment 20 Collabora GTK+ EWS bot 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
Comment 21 Daniel Bates 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
Comment 22 Fady Samuel 2011-10-31 07:56:04 PDT
Created attachment 113050 [details]
Patch
Comment 23 Gustavo Noronha (kov) 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
Comment 24 Simon Fraser (smfr) 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.
Comment 25 Fady Samuel 2011-10-31 10:04:37 PDT
Created attachment 113065 [details]
Patch
Comment 26 Fady Samuel 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.
Comment 27 Gustavo Noronha (kov) 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
Comment 28 Daniel Bates 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
Comment 29 Fady Samuel 2011-10-31 13:09:39 PDT
Created attachment 113076 [details]
Patch
Comment 30 Fady Samuel 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.
Comment 31 Gustavo Noronha (kov) 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
Comment 32 Daniel Bates 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
Comment 33 Fady Samuel 2011-10-31 16:24:36 PDT
Created attachment 113102 [details]
Patch
Comment 34 Simon Fraser (smfr) 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.
Comment 35 Fady Samuel 2011-10-31 18:30:22 PDT
Created attachment 113120 [details]
Patch
Comment 36 Fady Samuel 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.
Comment 37 Daniel Bates 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
Comment 38 Fady Samuel 2011-11-03 07:22:59 PDT
Created attachment 113486 [details]
Patch
Comment 39 Simon Fraser (smfr) 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.
Comment 40 Fady Samuel 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.
Comment 41 Fady Samuel 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.
Comment 42 Fady Samuel 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.
Comment 43 Fady Samuel 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.
Comment 44 Simon Fraser (smfr) 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?
Comment 45 Fady Samuel 2011-12-01 12:09:55 PST
Created attachment 117454 [details]
Patch
Comment 46 Fady Samuel 2011-12-02 11:39:52 PST
Created attachment 117661 [details]
Patch for landing
Comment 47 WebKit Review Bot 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
Comment 48 Fady Samuel 2011-12-02 14:04:44 PST
Comment on attachment 117661 [details]
Patch for landing

gclient sync failed on cr-linux, retrying.
Comment 49 WebKit Review Bot 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>
Comment 50 WebKit Review Bot 2011-12-02 16:05:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 51 Fady Samuel 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.