272885 - disable rendering of some elements when their width or height is zero

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Rendering of certain elements should be disabled if their width or height
attributes are set to zero. This is already the case in nsSVGRectFrame and
nsSVGOuterSVGFrame. We need to correct the beheviour for
nsSVGForeignObjectFrame, nsSVGUseFrame and nsSVGInnerSVGFrame. While we're at it
I'd like to make the default values for these attributes zero if a value is
required for them by the spec. That way they will be disabled if someone forgets
to specify them.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

This patch fixes the above and also fixes some memory leaks caused by
forgetting to RemoveObserver().

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 167701 [details] [diff] [review]
patch

Can you review, and check in if you're happy with this Tim?

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch with better comment grammer

otherwise the same

Comment 4

[Hixie (not reading bugmail)]

You shouldn't default to zero, you should act as described in SVG 1.1 appendix
F, namely, abort rendering of the SVG fragment at that point and display an
inline error message over the <svg> element's render area. See bug 246514.

Comment 5

[Hixie (not reading bugmail)]

Just to clarify, my last comment was regarding the sentence in comment 0 that
says "while we're at it I'd like to make the default values for these attributes 
zero if a value is required for them by the spec". Some attributes can indeed be
omitted and default to zero and cause things to not be rendered, but that is not
what I was referring to.

Comment 6

[Jonathan Watt [:jwatt]]

I know the spec says that, but we agreed we aren't going to do that in the
forseeable future. Preventing the element from being displayed is the best I
think we can do for now, and it provides more of a hint to document authors that
they are doing something wrong than simply displaying it anyway. The sooner moz
starts indicating to people that their code is in error, the less legacy SVG
we'll have that suddenly won't work if we do decided to go about stopping
rendering and showing a hatched area/error message/whatever in the future. As
you can see in the patch, the code that does this has an

  // XXX: enforce requiredness
  // for now we just set the width to zero to disable rendering of the element

comment right above it. I didn't intend this as a permenant fix.

Comment 7

[tor]

Comment on attachment 167707 [details] [diff] [review]
patch with better comment grammer

Don't want to review until the HasAttr problem is fixed.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Isn't the HasAttr problem fixed now?

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 167707 [details] [diff] [review]
patch with better comment grammer

Yes. I CC'ed you and bz since you know how the display:none stuff works.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Nope, I don't :) Layout is still uncharted terratory for me :)

Comment 11

[Jonathan Watt [:jwatt]]

Ah, my mistake. :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Which particular display:none stuff?  I see no mention of it in the bug or the
patch....

Comment 13

[Jonathan Watt [:jwatt]]

For some attributes (typically 'width' and 'height') the spec says "A value of
zero disables rendering of the element". I assume that's roughly what
display:none does. If I remember correctly, display:none prevents the frames
from being constructed. I'm not sure if we can do that here or if we have to do
something more along the lines of what I was doing and prevent painting instead.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Don't width and height depend on the style?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

bz, what do we do for images with width/height set to 0? I suspect (though i'm
not sure) that we still want to create the frame, possibly so that we can deal
with the situation of the attribute being set to a non-zero value dynamically.

Just make sure that we don't paint it or make it recieve events (i.e. you
shouldn't be able to hover one of these elements)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Images with width/height set to zero skip painting the actual image (see
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsImageFrame.cpp#1309). 
They still have to paint borders and padding (the background paints under the
latter), of course.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, and hovering the border or padding of a width-zero or height-zero image
will, of course, dispatch an event to the image.

Also, such images take up space in the layout (eg a width-zero but nonzero
height image with block display will push down things that are below it in the
block progression).

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: patch

This bug has been mostly fixed by other checkins. Here's a patch for nsSVGInnerSVGFrame.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch with gfxContext.h change

Should probably include the gfxContext.h change in the diff. :-P Requesting sr from vlad for the change to that file.

Comment 20

[Jonathan Watt [:jwatt]]

Actually, we aren't acquiring a resource as such, so this is more a sentry class. A better comment would be:

/**
 * Sentry helper class for functions with multiple return points that need to
 * call Save() on a gfxContext and have Restore() called automatically on the
 * gfxContext before they return.
 */

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 299442 [details] [diff] [review]
patch with gfxContext.h change

gfxContext change is fine, I'm not a huge fan of the name but I can't think of anything better.  Maybe just "gfxAutoSaveContext"

Comment 22

[tor]

Comment on attachment 299442 [details] [diff] [review]
patch with gfxContext.h change

>+/**
>+ * RAII helper class to automatically call Save() and Restore() for complex
>+ * functions with multiple return points.
>+ */

I was about to say that you may want to expand the acronym for those who haven't memorized a design patterns book, but I see you have another comment in mind.

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 299442 [details] [diff] [review]
patch with gfxContext.h change

Requesting approval1.9. Low risk patch to optimize case where there's nothing to paint.

Comment 24

[Jonathan Watt [:jwatt]]

Checked in. I just left the name as-is Vlad, since I think the "Restore" is the more important bit.

I'm going to call this fixed. We don't return for <use>, but I don't think it's worth adding an extra virtual function and getting and checking the width/height for what should be a rare case.

Comment 25

[Robert Longson [:longsonr]]

Maybe it's me but I can't find the code change.

Comment 26

[Jonathan Watt [:jwatt]]

Yeah, sorry, I bungled the checkin. I'll check in for real once the red has cleared.

Comment 27

[Robert Longson [:longsonr]]

Oops, looks like I reopened this just as you checked it in.