Closed Bug 1125767 Opened 9 years ago Closed 9 years ago

css filter acts like position: relative + overflow: hidden

Categories

(Core :: Web Painting, defect)

35 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: me, Assigned: roc)

References

(Depends on 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

Attached image What happens
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.16 Safari/537.36

Steps to reproduce:

Place an absolutely-positioned block in block with "filter" css property (i.e. "filter: blur(0px);").

http://jsfiddle.net/lastw/auv3x2y9/


Actual results:

Block with filter acts like mask for absolute-positioned child (hides overflowed contents) — see attached screenshot


Expected results:

Block should not affect children in this way.
Reproduces in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Hmm.  That clipping looks weird.  roc, who's working on CSS filter stuff?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(roc)
Me I guess.

In that testcase, Chrome doesn't apply the filter to the abs-pos descendant. That also seems wrong to me.

This is quite closely related to a spec issue I raised recently: https://lists.w3.org/Archives/Public/public-fx/2015JanMar/0002.html. Followed up here:
https://lists.w3.org/Archives/Public/public-fx/2015JanMar/0029.html
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> In that testcase, Chrome doesn't apply the filter to the abs-pos descendant.
> That also seems wrong to me.

This is wrong, my test was incorrect. Chrome does filter the abs-pos descendant.
We decided to have 'filter' make the element a containing block for abs-pos and fixed-pos descendants:
http://www.w3.org/2015/02/10-fx-minutes.html#action02
Assignee: nobody → roc
Status: NEW → ASSIGNED
Comment on attachment 8574615 [details] [diff] [review]
Centralize code into nsStylePosition::IsContainerForAbsAndFixedPosDescendants

>layout/base/nsCSSFrameConstructor.cpp
>-  // When working with the -moz-transform property, we want to hook
>+  // When working with the transform and filter properties, we want to hook
>   // the abs-pos and fixed-pos lists together, since transformed
>   // elements are fixed-pos containing blocks.

s/since transformed/since such/ because it now includes more than
transformed elements.

> // This check is identical to nsStyleDisplay::IsPositioned except...

Please add a comment in IsPositioned that points out that this
block needs to be updated when making changes in that method.

It would be nice if we could share the code, say IsPositioned calls
IsPositionedInternal with "bool aWithAssert" true and this block
calls it with false.  But I guess that ripples through to your new
method and HasTransform(nsIFrame*) too, so it might not be worth it.


>+nsStylePosition::IsContainerForAbsAndFixedPosDescendants(...)
>+{
>+  return HasTransform(aContextFrame) || HasPerspectiveStyle();
>+}

Should we add "&& !aContextFrame->IsSVGText()" in this method too?

I'd prefer ContainingBlock instead of ContainerFor and Descendants
is perhaps redundant?
Also, with the change to IsPositioned() and the addition of 'filter' in
the next patch, that name becomes non-sensical.  Why should 'filter'
make IsPositioned() true?  Both methods are somewhat related so it
would be good if they had consistent naming.

I would suggest something like:
IsFixedPosContainingBlock() / IsAbsPosContainingBlock()
or
IsContainingBlockForFixedPos() / IsContainingBlockForAbsPos()

I get the intention of "AbsAndFixedPos" but it seems more confusing than
informative.  Looking at the call sites I think "Fixed" works better.
(Don't forget to update the commit message if you decide to rename
the methods.)

In IsPositioned() we could remove the last if-statement and just
return the condition instead.

Also, there's a perf win if you make it:
  ((IsAbsolutelyPositionedStyle() || IsRelativelyPositionedStyle()) &&
   !aContextFrame->IsSVGText()) || ...

(assuming your new method also checks that)
Attachment #8574615 - Flags: review?(mats) → review+
Attachment #8574616 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #8)
> Comment on attachment 8574615 [details] [diff] [review]
> >layout/base/nsCSSFrameConstructor.cpp
> >-  // When working with the -moz-transform property, we want to hook
> >+  // When working with the transform and filter properties, we want to hook
> >   // the abs-pos and fixed-pos lists together, since transformed
> >   // elements are fixed-pos containing blocks.
> 
> s/since transformed/since such/ because it now includes more than
> transformed elements.

OK

> > // This check is identical to nsStyleDisplay::IsPositioned except...
> 
> Please add a comment in IsPositioned that points out that this
> block needs to be updated when making changes in that method.

OK

> >+nsStylePosition::IsContainerForAbsAndFixedPosDescendants(...)
> >+{
> >+  return HasTransform(aContextFrame) || HasPerspectiveStyle();
> >+}
> 
> Should we add "&& !aContextFrame->IsSVGText()" in this method too?

OK

> I'd prefer ContainingBlock instead of ContainerFor and Descendants
> is perhaps redundant?
> Also, with the change to IsPositioned() and the addition of 'filter' in
> the next patch, that name becomes non-sensical.  Why should 'filter'
> make IsPositioned() true?  Both methods are somewhat related so it
> would be good if they had consistent naming.
> 
> I would suggest something like:
> IsFixedPosContainingBlock() / IsAbsPosContainingBlock()
> or
> IsContainingBlockForFixedPos() / IsContainingBlockForAbsPos()
> 
> I get the intention of "AbsAndFixedPos" but it seems more confusing than
> informative.  Looking at the call sites I think "Fixed" works better.
> (Don't forget to update the commit message if you decide to rename
> the methods.)

OK, I'll go with IsFixedPosContainingBlock() / IsAbsPosContainingBlock().

> In IsPositioned() we could remove the last if-statement and just
> return the condition instead.
> 
> Also, there's a perf win if you make it:
>   ((IsAbsolutelyPositionedStyle() || IsRelativelyPositionedStyle()) &&
>    !aContextFrame->IsSVGText()) || ...
> 
> (assuming your new method also checks that)

OK
David, the patches here make decisions about what is an abs-pos/fixed-pos containing block depend on nsStyleSVGReset::mFilters. Should we move mFilters into nsStylePosition?
Flags: needinfo?(dbaron)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> David, the patches here make decisions about what is an abs-pos/fixed-pos
> containing block depend on nsStyleSVGReset::mFilters. Should we move
> mFilters into nsStylePosition?

I don't think that's necessary.  Although both this and bug 1139283 make me wonder if some of these methods should move from style struct to frame (or maybe style context?).
Flags: needinfo?(dbaron)
Yeah, I think you're right about moving to nsIFrame. I'll do that.
Moving these functions to nsIFrame is quite complicated, as it turns out. I'd rather do that under a separate bug.
Also renames IsPositioned to IsAbsPosContainingBlock.
Attachment #8575586 - Flags: review?(mats)
Comment on attachment 8575586 [details] [diff] [review]
Centralize code into nsStylePosition::IsFixedPosContainingBlock

>layout/generic/nsContainerFrame.cpp
>   // See if the frame is being relatively positioned or absolutely
>   // positioned
>-  bool isPositioned = aFrame->IsPositioned();
>+  bool isPositioned = aFrame->IsAbsPosContaininingBlock();

That code comment could use an update or be removed.


>layout/style/nsStyleStructInlines.h
>+nsStylePosition::IsFixedPosContainingBlock(...
>+{
>+  return (HasTransform(aContextFrame) || HasPerspectiveStyle()) &&
>+      !aContextFrame->IsSVGText();

>+nsStyleDisplay::IsAbsPosContainingBlock(const nsIFrame* aContextFrame) const
>+  return (IsAbsolutelyPositionedStyle() ||
>+          IsRelativelyPositionedStyle() ||
>+          aContextFrame->StylePosition()->
>+              IsFixedPosContainingBlock(aContextFrame)) &&
>+      !aContextFrame->IsSVGText();

If IsFixedPosContainingBlock is what makes the first part true,
then !aContextFrame->IsSVGText() will be tested twice.  So I would
write that:

  return ((IsAbsolutelyPositionedStyle() ||
           IsRelativelyPositionedStyle()) &&
          !aContextFrame->IsSVGText()) ||
         aContextFrame->StylePosition()->
             IsFixedPosContainingBlock(aContextFrame);
Attachment #8575586 - Flags: review?(mats) → review+
Attachment #8575587 - Flags: review?(mats) → review+
https://hg.mozilla.org/mozilla-central/rev/b9951cca6d1f
https://hg.mozilla.org/mozilla-central/rev/78b229e43ce1
https://hg.mozilla.org/mozilla-central/rev/fd64e2d0cbee
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1187851
I'm sorry but this doesn't seems to be fixed..

I've made a demo:
http://jsfiddle.net/k312np94/1/

I've made a GIF showing the problem:
https://media.giphy.com/media/l0MYtBNgXAM4oNJjW/giphy.gif
What doesn't seem to be fixed?

This bug was about absolutely positioned children of a filtered element being clipped to the box of the filtered element.  Neither your testcase nor your gif shows this happening...

Your testcase _does_ show filtered content acting as a containing block for fixed-position descendants, but that's what the working group decided it should do.  See comment 5 for a link to the working group meeting minutes from last year.
Sorry, I thought this bug extends to my case as well. 

The behavior none-the-less a bizarre one and might trigger many open issues from other developers, since it is so not intuitive and difficult to work-around. I am amazed at this. I imagine this knowledge is known by very few...
Depends on: 1299754
Depends on: 1258421
No longer depends on: 1281068
Depends on: 1426919
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: