Closed Bug 291216 Opened 20 years ago Closed 20 years ago

canvas should not be an empty element

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: annevk, Assigned: vlad)

Details

Attachments

(1 file, 2 obsolete files)

updated patch for non-empty element, style, css width/height fixes
24.26 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
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: 20 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: 20 years ago → 20 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: 20 years ago → 20 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.

Attachment

General

Creator:
Created:
Updated:
Size: