Open Bug 376027 Opened 17 years ago Updated 6 months ago

SVG display:none rules

Categories

(Core :: SVG, defect, P3)

Product:
Core
Component:
SVG
Platform:
x86
Linux
Type:
defect

Tracking

()

People

(Reporter: tor, Unassigned, NeedInfo)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(9 files, 6 obsolete files)

proof of concept of second approach
5.53 KB, patch
Details | Diff | Splinter Review
simple testcase of display:none linearGradient - should display text with gradient
348 bytes, image/svg+xml
Details
Test case 2: SVG in HTML (pattern inside display:none paragraph)
506 bytes, text/html
Details
Test case 3: HTML in SVG (pattern with foreignObject HTML inside a display:none g)
514 bytes, image/svg+xml
Details
Test case 4: foreignObject and use
461 bytes, image/svg+xml
Details
Test case 1: SVG only (pattern inside display:none group)
386 bytes, image/svg+xml
Details
Ref test from patch
10.74 KB, text/html
Details
Proposed patch v1c; r=bz,longsonr;feedback=davidb
25.78 KB, patch
dbaron
: superreview-
Details | Diff | Splinter Review
Some ideas in progress
24.29 KB, patch
Details | Diff | Splinter Review 
Spinoff from bug 375775, regarding our behavior with display:none in a SVG environment.

The SVG specification has a number of elements that can be referred to in the rendering of other elements.  These include <filter>, gradients, <pattern>, <marker>, <clipPath>, and <mask>.  All of these have a note in the specification regarding how display:none should affect them (<filter> used as an example here):

"'filter' elements are never rendered directly; their only usage is as something that can be referenced using the 'filter' property. The 'display' property does not apply to the 'filter' element; thus, 'filter' elements are not directly rendered even if the 'display' property is set to a value other than none, and 'filter' elements are available for referencing even when the 'display' property on the 'filter' element or any of its ancestors is set to none."

This doesn't work well with Gecko, as frame construction stops when display:none is encountered as an optimization in the CSS rendering model.  We need the frames  for style information used to render these element and their children (arbitrary SVG content for all but the filters and gradients cases).

A couple possible solutions:  1) Move everything SVG into content and have them hold onto the style context.  The problem with this is getting notification when style has changed.  2) Ignore display:none for SVG in the frame constructor and have the SVG frame code check for the display property where necessary.  I have vague memories of proposing this second approach before and having it shot down, but don't remember the particulars.
A trial at modifying nsCSSFrameConstructor to ignore display:none for SVG and handling the property in the SVG painting code.  Seems to achieve the spec behavior.

Not complete - there's code in layout/style that checks for display:none which I'm not sure if needs to be changed, and the SVG code also needs to check the property when doing hit testing.
What if display:none is set on some ancestor of the filter?
"...available for referencing even when the 'display' property on the 'filter' element or any of its ancestors is set to none."

Filter should still work in that case.

Right, but your frame constructor approach won't work, right?
No, it works fine - the display:none is ignored and all the frames needed are constructed.  Just tested it by throwing a display:none <defs> around the filter in the attached testcase.

In a mixed namespace environment things would break down, but that's something for the CDF group to figure out (or not):

<html:div style="display: none;">
  <svg:filter>
    ...
  </svg:filter>
</html:div>

<svg:svg>
  <svg:rect fill="#filter" .../>
</svg:svg>

It seems like this is pretty unlikely to interoperate with other implementations for the compound document case.  If something is supposed to work when a node is display:none or inside something that's display:none, it should generally operate off of content nodes rather than frames.
Even if we pulled all the functionality of SVG into the content nodes, stopping frame construction wouldn't work with foreignObject.  For example:

<svg:pattern style="display:none">
  <svg:foreignObject>
    <html:iframe width="50" height="50" src="http://www.google.com/"/>
  </svg:foreignObject/>
</svg:pattern>

Isn't that just a pattern that contains display:none content, so it doesn't draw anything?

Consider this: how should the IFRAME be laid out? It's display:none so even if we think it "should" be rendered we don't know if it's a table, block, inline or what.
display doesn't inherit; it sounds like you're thinking it does.
Seems OK as of (recent?) Firefox 3 builds.

Attachment "simple testcase of display:none filter - should display blurred rectangle":
Firefox 2 - KO
Firefox 3 - OK

Version:
Firefox 2 - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13
Firefox 3 - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre
The only reasonable way I can think of to fix this is to add a mode that causes us to create special frames for display:none content --- invisible frames that don't do anything, that don't even call Reflow or BuildDisplayList on their children. We'd enter this mode when a document got content that needs it --- SVG content, say. Boris, any thoughts?
Why do we look at frames rather than content nodes when we're using a filter, anyway?  What are the frames for?
We just get style data from them. We could get style data via content, but then we wouldn't be notified on style changes.

It's not just filters, though. The worst case is comment #8, where we actually need HTML children of a display:none element to have frames.
For the comment 8 case, don't we want it to not only have frames but also to reflow them?
We can reflow the foreignObject just by calling nsSVGForeignObjectFrame::MaybeReflowFromOuterSVGFrame directly "at the right time".
Do we really want to fix this, or should we try to get the spec changed? What do authors actually gain from supporting this?
I vote for getting the spec changed. We're unlikely to want to change the way html works because of the likely performance hit so comment 7 is probably a showstopper for a complete working implementation as far as I can see.
Changing the spec sounds reasonable to me. I don't think authors gain anything.
Assignee: general → nobody
QA Contact: ian → general
The IE test center covers this now
URL: http://samples.msdn.microsoft.com/iet...
Results of cross-browser testing:

Test case 1 (SVG only: pattern inside <g style="display:none">):

Chrome 15: Renders pattern
IE 9: Renders nothing
Opera 11.50: Renders nothing (except outline)
Firefox: Black fill

Test case 2 (SVG in HTML: pattern inside <p style="display:none">):

Chrome 15: Black fill
IE 9: Renders nothing
Opera 11.50: Renders nothing (but w/o display:none it also renders nothing)
Firefox: Black fill

Test case 3 (HTML in SVG: pattern with HTML <foreignObject> inside <g style="display:none">):

Chrome 15: Renders pattern
IE 9: Renders nothing
Opera 11.50:  Renders nothing (except outline)
Firefox: Black fill

In summary:

WebKit supports referencing content with an SVG parent that is display:none but not an HTML parent with display:none.
Other browsers don't support any combination.

Proposal:

Basically what tor's proposed patch does. Create frames for SVG content even with display:none but then be careful not to render them. That should give us similar behaviour to WebKit.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — — Splinter Review 
Here's a wip patch.
I still need to:
* Add a reduced version of test case 3 as a crashtest (it was firing assertions) about the dirty rect being null
* Fix how we handle glyph frames that are display none. I think we shouldn't be generating frames for them.
* Review some of the changes made to nsCSSFrameConstructor.
Attached patch Proposed patch v1a (obsolete) — — Splinter Review 
Proposed patch.

One note is that I removed a line from the SVG mochitest test_text.html. Previously even when getNumberOfChars returned 0, we failed to return an out of range error for getSubStringLength(0, 3). Now we return the error.

(Whether getNumberOfChars *should* return 0 in this case is another question. I will post to www-svg about that but in any case that is a separate bug. This patch only makes us more consistent.)
Attachment #575082 - Attachment is obsolete: true
Attachment #577173 - Flags: review?(bzbarsky)
(In reply to Brian Birtles (:birtles) from comment #33)
> (Whether getNumberOfChars *should* return 0 in this case is another
> question. I will post to www-svg about that but in any case that is a
> separate bug. This patch only makes us more consistent.)

http://lists.w3.org/Archives/Public/www-svg/2011Nov/0135.html
I don't understand the nsCSSFrameConstructor::FindTextData changes.  What is the intent of those?  Why is checking just the parent, and not arbitrary ancestors of it, for display:none enough?

The clip path frame and svg container frame changes need review from someone who knows that code.  Same for svgutils.

The rest looks fine.
Attached patch Proposed patch v1b (obsolete) — — Splinter Review 
(In reply to Boris Zbarsky (:bz) from comment #35)
> I don't understand the nsCSSFrameConstructor::FindTextData changes.  What is
> the intent of those?  Why is checking just the parent, and not arbitrary
> ancestors of it, for display:none enough?

It's not, it's a bug. I didn't understand how the frame construction works. It should be fixed now in this updated patch. I've added a test as well to check.

With regard to the intent of the changes, half of them are just making the code a little easier to follow by replacing nesting with early returns. The rest is to ensure that tspans with display:none don't end up creating space in text runs. For most SVG frames we can just test display:none when we go to paint and turn it off then, but tspans are different.

I've made the change in FindTextData so it returns nsnull when we detect display:none on an ancestor. Alternatively, we could do more or less the same thing in FindSVGData and return &sSuppressData. The main reason I haven't done that is it looked messier to do it there but I don't know if there are implications of doing it one way or the other.

> The clip path frame and svg container frame changes need review from someone
> who knows that code.  Same for svgutils.

I'll request further review if you're happy with the updated patch.

Thanks Boris!
Attachment #577173 - Attachment is obsolete: true
Attachment #578973 - Flags: review?(bzbarsky)
Attachment #577173 - Flags: review?(bzbarsky)
Comment on attachment 578973 [details] [diff] [review]
Proposed patch v1b

Returning null from FindTextData is fine; there's an explicit check in the caller for that.

r=me on the frame construction bits.  Probably good to have dbaron sr...

And maybe ask the a11y folks to take a look?
Attachment #578973 - Flags: review?(bzbarsky) → review+
Why not change 

NS_IMETHODIMP_(nsSVGGlyphFrame *)
nsSVGGlyphFrame::GetFirstGlyphFrame()
{
  nsSVGGlyphFrame *frame = this;
  while (frame && frame->IsTextEmpty()) {
    frame = frame->GetNextGlyphFrame();
  }
  return frame;
}

to

nsSVGGlyphFrame::GetFirstGlyphFrame()
{
  nsSVGGlyphFrame *frame = this;
  while (frame &&
         frame->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_NONE &&
         frame->IsTextEmpty()) {
    frame = frame->GetNextGlyphFrame();
  }
  return frame;
}

instead of all that bodging around in nsCSSFrameConstructor for text frames? Isn't that what you need?
Or more correctly...

nsSVGGlyphFrame::GetFirstGlyphFrame()
{
  nsSVGGlyphFrame *frame = this;
  while (frame &&
         (frame->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_NONE ||
          frame->IsTextEmpty())) {
    frame = frame->GetNextGlyphFrame();
  }
  return frame;
}
(In reply to Robert Longson from comment #38)
> instead of all that bodging around in nsCSSFrameConstructor for text frames?
> Isn't that what you need?

Unfortunately using that approach you end up with space in the text run.

In the long run we will need to do something different for these tspans in order to give more sensible results for the SVG text metrics APIs (as per http://lists.w3.org/Archives/Public/www-svg/2011Nov/0135.html) but that's a separate bug.
Comment on attachment 578973 [details] [diff] [review]
Proposed patch v1b

On that note, Robert, would you be able to look at the SVG specific stuff in here? Specifically the SVG frame stuff and SVG utils stuff as Boris pointed out in comment 35?

Requesting sr from dbaron too as suggested.
Attachment #578973 - Flags: superreview?(dbaron)
Attachment #578973 - Flags: review?(longsonr)
Comment on attachment 578973 [details] [diff] [review]
Proposed patch v1b

David Bolter, would you be able to have a look at this from an a11y point of view?
Attachment #578973 - Flags: feedback?(bolterbugz)
Would you please give some summary of the patch to get some idea how it could affect on accessibility? In general svg content is not accessible and svg element itself is exposed as diagram to screen readers and nothing else.
(In reply to alexander :surkov from comment #43)
> Would you please give some summary of the patch to get some idea how it
> could affect on accessibility? In general svg content is not accessible and
> svg element itself is exposed as diagram to screen readers and nothing else.

In that case I guess there's no impact.

Summary of the patch: normally when we go to render HTML content that is marked 'display:none' we ignore it and don't generate frames for it. However, SVG has special rules that says that some content--even if its ancestor is display:none--can still be referred to elsewhere and rendered. Currently we don't support that in Gecko and this patch tries to fix it. (Frames for most display:none SVG content will now be generated but simply not painted.)

I'm not sure exactly what Boris had in mind regarding a11y. The behaviour with regard to SVG text is essentially unchanged in that frames are still not generated for display:none.
(In reply to Brian Birtles (:birtles) from comment #44)

> Summary of the patch: normally when we go to render HTML content that is
> marked 'display:none' we ignore it and don't generate frames for it.
> However, SVG has special rules that says that some content--even if its
> ancestor is display:none--can still be referred to elsewhere and rendered.
> Currently we don't support that in Gecko and this patch tries to fix it.
> (Frames for most display:none SVG content will now be generated but simply
> not painted.)

could svg element be hidden by display:none and has generated (not painted) frames?
Comment on attachment 578973 [details] [diff] [review]
Proposed patch v1b

>diff --git a/layout/svg/base/src/nsSVGClipPathFrame.cpp b/layout/svg/base/src/nsSVGClipPathFrame.cpp
>--- a/layout/svg/base/src/nsSVGClipPathFrame.cpp
>+++ b/layout/svg/base/src/nsSVGClipPathFrame.cpp
>@@ -95,16 +95,21 @@ nsSVGClipPathFrame::ClipPaint(nsSVGRende
>       clipPathFrame->ClipPaint(aContext, aParent, aMatrix);
>     } else {
>       gfx->PushGroup(gfxASurface::CONTENT_ALPHA);
>     }
>   }
> 
>   for (nsIFrame* kid = mFrames.FirstChild(); kid;
>        kid = kid->GetNextSibling()) {

Looks like we could do with a GetFirstDisplayedFrame() and GetNectDisplayedSibling() on nsIFrame which would skip kids that are DISPLAY_NONE.

I still don't like the nsCSSFrameConstructor stuff. From your comment about my previous suggestion it would seem that you would need to skip display none frames in

GetFirstGlyphFragmentChildNode, GetNextGlyphFragmentChildNode and BuildPositionList
Comment on attachment 578973 [details] [diff] [review]
Proposed patch v1b

f=me thanks for asking! The work here is very interesting to me (I'll may crib from it for canvas a11y).
Attachment #578973 - Flags: feedback?(bolterbugz) → feedback+
Additional note:
We (accessibility) do create accessible objects for svg elements with ARIA attributes (since the html5 parser was flipped on). For example: http://people.mozilla.com/~dbolter/svg-checkbox-test.html

I don't forsee any huge problems. We can should be able to fix minor problems as we find them (e.g. <filter role="checkbox"> might be a bit too silly). Accessibility usually bails for elements with no frame, so I think the silly example above wouldn't be able to work prior to this patch. This frame dependence is also something related to our canvas a11y work.
I missed the point that ARIA can be used to make svg elements (other than svg element) accessible like http://people.mozilla.com/~dbolter/svg-checkbox-test.html. Thanks, David, to pointing this out.

So it sounds like we could have accessible tree update problems. For example, if the following scenario is possible. Say the ancestor gets the style display:none, then a11y receive the notification that frame for the ancestor was destroyed and we destroy its whole accessible subtree including svg elements that may stay visible. If that's a valid scenario then we need to deal with it.
(In reply to alexander :surkov from comment #49)
> So it sounds like we could have accessible tree update problems. For
> example, if the following scenario is possible. Say the ancestor gets the
> style display:none, then a11y receive the notification that frame for the
> ancestor was destroyed and we destroy its whole accessible subtree including
> svg elements that may stay visible. If that's a valid scenario then we need
> to deal with it.

Firstly, this only applies to SVG content (and excluding the outermost <svg>). If we have:

  <div style="display:none">
    <svg>...</svg>
  </div>

No frames will be generated for the SVG subtree.

Likewise:

  <div>
    <svg style="display:none">...</svg>
  </div>

No frames will be generated for the SVG subtree.

It's only when we have:

  <svg>
    <g style="display:none">...</g>
  </svg>

that we will still generate frames for the <g> and it's descendants.

So I guess it depends if you're creating any accessibility objects from the SVG frames other than the outer <svg>.
(In reply to Robert Longson from comment #46)
> I still don't like the nsCSSFrameConstructor stuff. From your comment about
> my previous suggestion it would seem that you would need to skip display
> none frames in
> 
> GetFirstGlyphFragmentChildNode, GetNextGlyphFragmentChildNode and
> BuildPositionList

and GetNextGlyphFrame.

Can you explain what is specifically of concern with regards to nsCSSFrameConstructor::FindTextData?

Although I've implemented the alternative approach (change nsGlyphFrame::GetFirstGlyphFrame,GetNextFrame; nsTextContainerFrame::GetFirstGlyphFragmentChildNode,GetNextGlyphFragmentChildNode,BuildPositionList) I'm a bit uncomfortable with it because I'm not sure even those five methods have covered all bases.

If we go with this approach I think a lot more testing is required both for this change and to be sure we don't regress in the future by forgetting to skip display:none frames is any new functionality that iterates over glyph frames. Helper methods such as you've suggested will mitigate this to some extent.

Actually, I think in order to return reasonable results from text metrics APIs (see comment 40) we will have to do something like what you suggest in the future. But I'd prefer to tackle that as a separate change. i.e. create a whole heap of tests for display:none and tspans that cover both the rendering and APIs and then refactor things at that point. I've filed bug 708502 for that.
Note that with Cameron's work, SVG text will go through the regular nsBlockFrame/nsInlineFrame/nsTextFrame frame construction path. That will affect bug 708502. We can discuss it further there.
(In reply to Brian Birtles (:birtles) from comment #50)

> It's only when we have:
> 
>   <svg>
>     <g style="display:none">...</g>
>   </svg>
> 
> that we will still generate frames for the <g> and it's descendants.

So if you change display style here then it doesn't result in frame creation/desctruction. That should be ok with a11y. But I bet I need to know svg better to be sure.

> So I guess it depends if you're creating any accessibility objects from the
> SVG frames other than the outer <svg>.

yes, we do if the author uses ARIA (put @role attribute on element).
(In reply to Brian Birtles (:birtles) from comment #51)

> Actually, I think in order to return reasonable results from text metrics
> APIs (see comment 40) we will have to do something like what you suggest in
> the future. But I'd prefer to tackle that as a separate change. i.e. create
> a whole heap of tests for display:none and tspans that cover both the
> rendering and APIs and then refactor things at that point. I've filed bug
> 708502 for that.

OK, I can live with punting this to bug 708502 and doing it your way for the moment.

I'd still like to see a GetFirstDisplayedFrame() and GetNectDisplayedSibling() implemented with an assertion in each to ensure that the frame content is in the SVG namespace.
And I see now you're doing the displayed frame stuff there too.
Attachment #578973 - Flags: review?(longsonr) → review+
Is there a summary somewhere of what this patch does?

(Ideally, such a summary goes in the commit message, but outside of the first line of the commit message.)
(In reply to David Baron [:dbaron] from comment #56)
> Is there a summary somewhere of what this patch does?

Yes, comment 44 gives an overview.

From a slightly more technical perspective this patch:

- Alters nsCSSFrameConstructor so that we now generate frames for SVG content even if it is display:none (with two main exceptions, outer <svg> elements with display:none and text nodes)
- Alters SVG frame code to skip painting when display is 'none'.

I wasn't aware of the practice of adding a longer summary to patches but will do so before landing.
Brian, can you point me to a windows try build? I'd like to test something.
(In reply to David Bolter [:davidb] from comment #58)
> Brian, can you point me to a windows try build? I'd like to test something.

I just kicked off a new build since it's been a while since the last time I checked and this patch is just pending sr now.

https://tbpl.mozilla.org/?tree=Try&rev=75dc875a2e3e
Try run for 75dc875a2e3e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=75dc875a2e3e
Results (out of 206 total builds):
    success: 178
    warnings: 28
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-75dc875a2e3e
Thanks Brian. My tests didn't raise any flags.
So this really feels like the wrong approach architecturally to me.  It also seems like it still wouldn't fix cases where the SVG is inside something in HTML that's display:none.

The normal way that things work in our architecture is that behavior that still happens when something is display:none should live in the content nodes rather than the frames.  How hard would that be to do?
We would need to at least be able to compute (and dynamically update) style contexts for frameless content nodes.
OK, I guess.  In the long run style data should probably be attached to content.  (If we clone for things like svg:use, XBL, etc., then that should be ok.)

What happens with this patch if an svg:foreignObject is inside an SVG element with display:none?  I don't see anything that would suppress frame construction or reflow for such a foreignObject; I tend to think there probably should be something.
Fix bitrot. (In the meantime, if we'd landed this back in Dec we would have fixed bug 667324 and bug 716527 in the process!)
Attachment #578973 - Attachment is obsolete: true
Attachment #593728 - Flags: superreview?(dbaron)
Attachment #578973 - Flags: superreview?(dbaron)
(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #64)
> What happens with this patch if an svg:foreignObject is inside an SVG
> element with display:none?

It doesn't display (unless of course it is referenced elsewhere). See the attached test case 4. In this case you will only see the <use> instantiated <foreignObject> and <rect> both in a trunk build and with the patched build. A very basic test for this is included in the patch.

The frames are still created but not painted (in nsSVGUtils::PaintFrameWithEffects).

Is this problematic?




>  I don't see anything that would suppress frame
> construction or reflow for such a foreignObject; I tend to think there
> probably should be something.
Previously uploaded test case 1 was incorrect.
Attachment #574509 - Attachment is obsolete: true
Attachment #574510 - Attachment mime type: image/svg+xml → text/html
Attached file Ref test from patch — 
Attaching test case as separate attachment for easier cross-browser testing.
Attachment #593735 - Attachment mime type: image/svg+xml → text/html
Regarding the interaction between display:none in HTML/SVG mixed documents I've posted to www-svg:

  http://lists.w3.org/Archives/Public/www-svg/2012Feb/0000.html
David, shall I understand comment 64 to mean you're satisfied with the approach technically for the time being (although in the long-term a different approach may be better)?

Regarding the expected behaviour of SVG and HTML, the thread on www-svg didn't reach a conclusion. However, I think what we're doing (not creating frames for HTML display:none nodes and their descendants) is reasonable for the time being until that behaviour is clarified.

For now, I still thinking landing this is a step in the right direction. For the only-SVG case this makes our behaviour correct (and fixes the display of Santa's Workshop, Foxkeh clipart, and a lot of Illustrator output). The included test covers a number of cases which are incorrect in current builds but fixed by this patch. For the mixed SVG and HTML case it doesn't change our behaviour.

If we later decide to take a different approach, at least the test case included in this patch will ensure we don't regress the only-SVG case.

What do you think David?
superreview ping David?
Fix bitrot (again).

Any idea David on when you'll get to this? It's been quite a while.
Attachment #593728 - Attachment is obsolete: true
Attachment #616469 - Flags: superreview?
Attachment #593728 - Flags: superreview?(dbaron)
Attachment #616469 - Flags: superreview? → superreview?(dbaron)
superreview ping? Approaching the 6-month mark.
What do other implementations do here?  (If we'd be the first to implement this behavior, I think we should definitely fix the spec.)
Comment on attachment 616469 [details] [diff] [review]
Proposed patch v1c; r=bz,longsonr;feedback=davidb

OK, I'm going to put my foot down:  I think this is incompatible with the architecture underlying the CSS model, and we should get the spec changed.  Style data on the descendant of something that's display:none should not be relevant (except when explicitly queried).
Attachment #616469 - Flags: superreview?(dbaron) → superreview-
(In reply to David Baron [:dbaron] from comment #75)
> What do other implementations do here?  (If we'd be the first to implement
> this behavior, I think we should definitely fix the spec.)

With regards to SVG, we're not the first to implement, we're the last, and by a long shot. Furthermore there is a lot of content that relies on this behaviour (particularly content exported from Illustrator).

For the mix of SVG and HTML see comment 31 and comment 69.

(In reply to David Baron [:dbaron] from comment #76)
> Style data on the descendant of something that's display:none should not be
> relevant (except when explicitly queried).

I don't understand. It is explicitly queried here (via, e.g. filter properties, fill properties etc.)
(In reply to Brian Birtles (:birtles) from comment #77)
> (In reply to David Baron [:dbaron] from comment #75)
> > What do other implementations do here?  (If we'd be the first to implement
> > this behavior, I think we should definitely fix the spec.)
> 
> With regards to SVG, we're not the first to implement, we're the last, and
> by a long shot. Furthermore there is a lot of content that relies on this
> behaviour (particularly content exported from Illustrator).
> 

We should suggest to illustrator that they use <defs> or visibility="hidden" as appropriate rather than display="none".

I too am not convinced that this patch is a good idea despite it fixing some examples out there.
(In reply to Robert Longson from comment #78)
> We should suggest to illustrator that they use <defs> or visibility="hidden"
> as appropriate rather than display="none".

I agree <defs> is preferable, but what they're doing is still quite reasonable according to the current spec. If we want them to change we should change the spec.

> I too am not convinced that this patch is a good idea despite it fixing some
> examples out there.

Can you please explain your concern here. I'm not arguing for the merits of the patch, I just want to bring our implementation and the spec into line. If you think there's a good argument for getting the spec changed, then I'm all in favour of that.

We need to bear in mind, however, that the amount of content out there relying on this behaviour is significant--there's even some on Mozilla's own properties, and there is content beyond that generated from Illustrator (e.g. Santa's Workshop). Changing the spec will mean breaking a fair bit of content.
(In reply to Brian Birtles (:birtles) from comment #79)
> 
> I agree <defs> is preferable, but what they're doing is still quite
> reasonable according to the current spec. If we want them to change we
> should change the spec.

I think we should try to get the spec changed.

> Can you please explain your concern here. I'm not arguing for the merits of
> the patch, I just want to bring our implementation and the spec into line.
> If you think there's a good argument for getting the spec changed, then I'm
> all in favour of that.

No UA, including us with this patch, implements a mixed html/svg document where the html containers are display:none according to the letter of the svg spec.
(In reply to Robert Longson from comment #80)
> No UA, including us with this patch, implements a mixed html/svg document
> where the html containers are display:none according to the letter of the
> svg spec.

I'm not sure what this comment refers to. Can you illustrate?

I'm mostly interested in fixing the SVG-only case (which is what is implemented in other UAs). If you want to modify the patch to achieve that I'm all for it.
I don't think it's possible to fix the patch without affecting html performance.
(In reply to Brian Birtles (:birtles) from comment #81)
> (In reply to Robert Longson from comment #80)
> > No UA, including us with this patch, implements a mixed html/svg document
> > where the html containers are display:none according to the letter of the
> > svg spec.
> 
> I'm not sure what this comment refers to. Can you illustrate?

I presume something like
<div style="display:none">
  <svg>
    <linearGradient id="grad">...</linearGradient>
  </svg>
</div>
<svg fill="url(#grad)"></svg>

We need to sort this out!
I've re-read the thread on this to refresh my memory (http://lists.w3.org/Archives/Public/www-svg/2012Feb/0000.html).

Basically, when I tested a year ago, no UA allowed referencing content that is part of a display:none subtree where the display property appears on an HTML element, but my attempt get that standardised were blocked.

The options as I understand them are:

A) Do what this patch does, i.e. fix SVG display:none subtrees only--matches WebKit behaviour last I tested; probably has significant performance costs that need to be addressed.
B) Do nothing and get the spec changed--looks pretty much impossible and doesn't solve the problem with existing content from Illustrator / Santa's workshop
C) Do what Tab is suggesting--sounds like a lot of work

For now I'll work on un-bitrotting this patch and see if I can work out the current performance cost.
IMHO C is a ton of work that is unmotivated by use-cases. Given that our attempts to get the reasonable alternative B specced have been rejected, I think we should consider this a legacy compatibility problem, consider the de-facto spec to be A, ugly as it is, and implement that.

(If we ever decide to implement C, we can do that as a clean extension of A.)
I spoke with Cameron about some different approaches to this an here is what we came up with so far:

Basic approach: only create frames for descendants of SVG display:none subtrees that are referenced or have descendents that are referenced.

Background information:
-----------------------

In terms of indicating which content is referenced (or has descendent content that is referenced) we can annotate content with flags.

nsINode.h
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsINode.h
- Has two sets of flags
   a) NODE_* enum values (line 77 onwards). We shouldn't be extending these (according to bz).
   b) enum BooleanFlag (line 1253 onwards). Helper methods for setting these flags (e.g. NodeHasRenderingObservers -> SetHasRenderingObservers etc.)

We can add an extra enum value here (and corresponding getter/setter) to indicate which content is referenced (or has descendents that are referenced).

With regards to frame generation, when we generate frames we usually test the display property and if it is display:none, we skip generating frames.

There is also an NS_STATE_SVG_NONDISPLAY_CHILD state bit on a frame. This is used for things like <defs> etc. to prevent reflow of this content (except we do reflow of text).
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGUtils.h#67

This content has frames but is not rendered directly.

Outline of strategy:
--------------------

1. Add a flag to the content flags, "HasReferencedSVGContent" ?

2. When resolving references to mask content, clip path content, filters etc. set the HasReferencedSVGContent flag on the target element and all its ancestors up to the outermost SVG element.
  - This would probably be done in nsSVGEffects.h in the ID rendering observer plumbing. However, note that we don't need to set the flag in the case of <use> element references since they operate by cloning content.
  - At this point, if we are in a display:none subtree, we need to trigger frame generation at the appropriate point--probably the ancestor-most display:none node?

3. When generating frames we would test on both the display:none property and the presence of the HasReferencedSVGContent flag to determine if frames are needed or not.
  If we have display:none and HasReferencedSVGContent = true then we want to generate frames but with NS_STATE_SVG_NONDISPLAY_CHILD set to true

That's the general approach, but we haven't worked out the specifics. For example, consider the following case:
  
  <g display="none">
    <mask id="a">...</mask>
    <mask id="b"><rect/></mask>
  </g>
  <path mask="url(#b)"/>

In this case:
i)   For <g>, we have display:none and HasReferencedSVGContent, so we continue generating frames with NS_STATE_SVG_NONDISPLAY_CHILD.
ii)  For mask 'a' we need to detect that we have an ancestor with display:none (how to do this?) but no HasReferencedSVGContent flag so we should skip generating frames.
iii) For mask 'b' we also need to detect the display:none ancestor so that when we generate frames (because HasReferencedSVGContent is true) we set NS_STATE_SVG_NONDISPLAY_CHILD on the frame.
iv)  For the child path of 'b' we don't have HasReferenceSVGContent set to true... so how do we know to set NS_STATE_SVG_NONDISPLAY_CHILD? And how do we know to generate the frame? What distinguishes it from (ii)?

It seems like we want to pass some state down as we generate frames but then what happens when we just want to regenerate frames for a part of the tree?

If we are able to ensure the frame construction begins at the display:none root we can use the frame construction state to determine what type of tree we're in.

In general we don't bother clearing the HasReferencedSVGContent flag (once those frames get generated they just stick around) but there are probably some circumstances where we can safely clear it (e.g. when the nodes get removed from the document).


One other case we considered:

  <g id="a" display="none">
    <rect/>
    <g display="none">
      <circle/>
      <polygon id="b"/>
    </g>
  </g>

If #a and #b are referenced, how do we ensure that the <circle> does not contribute to #a, if it will get a frame created for it?  Maybe this doen't matter because the <g> and <polygon> here would really have to be <mask>, <clipPath>, etc., and they don't have any effect when children of each other?
I made a start on implementing some of the ideas in comment 87.

A few problems so far:
* It's hooking in too late. We detect when an element is referenced and then go to generate frames meaning sometimes you get flicker where we paint once without the referenced resource.
* It doesn't work with display:none gradient stops since they're not directly referenced. SVG doesn't actually say display:none is ignored on <stop> elements but Trident and Blink ignore it and that seems reasonable given that it's ignored for gradient elements. (I'll make sure this gets specified one way or the other in SVG2.)

I'm really not sure if this is the right approach. Should we just generate frames for all display:none SVG elements but set them to NS_FRAME_IS_NONDISPLAY?

I probably won't have a chance to look at this for a few days but maybe others have ideas?
(In reply to Brian Birtles (:birtles) from comment #88)
> * It's hooking in too late. We detect when an element is referenced and then
> go to generate frames meaning sometimes you get flicker where we paint once
> without the referenced resource.

I think we can fix this by having nsReferencedElement set the "referenced" flag and trigger restyling as soon as it identifies the target element.

> * It doesn't work with display:none gradient stops since they're not
> directly referenced. SVG doesn't actually say display:none is ignored on
> <stop> elements but Trident and Blink ignore it and that seems reasonable
> given that it's ignored for gradient elements. (I'll make sure this gets
> specified one way or the other in SVG2.)

I think we could probably fix this by forcing creation of frames for all SVG descendents of a gradient element via some flag passed through frame construction.

> I'm really not sure if this is the right approach. Should we just generate
> frames for all display:none SVG elements but set them to
> NS_FRAME_IS_NONDISPLAY?

I think that's probably fine too.
No longer blocks: 1127624
I came across this bug while attempting to build a javascript loader for external SVG resources. What I found interesting is that <symbol> and <marker> elements in <defs> sections behave differently. The <symbol> elements seem to behave correctly according to the logic of the spec. That is, I can have a <use> anywhere in the document that references the <symbol>, even if that <symbol> has an ancestor that is display:none. And this behavior works in mixed HTML/SVG documents regardless of whether the display:none is found on an SVG element or an HTML element, and it still works even if the <symbol> and <use> are parts of different SVG trees (that is, all common ancestors are HTML).

Not having dug into the Firefox codebase, I don't know why the behavior of <symbol> is so different from <marker> and the other referenced SVG elements covered by this bug. If <symbol> is compliant with the standard, can a similar approach be taken to making the other elements compliant?
symbols have worse bugs because of the approach we took there (cloning the content). The CSS cascade being wrong for symbols is the main issue, but performance of symbols is bad too as they need to be recloned on change.
Unassigning me since I don't plan to work on this anytime soon.
Assignee: bbirtles → nobody
Status: ASSIGNED → NEW
Assignee: nobody → cku
Read through comment, let me try to find a solution base on comment 87/88/89
Blocks: 1395221
Priority: -- → P3
Assignee: cku → nobody
Flags: needinfo?(jwatt)

Note: bug 1463336 comment 1 has a SVGWG resolution which updates the expected-outcome of this scenario.

If I'm understanding correctly: as of that resolution, the expected outcome is now that the masked (or filtered, etc) element should render as if it were not masked (or filtered, etc.)

That seems more-straightforward than what this bug was previously trying to achieve, to match the old spec requirements.

Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 13 duplicates and 11 votes.
:jwatt, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jwatt)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(jwatt)
Duplicate of this bug: 1755678
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: