1125767 - css filter acts like position: relative + overflow: hidden

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Vladislav]

Attachment: 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.

Comment 1

[Vladislav]

Reproduces in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  That clipping looks weird.  roc, who's working on CSS filter stuff?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Centralize code into nsStylePosition::IsContainerForAbsAndFixedPosDescendants

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: CSS/SVG filters on an element should make it a containing block for fixed-pos and abs-pos descendants

Comment 8

[Mats Palmgren (inactive)]

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)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I think you're right about moving to nsIFrame. I'll do that.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Moving these functions to nsIFrame is quite complicated, as it turns out. I'd rather do that under a separate bug.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Centralize code into nsStylePosition::IsFixedPosContainingBlock

Also renames IsPositioned to IsAbsPosContainingBlock.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Clarify aContextFrame parameter to nsStylePosition::IsFixedPosContainingBlock and nsStylePosition::HasTransform and make nsCSSFrameConstructor actually honor it

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Passes tests on Linux64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4112a78af8d3

Comment 17

[Mats Palmgren (inactive)]

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);

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9951cca6d1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b229e43ce1
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd64e2d0cbee

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9951cca6d1f
https://hg.mozilla.org/mozilla-central/rev/78b229e43ce1
https://hg.mozilla.org/mozilla-central/rev/fd64e2d0cbee

Comment 20

[yair even or]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[yair even or]

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