Closed
Bug 1125767
Opened 10 years ago
Closed 10 years ago
css filter acts like position: relative + overflow: hidden
Categories
(Core :: Web Painting, defect)
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)
80.01 KB,
image/png
|
Details | |
3.35 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
26.92 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
5.89 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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
Comment 2•10 years ago
|
||
Hmm. That clipping looks weird. roc, who's working on CSS filter stuff?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(roc)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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 | ||
Comment 6•10 years ago
|
||
Attachment #8574615 -
Flags: review?(mats)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → roc
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8574616 -
Flags: review?(mats)
Comment 8•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8574616 -
Flags: review?(mats) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(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
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Yeah, I think you're right about moving to nsIFrame. I'll do that.
Assignee | ||
Comment 13•10 years ago
|
||
Moving these functions to nsIFrame is quite complicated, as it turns out. I'd rather do that under a separate bug.
Assignee | ||
Comment 14•10 years ago
|
||
Also renames IsPositioned to IsAbsPosContainingBlock.
Attachment #8575586 -
Flags: review?(mats)
Assignee | ||
Updated•10 years ago
|
Attachment #8574615 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8575587 -
Flags: review?(mats)
Assignee | ||
Comment 16•10 years ago
|
||
Passes tests on Linux64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4112a78af8d3
Comment 17•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8575587 -
Flags: review?(mats) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
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: 10 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1281068
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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...
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•