376027 - SVG display:none rules

Status: NEW

(Core :: SVG, defect, P3)

Description

[tor]

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.

Comment 1

[tor]

Attachment: simple testcase of display:none filter - should display blurred rectangle

Comment 2

[tor]

Attachment: proof of concept of second approach

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.

Comment 3

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

What if display:none is set on some ancestor of the filter?

Comment 4

[tor]

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

Comment 5

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

Right, but your frame constructor approach won't work, right?

Comment 6

[tor]

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>

Comment 7

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

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.

Comment 8

[tor]

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>

Comment 9

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

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.

Comment 10

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

display doesn't inherit; it sounds like you're thinking it does.

Comment 11

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

Er yeah. Hmm.

Comment 13

[Helder "Lthere" Magalhães]

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

Comment 14

[Robert Longson [:longsonr]]

Attachment: simple testcase of display:none linearGradient - should display text with gradient

Comment 15

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

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?

Comment 16

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

Why do we look at frames rather than content nodes when we're using a filter, anyway?  What are the frames for?

Comment 17

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

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

For the comment 8 case, don't we want it to not only have frames but also to reflow them?

Comment 19

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

We can reflow the foreignObject just by calling nsSVGForeignObjectFrame::MaybeReflowFromOuterSVGFrame directly "at the right time".

Comment 20

[Jonathan Watt [:jwatt]]

Do we really want to fix this, or should we try to get the spec changed? What do authors actually gain from supporting this?

Comment 21

[Robert Longson [:longsonr]]

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.

Comment 22

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

Changing the spec sounds reasonable to me. I don't think authors gain anything.

Comment 24

[José Jeria]

The IE test center covers this now

Comment 28

[Brian Birtles (:birtles)]

Attachment: Test case 1: SVG only (pattern inside display:none group)

Comment 29

[Brian Birtles (:birtles)]

Attachment: Test case 2: SVG in HTML (pattern inside display:none paragraph)

Comment 30

[Brian Birtles (:birtles)]

Attachment: Test case 3: HTML in SVG (pattern with foreignObject HTML inside a display:none g)

Comment 31

[Brian Birtles (:birtles)]

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.

Comment 32

[Brian Birtles (:birtles)]

Attachment: WIP patch

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.

Comment 33

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1a

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

Comment 34

[Brian Birtles (:birtles)]

(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

Comment 35

[Boris Zbarsky [:bzbarsky]]

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.

Comment 36

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1b

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

Comment 37

[Boris Zbarsky [: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?

Comment 38

[Robert Longson [:longsonr]]

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?

Comment 39

[Robert Longson [:longsonr]]

Or more correctly...

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

Comment 40

[Brian Birtles (:birtles)]

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

[Brian Birtles (:birtles)]

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.

Comment 42

[Brian Birtles (:birtles)]

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?

Comment 43

[alexander :surkov (:asurkov)]

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.

Comment 44

[Brian Birtles (:birtles)]

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

Comment 45

[alexander :surkov (:asurkov)]

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

[Robert Longson [:longsonr]]

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 47

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 48

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 49

[alexander :surkov (:asurkov)]

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.

Comment 50

[Brian Birtles (:birtles)]

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

Comment 51

[Brian Birtles (:birtles)]

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

Comment 52

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

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.

Comment 53

[alexander :surkov (:asurkov)]

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

Comment 54

[Robert Longson [:longsonr]]

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

Comment 55

[Robert Longson [:longsonr]]

And I see now you're doing the displayed frame stuff there too.

Comment 56

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

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

Comment 57

[Brian Birtles (:birtles)]

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

Comment 58

[David Bolter [:davidb] (NeedInfo me for attention)]

Brian, can you point me to a windows try build? I'd like to test something.

Comment 59

[Brian Birtles (:birtles)]

(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

Comment 60

[Mozilla RelEng Bot]

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/[e-mail]-75dc875a2e3e

Comment 61

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Brian. My tests didn't raise any flags.

Comment 62

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

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?

Comment 63

[Boris Zbarsky [:bzbarsky]]

We would need to at least be able to compute (and dynamically update) style contexts for frameless content nodes.

Comment 64

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

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.

Comment 65

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1c; r=bz,longsonr;feedback=davidb

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

Comment 66

[Brian Birtles (:birtles)]

Attachment: Test case 4: foreignObject and use

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

Comment 67

[Brian Birtles (:birtles)]

Attachment: Test case 1: SVG only (pattern inside display:none group)

Previously uploaded test case 1 was incorrect.

Comment 68

[Brian Birtles (:birtles)]

Attachment: Ref test from patch

Attaching test case as separate attachment for easier cross-browser testing.

Comment 69

[Brian Birtles (:birtles)]

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

Comment 70

[Brian Birtles (:birtles)]

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?

Comment 71

[Brian Birtles (:birtles)]

superreview ping?

Comment 72

[Brian Birtles (:birtles)]

superreview ping David?

Comment 73

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1c; r=bz,longsonr;feedback=davidb

Fix bitrot (again).

Any idea David on when you'll get to this? It's been quite a while.

Comment 74

[Brian Birtles (:birtles)]

superreview ping? Approaching the 6-month mark.

Comment 75

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

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 76

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

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

Comment 77

[Brian Birtles (:birtles)]

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

Comment 78

[Robert Longson [:longsonr]]

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

Comment 79

[Brian Birtles (:birtles)]

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

Comment 80

[Robert Longson [:longsonr]]

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

Comment 81

[Brian Birtles (:birtles)]

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

Comment 82

[Robert Longson [:longsonr]]

I don't think it's possible to fix the patch without affecting html performance.

Comment 84

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

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

Comment 85

[Brian Birtles (:birtles)]

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.

Comment 86

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

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

Comment 87

[Brian Birtles (:birtles)]

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?

Comment 88

[Brian Birtles (:birtles)]

Attachment: Some ideas in progress

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?

Comment 89

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

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

Comment 92

[dan]

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?

Comment 93

[Robert Longson [:longsonr]]

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.

Comment 96

[Brian Birtles (:birtles)]

Unassigning me since I don't plan to work on this anytime soon.

Comment 97

[u459114]

Read through comment, let me try to find a solution base on comment 87/88/89

Comment 101

[Daniel Holbert [:dholbert]]

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.

Comment 102

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 103

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

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.

Comment 107

[Jeff Muizelaar [:jrmuizel]]

Do we have clear next steps to resolving this?

Comment 108

[Robert Longson [:longsonr]]

Not really. The patches in the bug attempted to resolve <svg style="display: none"><filter></filter></svg> but we've never had anything that would resolve <template style="display: none"><svg><filter></filter></svg></template>

i.e. if you set display none at a higher level than the root SVG element then nothing will work. SVG Filters do rather require CSS to function e.g. <feFlood floor-color="currentColor"/> or <feFlood flood-color="red"/>

We have no real mechanism for display: none nodes to have style updates AFAIK so if we operated on content and then changed the colour of an feFlood node how would we get notified. This problem was identified right from the off and has never been solved. It needs input from people who are experts in the style system.

Even asking for the specification to be changed is a problem because I don't think the w3c SVG working group exists any more. That was my suggestion back in comment 21.