Bug 31283 - position:fixed and transform on same element breaks fixed behavior
Summary: position:fixed and transform on same element breaks fixed behavior
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 36652
  Show dependency treegraph
 
Reported: 2009-11-09 19:36 PST by Simon Fraser (smfr)
Modified: 2010-03-28 14:52 PDT (History)
4 users (show)

See Also:


Attachments
Testcase (574 bytes, text/html)
2009-11-09 19:36 PST, Simon Fraser (smfr)
no flags Details
Patch (9.78 KB, patch)
2010-01-05 12:39 PST, Simon Fraser (smfr)
mitz: review-
Details | Formatted Diff | Diff
Revised patch (9.88 KB, patch)
2010-01-05 17:11 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2010-01-12 15:23 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-11-09 19:36:33 PST
Created attachment 42844 [details]
Testcase

If an element has both position:fixed and a transform, then it behaves as if it's absolutely positioned.

I think the fixed positioning should take effect, and then the transform.
Comment 1 Simon Fraser (smfr) 2010-01-05 12:39:22 PST
Created attachment 45920 [details]
Patch
Comment 2 mitz 2010-01-05 14:47:59 PST
Comment on attachment 45920 [details]
Patch

> @@ -982,9 +984,14 @@ void RenderBox::mapAbsoluteToLocalPoint(bool fixed, bool useTransforms, Transfor
>      if (style()->position() == FixedPosition)
>          fixed = true;
>  
> +    bool isFixedPos = style()->position() == FixedPosition;
>      bool hasTransform = hasLayer() && layer()->transform();
> -    if (hasTransform)
> -        fixed = false;  // Elements with transforms act as a containing block for fixed position descendants
> +    if (hasTransform) {
> +        // If this box has a transform, it acts as a fixed position container for fixed descendants,
> +        // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
> +        fixed &= isFixedPos;
> +    } else
> +        fixed |= isFixedPos;

This doesn’t look right.
Comment 3 Simon Fraser (smfr) 2010-01-05 17:11:57 PST
Created attachment 45943 [details]
Revised patch
Comment 4 Eric Seidel (no email) 2010-01-12 14:57:32 PST
This patch is missing the binary data, which is why it's failing to apply correctly.  Note that "webkit-patch upload" or "webkit-patch post" or "webkit-patch post-commits" all know how to generate git diff --binary patches correctly so that the bots will be able to property process them. :)
Comment 5 Simon Fraser (smfr) 2010-01-12 15:23:02 PST
Created attachment 46404 [details]
Patch
Comment 6 mitz 2010-01-12 16:11:04 PST
Comment on attachment 46404 [details]
Patch

> +    EPosition position;
> +    if ((position = renderer()->style()->position()) == FixedPosition) {
> +        if (!ancestorLayer || ancestorLayer == renderer()->view()->layer()) {

Please just initialize position first, then do a single if statement:
if (position == FixedPosition && (!ancestorLayer || ancestorLayer == renderer()->view()->layer())) {
…

r=me
Comment 7 Simon Fraser (smfr) 2010-01-12 18:08:42 PST
http://trac.webkit.org/changeset/53173
Comment 8 Simon Hausmann 2010-03-26 08:43:32 PDT
cherry-pick-for-backport: <r53173>
Comment 9 Simon Hausmann 2010-03-26 08:44:04 PDT
Revision r53173 cherry-picked into qtwebkit-2.0 with commit 08dfb1202a63ed6d24be50484e3fbf8bbb9697a4