Closed Bug 291216 Opened 15 years ago Closed 14 years ago

canvas should not be an empty element

Categories

(Core :: HTML: Parser, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: annevk, Assigned: vlad)

Details

Attachments

(1 file, 2 obsolete files)

Making the CANVAS element an empty element is a bad design choice. It requires
parsers to actually understand CANVAS in order to read the fallback content in
the very limited ALT attribute.

Making the contents of the element the fallback content gives a lot more options
(see OBJECT element). (Flash, animated GIF, link to a server-side generated
image and other have been mentioned.)

IMHO this is a major bug and we should not ship Firefox 1.1 with CANVAS support
in this state.
Would it be possible still to write code that works in both safari and gecko? I
guess it'll ignore the end-tag but render the contents?
Assignee: nobody → parser
Component: Layout: Misc Code → HTML: Parser
Flags: blocking1.8b3?
QA Contact: layout.misc-code → mrbkap
Bug 288714 comment 27 says Safari does that, yes. So I guess an empty CANVAS
element would work cross browser. Safari should fix their bug though. Their
design is horrible.
If WHATWG conformance is desired, it has to be a non-empty element.
http://www.whatwg.org/specs/web-apps/current-work/#graphics says you can place
fallback content in it.
Assignee: parser → vladimir
Given that it actually would be possible to write code that works in both
browsers by using an empty element I agree that we should do this. It wouldn't
be possible to get fallback that way, but that's impossible in safari anyway.
What's the point of fallback in a client-side drawn graphical element?  Would
the page have to render both to the canvas, and update the canvas children each
time it changes the graphical data?  It can know whether a canvas is supported
based on whether it can get a context from it anyway; if a canvas isn't
supported, the app can replace the canvas element with fallback.

There's no point in talking about static fallback, IMO, because at that point
you'd just use an <img>, and not <canvas>.
Note that the WHATWG spec will change to reflect whatever the concensus is here.
So the reason to do this is not because the spec says so. Having said that, I do
strongly believe we should do this, for the accessibility and back compat
reasons already listed in this bug.

Regarding Safari, <canvas></canvas> should indeed work across both browsers. One
can also do this:

   <canvas><span class="fallback">...</span></canvas>

...with these styles:

   canvas + .fallback { display: none; }

...to get the fallback to disappear in Safari. (Probably. Untested.)
The reasons for fallback are numerous, for instance:

 * Providing a message to down-level clients about required support.
 * Providing a link to an alternate version for down-level clients.
 * Providing data in tabular form for a graph that is dynamically generated.

This is similar to the ability to provide a static image fallback for an
embedded movie clip, or a text description for a static image, or a message for
no-script UAs, or any number of other examples.

Authors might not always use the ability to have fallback content, but sometimes
they do want it. We should have a good answer to those authors who actually care
for their users.
Also,

* fallback to Flash
* fallback to an animated GIF
* fallback to a bunch of text that explains what should be going on, for blind users
Yes, you could always detect canvas with script, but it's more elegant (and more
reliable, for users with scripting disabled) to not to have to use script.

Hmm, perhaps we should in fact disable canvas when scripting is disabled.
Shouldn't fallback be different from accessibility? I.e. the spec should support
both an alt attribute (for a11y) as well as children (for fallback).

The argument I could see for using the same mechanism for both would be that if
something is good enough as accessibility then it might be good enough for
fallback too. Though i'm afraid it would often be abused.
Vladimir: You're assuming the canvas content will be animated and thus a static
fallback useless. I think canvas will be used even for semi-static images that
are easier to generate using javascript then through some serverside component.
roc: Good idea.

jonas: For the most part I would say that fallback content and accessibility, in
this context, are equivalent. In both cases we are talking about a UA that
doesn't natively support <canvas>. It doesn't matter if the reason for this is
that the UA is a legacy UA or a UA on a non-visual media.
(In reply to comment #11)
> Vladimir: You're assuming the canvas content will be animated and thus a static
> fallback useless. I think canvas will be used even for semi-static images that
> are easier to generate using javascript then through some serverside component.

No, I'm assuming pretty much that -- for example, I see line graphs as being
fairly easy to generate client-side using canvas.  Your fallback would in theory
have to be a server-side generated line graph image; what's the point of having
the canvas then?

Fallback for <canvas> by itself is worthless in any case -- you need fallback
for the specific context.  The assumption being made in this bug is that
<canvas> == 2D, but right now you can get a <canvas> with support for NO
contexts (if you build moz without cairo).  The element would be perfectly
valid, but you'd get no fallback, and any script expecting to get a context back
will just get bakc null.  You also might prefer a 3d context, but can get by
with a 2d contenxt -- or you might prefer "moz-2d" but can use normal "2d" also,
or whatever.  Simple element fallback doesn't handle any of these cases.

Script-based fallback by detecting exactly what's supported context-wise, and
then twiddling the DOM based on the individual script's requirements is the only
sane fallback, IMO.
(In reply to comment #13)
> No, I'm assuming pretty much that -- for example, I see line graphs as being
> fairly easy to generate client-side using canvas.  Your fallback would in
> theory have to be a server-side generated line graph image; what's the point
> of having the canvas then?

It uses less bandwidth, requires less server load, and can be resized dynamically.

Even when the canvas is animated, fallback will still be very useful:
-- In many cases falling back to a static display will convey basically the same
information, just in a less compelling way
-- You can fall back to an animated GIF if you want to (it won't resize and it
will suck bandwidth, but it will do the job)
-- You can fall back to Flash (heavier-weight, less well integrated into the
page, but it will work too)

> Fallback for <canvas> by itself is worthless in any case -- you need fallback
> for the specific context.  The assumption being made in this bug is that
> <canvas> == 2D, but right now you can get a <canvas> with support for NO
> contexts (if you build moz without cairo).  The element would be perfectly
> valid, but you'd get no fallback, and any script expecting to get a context
> back will just get bakc null.  You also might prefer a 3d context, but can get
> by with a 2d contenxt -- or you might prefer "moz-2d" but can use normal "2d"
> also, or whatever.  Simple element fallback doesn't handle any of these cases.

If there's no 2D context available, then we should treat <canavs> as completely
unsupported.

BTW, has anyone considered writing an IE behaviour that implements <canvas> on
top of Flash?
(In reply to comment #14)
> If there's no 2D context available, then we should treat <canavs> as completely
> unsupported.

And fallback to its contents, imho. Just like we do with the OBJECT element.
This is also what older UAs would do we do not support CANVAS unless we do not
fix this bug of course; then they will have nothing to fallback to.
No longer blocks: 288714
The fallback problems here are all very good reasons against a CANVAS element.
If it is just an interface that has to choose an element to replace, you've got
automatic fallback when script is disabled or no context is available.
I'm not sure what people mean by "if there is no 2D context". We can't just
switch between displaying and not displaying just on the fly as the script
creates contexts, if we did that we'd end up with lots of flicker at odd times,
as well as requiring authors to spawn a context immediately (without waiting).

Vlad has a point, but I still think that declarative fallback is worthwhile,
even if just for the no-script case. The case where <canvas> is supported but
the context isn't can be handled by the script itself.
Testcase to check if </canvas> works with Safari:
   http://www.hixie.ch/tests/adhoc/html/canvas/001.html

For handling the script fallback case, we might want a .showFallback() method on
the HTMLCanvasElement that forces the canvas to show itself as fallback instead.
Typical scenario would be:

   var c = canvas.getContext('3d');
   if (!c) {
      canvas.showFallback();
      return;
   }
I think that by "there is no 2D context" people mean that the brower doesn't
support "2d" as argument to getContext. This isn't allowed by spec, but can
currently happen in mozilla.

There is a problem here that 'supporting' canvas might not be enough for a page,
that most likly it relies on a specific context being available too.

I think hixies suggestion in comment 18 seems like a good idea. An alternative
might be a declerative approach like
<canvas require-context="3d funky">
This would use fallback if neither a '3d' or 'funky' context is available. This
argument would either use '2d' as default value, or a '2d' context would always
be required.

Though I think I like hixies suggestion better since it would allow things like
requireing two contexts.
(In reply to comment #17)
> I'm not sure what people mean by "if there is no 2D context". We can't just
> switch between displaying and not displaying just on the fly as the script
> creates contexts, if we did that we'd end up with lots of flicker at odd
> times, as well as requiring authors to spawn a context immediately (without
> waiting).

Of course I didn't mean that. I meant what vlad said here:
> The assumption being made in this bug is that
> <canvas> == 2D, but right now you can get a <canvas> with support for NO
> contexts (if you build moz without cairo).

But Jonas pointed out that the spec already doesn't allow that. So basically the
spec already says what I wanted it to say: if you don't support "2d", then you
can't support <canvas> at all. (And Mozilla will conform as soon as we
--enable-cairo everywhere.) Sorry for the confusion!
The spec does not disallow not supporting the 2d context; it merely defines a 2d
context.   (Or rather, the spec doesn't require support for any particular
context, it just defines a set of standard contexts that may be supported.)

We really have two reasonable options here:

1) We require a </canvas>.  Anything in between is fallback content (deciding
when to use fallback should be a separate discussion from this).  Safari will
correctly ignore child elements of <canvas></canvas>.  Thus, <canvas> uses that
work in Gecko will work in Safari.  However, apps that work on Safari (without
</canvas>) will require a small modification to work in Gecko.  All
Apple-provided documentation regarding canvas does not include a </canvas>, so
there is potential for confusion.  We'd have to loudly publicize our canvas
support and "best cross-platform practices" in <canvas>.

2) We require no </canvas> and have no support for fallback content.  We end up
having perfect compatability with Safari, but have no option for fallback.  With
our current htmlparser, it's not possible to specify that </canvas> is optional
(well, it is, but it'll only kick in once the first unexpected closing tag is
found, which will almost always result in an incorrect DOM tree).

(In reply to comment #21)
> The spec does not disallow not supporting the 2d context; it merely defines a
> 2d context.   (Or rather, the spec doesn't require support for any particular
> context, it just defines a set of standard contexts that may be supported.)

It says:
> This specification only defines one context, with the name "2d". If
> getContext() is called with that exact string, then the UA must return a
                                                             ^^^^
> reference to an object implementing CanvasRenderingContext2D.

Am I misunderstanding something?

> 1) We require a </canvas>.  Anything in between is fallback content (deciding
> when to use fallback should be a separate discussion from this).  Safari will
> correctly ignore child elements of <canvas></canvas>.  Thus, <canvas> uses
> that work in Gecko will work in Safari.  However, apps that work on Safari
> (without </canvas>) will require a small modification to work in Gecko.  All
> Apple-provided documentation regarding canvas does not include a </canvas>, so
> there is potential for confusion.  We'd have to loudly publicize our canvas
> support and "best cross-platform practices" in <canvas>.

In reality, most Web developers and users will try it with Gecko first. The rest
will try it with Gecko second. The few developers who develop and test only in
Safari can be ignored.
(In reply to comment #22)

> It says:
> > This specification only defines one context, with the name "2d". If
> > getContext() is called with that exact string, then the UA must return a
>                                                              ^^^^
> > reference to an object implementing CanvasRenderingContext2D.
> 
> Am I misunderstanding something?


LAMESPEC. ;)  The two paragraphs that follow indicate that the method should
return null if a requested context isn't supported; the section you quoted
should say something like "then the UA must either return null to indicate that
2D is not supported, or it must return a reference to an object implementing the
CanvasRenderingContext2D described here."

> In reality, most Web developers and users will try it with Gecko first. The rest
> will try it with Gecko second. The few developers who develop and test only in
> Safari can be ignored.

I'd agree with this; chances are anyone developing and testing with Safari will
already be testing with Firefox.  Someone developing and testing with IE only
won't be affected either.

So, </canvas> it is.  Patch shortly.
The spec isn't lame, it's correct.

If you want to claim support for all of that spec, i.e. if you want to be fully
compliant, then you have to support all the contexts it defines, just like you
have to support all the other features it defines. This includes '2d'. Thus, you
"must" return a '2d' context when asked for one.

But if you _don't_ support '2d', then you must return null. (You're also not a
fully compliant implementation, but that's another story.) Thus the behaviour of
partially-implemented UAs is also defined.

It would make no sense to make all the features in the spec optional, since then
you could just implement nothing and claim to be conformant, which is obviously
a useless claim as far as Web authors are concerned. They don't care about
technicalities, they want browsers that implement everything so they know what
they can reliably use.


I propose we start with <canvas></canvas>, and document the "canvas + .fallback"
trick for compatibility with Safari. If we find that a lot of authors write
content that only works with them and not us, then we can change our behaviour
in a later version. Going from with-tag to no-tag is possible. Going the other
way would be very painful.
Attached patch fix empty element (obsolete) — Splinter Review
This patch makes canvas a non-empty element, with the </canvas> tag as
required.  It also adds a default width/height, and begins to fix some of the
CSS property interactions.  The Frame now also properly queries the size of the
content during reflow, and not during init.
Comment on attachment 182726 [details] [diff] [review]
fix empty element

>Index: layout/style/html.css
>+/* children of canvas are part of fallback content and should not be displayed */
>+canvas > * { 
>+   display: none;

This doesn't make me so happy, since it means style resolution has to check the
parent of every element.  In any case, non-HTML children would still be
displayed given this CSS, which seems wrong.

I wish I had the leaf frame stuff working automagically in frame construction;
failing that, could you please handle it the way it's handled for <object> for
now?  Search on ::object in the frame constructor to see how.

>Index: layout/style/nsCSSKeywordList.h

Did you really mean to include all the box-orient stuff here?  I Thought I
already reviewed that once!  ;)

>Index: content/html/content/src/nsHTMLCanvasElement.cpp

>+NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLCanvasElement, Width, width, DEFAULT_CANVAS_WIDTH)

So we really want canvas.width to be 300 for a canvas with no width attribute
set?  Is this specified somewhere?
> MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
>                       nsRuleData* aData)
> {
>+  nsGenericHTMLElement::MapImageSizeAttributesInto(aAttributes, aData);
>   nsGenericHTMLElement::MapCommonAttributesInto(aAttributes, aData);
> }

So we map in common attrs, width, height.

> nsHTMLCanvasElement::IsAttributeMapped(const nsIAtom* aAttribute) const
> {
>   static const MappedAttributeEntry* const map[] = {
>     sCommonAttributeMap,
>+    sImageMarginSizeAttributeMap

But we claim dependence on common, width, height, vspace, hspace?

Why the latter two?

> nsHTMLCanvasElement::ParseAttribute(nsIAtom* aAttribute,

This is still using ParseImageAttribute.  Should it really be doing that? 
Doesn't seem like it should....

> nsHTMLCanvasElement::GetContext(const nsAString& aContextId,
>                                 nsISupports **aContext)
> {
>+  nsIntSize sz = GetWidthHeight();

sz is unused, no?

>+    fprintf (stderr, "UpdateImageContainer: recreating frame %d %d -> %d %d\n",

You aren't checking that in, are you?  ;)

>+  if (!mImageContainer) {
>+    rv = UpdateImageContainer();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
> 
>   NS_IF_ADDREF(*aImageContainer = mImageContainer);

Can mImageContainer really be null there?
Attachment #182726 - Flags: review?(bzbarsky) → review-
The default dimensions are in the spec. You are checking the spec when reviewing
this right? :-)

   http://whatwg.org/specs/web-apps/current-work/#canvas
Default values for width and height is indeed specified in the spec. I actually
think that because of this you don't want to use NS_IMPL_INT_ATTR_DEFAULT_VALUE
since we want to return the default value when width='0' is in the markup.

Won't the new default values make you create an image frame a bunch of times
during initial parsing of the element? Remember that we will first create the
element and then call SetAttr on it for every attribute during parsing. You
should probably lazily create the image-holder so that either getContext or
reflow will create it.


   if ((value = GetParsedAttr(nsHTMLAtoms::width))) {
     if (value->Type() == nsAttrValue::eInteger) {
       size.width = value->GetIntegerValue();
-    } else if (value->Type() == nsAttrValue::eString) {
-      k = value->GetStringValue().ToInteger(&rv);
-      if (NS_SUCCEEDED(rv))
-        size.width = k;
     }
   }

Please combine the if-statements into one.
Ok, new patch, thanks to bz's extensive help (thanks!)

- Canvas is a non-empty element, and </canvas> is required.
- Default canvas has a size of 300x200, if not specified.
- width/height attributes on canvas are NOT mapped to CSS -- the attributes
specify the pixel size of the canvas, whereas the CSS properties specify the
size of the region the canvas is rendered to.  So, <canvas width="50"
height="50" style="width:200px; height:200px;"></canvas> gives a 50x50 canvas
that gets scaled up to 200x200 for rendering.
Attachment #182726 - Attachment is obsolete: true
Attachment #182829 - Flags: review?(bzbarsky)
Comment on attachment 182829 [details] [diff] [review]
canvas non-empty element, style, css width/height fixes

>+    /*parent,incl,exclgroups*/          kSpecial, (kFlowEntity|kInlineEntity|kSelf), kNone,

I mentioned this on IRC, but kFlowEntity is defined in nsElementTable.h to be
(kBlockEntity | kInlineEntity), making the kInlineEntity here redundant. Could
you please take it out when checkin in?
Comment on attachment 182829 [details] [diff] [review]
canvas non-empty element, style, css width/height fixes

None of my comments in comment 24 seems to be addressed.

Additionally I _do_ think that width and height should be mapped. How else are
you going to get the right size for something like:

<canvas width=20 height=20>

?

Also, why do you map the margin attributes in this patch?
Attachment #182829 - Flags: review?(bzbarsky) → review-
To clarify: Even if you map width and height you'll get the right size for

<canvas width="50" height="50" style="width:200px; height:200px;">

Since the style-rule will override the values in the attributes. The cascade
will take care of that. Even a stylerule in an attached stylesheet that looks like

canvas { width: 200px; height 200px; }

will override values specified in mapped attributes.
Removed kInlineEntity, coalesced nested if statements.

Width and height are explicitly not mapped because it causes a problem if the
style is changed for, e.g. printing.  The canvas cannot redraw itself without
outside input, and on resize, the only option it has is to clear the canvas.

I'm not sure what the difficulty is with <canvas width=20 height=20></canvas>;
you'll get a 20px by 20px canvas, because the intrinsic size is 20x20.
Attachment #182829 - Attachment is obsolete: true
Attachment #182833 - Flags: review?(bzbarsky)
Ok, i don't quite understand the mapping problem, but if bz says that this is
the way to do it i'll take his word for it. There's still the first two comments
in comment 28 though.
Comment on attachment 182833 [details] [diff] [review]
updated patch for non-empty element, style, css width/height fixes

>Index: layout/base/nsCSSFrameConstructor.cpp

>@@ -8479,17 +8482,18 @@ nsCSSFrameConstructor::ContentAppended(n

>+      frameType == nsLayoutAtoms::canvasFrame) {
>     // This handles APPLET, EMBED, OBJECT and COL

and CANVAS?  ;)

>@@ -9066,17 +9070,18 @@ nsCSSFrameConstructor::ContentInserted(n

>+        frameType == nsLayoutAtoms::canvasFrame) {
>       // This handles APPLET, EMBED, OBJECT and COL

Same.

>Index: layout/generic/nsHTMLCanvasFrame.cpp

> nsHTMLCanvasFrame::Init(nsPresContext*   aPresContext,

Can't you just stop implementing this method altogether, if it just forwards to
your superclass and does nothing else?

r+sr=bzbarsky with those nits picked.
Attachment #182833 - Flags: superreview+
Attachment #182833 - Flags: review?(bzbarsky)
Attachment #182833 - Flags: review+
Verbal a=brendan/shaver; this patch also fixes some additional bugs, will
resolve those as well.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
So I'm not sure exactly what the spec does but it isn't clear to me that the
semantics described in the current spec are what was implemented.

The spec describes a world wherein the attributes give the coordinate space of
the canvas' contexts, irrespective of the final rendered size. The CSS would
initially have its height and width mapped from the attributes but would, if set
to another value, override the _rendered_ dimensions of the canvas.

So if I say:

   <canvas height="10" width="10"/>

...but have:

   canvas { height: 20em; width: 20em; }

...the API calls will all be using units in the range 0..10, but the actual
rendering would always be in a square 20 times the font-size.

(The rendering would always be to a bitmap that is of a high enough resolution
that it maps to device pixels, of course; that is a further concern totally
separate from these.)
(In reply to comment #29)
> Created an attachment (id=182829) [edit]
> - Default canvas has a size of 300x200, if not specified.

Vlad, the spec at http://whatwg.org/specs/web-apps/current-work/#canvas says
that the "width attribute defaults to 300, and the height attribute defaults to
150." Could you make a followup patch that fixes this?
I filed bug 293225 for comment 39 (with patch). I guess comment 38 needs a
separate bug too if it really is a bug.
My comments in comment 28 _still_ hasn't been addressed! Along with the
following comment from bz:

> > nsHTMLCanvasElement::ParseAttribute(nsIAtom* aAttribute,

> This is still using ParseImageAttribute.  Should it really be doing that? 
> Doesn't seem like it should....

Also, merged if-statements use wrong whitespace:

if (..)
{
    <code here>
}

rather then

if (..) {
  <code here>
}

Note indentation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
ImageFrame creation issues should be resolved with the the patch in bug 293407.
I want to resolve this one way or another ASAP. Vlad, is this patch ready for
approvals?
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Whiteboard: [bs] has patch (vlad) with reviews, need status
The patch already received approvals and landed; not sure why this was reopened.
 Canvas is not an empty element.
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [bs] has patch (vlad) with reviews, need status
Indeed. We still return the wrong value for .width and .height when an illigal
value is given in the markup. And it'd be great if hixie could voice his
oppinion on which, if any, attributes should be mapped into style.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug is FIXED.  If there are issues to address, tacking them onto the end of
this bug is not a productive way to get them fixed.  Please file new bugs, so
that each reported defect or desired enhancement can be appropriately
prioritized, marked blocking as appropriate, and fixed.

Unless <canvas> becomes an empty element again, this bug should not be reopened.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
For the record, i'm not tacking this onto the end, these were comments I made
during reviewing that were ignored.
Yes, but trying to resolve them in the context of this bug requires any action
taken to be tacked onto this bug.  And your comments themselves were not related
to the subject of the bug (lack of support for fallback in <canvas>, admirably
resolved by vlad, bz, hixie and others), so that they weren't directly addressed
is not a reason to REOPEN the bug.  If you think there's a defect in canvas, you
should file a bug about that defect.  (Anne did, and that defect was resolved.)

I don't know, myself, why your concerns weren't responded to here.  But that has
nothing to do with the resolution of this bug; it was patched, reviewed,
committed, and FIXED.  And FIXED it shall remain.
(In reply to comment #49)
> I don't know, myself, why your concerns weren't responded to here.

That was my fault, and I apologize; I thought we went over the issues on irc,
but I could be pretty badly misremembering -- and either way should have
commented the resolution/discussion here in any case.  There were a lot of
things up in the air with canvas at the time, and for some things (like mapping
'margin') it seemed simpler to leave them as they are until the dust settled.
> it'd be great if hixie could voice his opinion on which, if any, attributes...

Please cc me on the relevant bug and I shall be happy to do so. I'm not really
sure what the question is at this point.
You need to log in before you can comment on or make changes to this bug.