Status

()

Core
Canvas: 2D
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
This patch moves the <canvas> implementation from XUL Box Object ghetto land
into a real HTML element, complete with hooks into DOM goodness and better paint
semantics, where we might actually not be slow as molasses on most platforms.

It's turned on whenever --enable-cairo is turned on, and builds with the in-tree
cairo (and, in theory, with any system cairo).  Stuart's working on making cairo
build on windows, because it's a tossup as to whether that will work right now.

The patch defines a few new things, along with associated sprinkled code to make
things like the html parser recognize <canvas> as a tag, to get the right DOM
ClassInfo for the various canvas interfaces, etc.

layout/generic/nsHTMLCanvasFrame -- a simple frame impl that can display a
single ImageFrame
dom/public/idl/html/nsIDOMHTMLCanvasElement.idl -- the base <canvas> element
content/html/content/src/nsHTMLCanvasElement.cpp -- <canvas> HTML element impl
content/canvas/* -- new content subdir composing the Canvas interfaces, and the
future place for future canvas context work (e.g. 3d)

Could use some reviews especially on the nsHTMLCanvasFrame and HTMLCanvasElement
impl.
(Assignee)

Comment 1

12 years ago
Created attachment 179340 [details] [diff] [review]
html-canvas.patch
Attachment #179340 - Flags: superreview?(dbaron)
Attachment #179340 - Flags: review?(pavlov)

Comment 2

12 years ago
The code to convert from an cairo_image_surface to a gfxIImageFrame isn't going
to work on windows or mac.  Windows needs the data to be upside down BGR and mac
needs XRGB (or is it RGBX?).  The other option here would be to get the
nsIRenderingContext's nsIDrawingSurface and QI it to platform specific versions
like nsIDrawingSurfaceWin, get the DC and create a windows cairo surface and
move_to(), show_surface() the image surface on to it.  That said, it doesn't
look like all platforms have their own drawing surface interface...
(Assignee)

Updated

12 years ago
Depends on: 288796
(Assignee)

Comment 3

12 years ago
Created attachment 179441 [details] [diff] [review]
html-canvas-1.patch

Patch fixes the content element - frame relationship to be more sane; things
like "display: none" and the like are correctly handled now, as well as resizes
of the canvas.	CSS colors are also correctly parsed now (requires separate
ParseColorString patch).

The image frame rendering bits pavlov pointed out aren't fixed yet; that's
coming up next.
Attachment #179340 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #179340 - Flags: superreview?(dbaron)
Attachment #179340 - Flags: review?(pavlov)
+SDK_XPIDLSRCS += nsIDOMHTMLCanvasElement.idl

that file seems to be missing from the patch?
And even if it were in the patch, it probably doesn't belong in SDK_XPIDLSRCS,
since that is only for frozen interfaces.
(Assignee)

Comment 6

12 years ago
Created attachment 179888 [details] [diff] [review]
html-canvas-2.patch

(Forgetting the -N option sucks)

More fixes, including drawImage support and handling of MACOSX/WIN
RGB_A8-only-not image formats.	Only thing missing now is pattern/gradient
objects.  Most of the code should be ready for review (especially frame/content
bits)
Attachment #179441 - Attachment is obsolete: true
+        aStr.Assign(NS_ConvertUTF8toUTF16(nsPrintfCString(100, "#%02x%02x%02x",

CopyUTF8toUTF16

Comment 8

12 years ago
In the cairo_* function calls, about half use |foo()| while the other half use
|foo ()|.  Aside from that, the patch looks correct.  I think we should get it
in the tree for some further testing.
Comment on attachment 179888 [details] [diff] [review]
html-canvas-2.patch

>Index: layout/generic/nsHTMLCanvasFrame.cpp
>+ * Portions created by the Initial Developer are Copyright (C) 1998

2004, perhaps?

>+ * Contributor(s):

Might want to do an "(original author)" line with email address.

>+  fprintf (stderr, "*** NS_NewHTMLCanvasFrame\n");
>+
>+  NS_PRECONDITION(aNewFrame, "null OUT ptr");
>+  if (nsnull == aNewFrame) {
>+    return NS_ERROR_NULL_POINTER;
>+  }

No need for this (especially the fprintf).

>+NS_INTERFACE_MAP_BEGIN(nsHTMLCanvasFrame)
>+  NS_INTERFACE_MAP_ENTRY(nsIFrame)
>+NS_INTERFACE_MAP_END
>+
>+NS_IMETHODIMP_(nsrefcnt) nsHTMLCanvasFrame::AddRef(void)
>+{
>+  NS_WARNING("not supported for frames");
>+  return 1;
>+}
>+
>+NS_IMETHODIMP_(nsrefcnt) nsHTMLCanvasFrame::Release(void)
>+{
>+  NS_WARNING("not supported for frames");
>+  return 1;
>+}

No need for any of this either (and the QueryInterface you inherit is better).

>+NS_IMETHODIMP
>+nsHTMLCanvasFrame::Destroy(nsPresContext* aPresContext)
>+{
>+  return nsSplittableFrame::Destroy(aPresContext);
>+}

No need for this either (doesn't change anything).

>+
>+NS_IMETHODIMP
>+nsHTMLCanvasFrame::Init(nsPresContext*   aPresContext,
>+                        nsIContent*      aContent,
>+                        nsIFrame*        aParent,
>+                        nsStyleContext*  aContext,
>+                        nsIFrame*        aPrevInFlow)
>+{
>+  nsICanvasElement *canvas;
>+  CallQueryInterface(aContent, &canvas);
>+  if (!canvas)
>+    return NS_ERROR_FAILURE;

This should probably be NS_ENSURE_TRUE so it gives an assertion.  Also, it's
perhaps better to check NS_SUCCEEDED() on the result of QueryInterface instead
of a null-check.


>+NS_METHOD
>+nsHTMLCanvasFrame::HandleEvent(nsPresContext* aPresContext,
>+                           nsGUIEvent* aEvent,
>+                           nsEventStatus* aEventStatus)
>+{
>+  return nsSplittableFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
>+}
>+
>+
>+NS_METHOD
>+nsHTMLCanvasFrame::GetCursor(const nsPoint& aPoint,
>+                        nsIFrame::Cursor& aCursor)
>+{
>+  return nsFrame::GetCursor(aPoint, aCursor);
>+}

These don't seem to serve any purpose either.

>Index: layout/generic/nsHTMLCanvasFrame.h

Same comments on extra methods as for the .cpp file.

>+  virtual void GetDesiredSize(nsPresContext* aPresContext,
>+                              const nsHTMLReflowState& aReflowState,
>+                              nsHTMLReflowMetrics& aDesiredSize);

No need for this to be virtual since you're not inheriting from nsLeafFrame. 
In fact, probably no need for it at all.

More later, perhaps...
And there are a bunch of places where you used NS_METHOD instead of NS_IMETHODIMP.
Blocks: 279908
(Assignee)

Comment 11

12 years ago
Created attachment 180613 [details] [diff] [review]
html-canvas-2.patch

Updated with dbaron's feedback, and also updated to latest tree changes.
Attachment #179888 - Attachment is obsolete: true
(Assignee)

Comment 12

12 years ago
Created attachment 180617 [details] [diff] [review]
html-canvas-3.patch

Forgot -N, again. Grr.
Attachment #180613 - Attachment is obsolete: true
(Assignee)

Comment 13

12 years ago
Will be landing this tonight, hopefully; any comments, please speak up :).. pav
cought a NS_IMETHODIMP, and there's also some extraneous #if 0's and fprintf's
in that patch that i'll take out before committing.  (Various people have looked
over bits and pieces of this as appropriate.)
What's the purpose of the nsICanvasRenderingContext interface? Couldn't you make
nsIDOMHTMLCanvasElement.getCanvas return an nsISupports and then just have
nsICanvasRenderingContext2D. The interface is less then ideal with one scripted
and one unscripted function.

Also nsICanvasRenderingContext2D should probably be named
nsIDOMCanvasRenderingContext2D to follow the naming convention of other DOM
interfaces. Also we do some magic on interfaces whos names start with 'nsIDOM'
which you probably want ('CanvasRenderingContext2D.prototype.foo = bar' would
work in script for example). The same applies to nsICanvasPattern and
nsICanvasGradient
sicking: I hate to be a name pest, but don't we also put nsIDOM-prefixed
*non-w3c-standard* interfaces in an nsIDOMNS-prefixed sub-space?

/be
+nsHTMLCanvasElement::GetContext(const nsAString& aContextId,
+                                nsICanvasRenderingContext **aContext)
+{
+  if (mCurrentContextId.IsEmpty()) {
+    nsCString ctxString("@mozilla.org/content/canvas-rendering-context;1?id=");
+    ctxString.Append(NS_LossyConvertUTF16toASCII(aContextId));
+
+    mCurrentContext = do_CreateInstance(nsPromiseFlatCString(ctxString).get());


Is this safe to do given that aContextId comes from script? I can't think of any
risks offhand, but it feels a bit scary to instantiate a contract-id partially
comming from script.
It seems we only do that when we want to extend objects that already have a w3c
interface, like nsIDOMNSRange. We havn't done it for nsIDOMWindow or
nsIDOMPluginArray. I'm not entierly sure when we want to use nsIDOM and when we
don't, i'd love to get a second oppinion from jst or peterv.
+    ctxString.Append(NS_LossyConvertUTF16toASCII(aContextId));

LossyAppendUTF16toASCII
(Assignee)

Comment 19

12 years ago
(In reply to comment #16)
> +    nsCString ctxString("@mozilla.org/content/canvas-rendering-context;1?id=");
> +    ctxString.Append(NS_LossyConvertUTF16toASCII(aContextId));
>
> Is this safe to do given that aContextId comes from script? I can't think of any
> risks offhand, but it feels a bit scary to instantiate a contract-id partially
> comming from script.

I /think/ it's ok, because we have a full contractid before the bit we just
append as the "id" element.  To be sure, we can probably limit aContextId to
[A-Za-z0-9_-].
The entire thing feels a bit overengineered though, are there really enough
frozen interfaces for anyone to actually write a plugin for this anyway? If not
a simple if(aContextId.EqualsLiteral('id') { foo = new nsContext2D() } would do
just as well IMHO. It doesn't matter that much to me though, as long as we're
not exposing ourselvs to security problems.
I think we're safe here also because we force it to QI to the
nsICanvasRenderingContext interface.  Anything with that contract-ID prefix
which QIs appropriately had better be safe to hand back, because that's the only
reason to have that contract ID.

If we had meaningful contract-ID parameterization, that might be worth worrying
about a bit more, but I think we're fine here, and finer still with a
[A-Za-z0-9_-] clamp.
nsICanvasRenderingContext is scriptable too though, but if we move Init to
nsICanvasRenderingContextInternal (which i think we should do anyway) we should
be fine.
As our long history with Java plugins has shown, people can write useful
extensions that are locked to a specific version or set of versions, without
needing the benefit of universally frozen interfaces.  I think XForms is in the
same situation.
(Assignee)

Comment 24

12 years ago
(In reply to comment #22)
> nsICanvasRenderingContext is scriptable too though, but if we move Init to
> nsICanvasRenderingContextInternal (which i think we should do anyway) we should
> be fine.

Yep, this is done; I got rid of nsICanvasRenderingContext entirely, there's just
Internal and 2D now.
(Assignee)

Comment 25

12 years ago
Created attachment 181013 [details] [diff] [review]
html-canvas-4.patch

Patch that will be landing shortly.

Changes:

- <canvas> is now always valid; the only thing that's enabled by --enable-cairo
is the 2D context itself.  This lets us do non-cairo contexts, and lets people
add in contexts via extensions.

- the context ID is clamped to [A-Za-z0-9_-]

- getContext() now returns nsISupports, and nsIDOMCanvasRenderingContext2D.idl
lives in dom/public/idl/canvas.

- no more nsI(DOM)CanvasRenderingContext; nsIDOMCanvasRenderingContext2D
inherits from nsISupports directly, and just implements the "canvas" attribute
that was on nsIDOMCanvasRenderingContext before.
Attachment #180617 - Attachment is obsolete: true
Comment on attachment 181013 [details] [diff] [review]
html-canvas-4.patch

Yay, a=me for 1.8b2.

/be
Attachment #181013 - Flags: approval1.8b2+
dbaron: (with reference to a question you asked on IRC) According to the WHATWG
spec, <canvas> has an end tag and the contents are used as fallback content.
This is critical for accessibility and important for backwards compatibility.

Apple didn't implement this; their <canvas> element has no end tag and renders
both the fallback content and the canvas. (They support alt="", but that's
useless as a backwards compatibility thing since it means browsers have to
support <canvas> in order to not support it, as it were, which is silly.)

I strongly suggest that we stick with what is in the spec, so that accessibility
is addressed and we have a backwards compatibility story. While there is little
to no content out there, we can still do the right thing and force Apple to
change their implementation.
Comment on attachment 181013 [details] [diff] [review]
html-canvas-4.patch

>Index: dom/src/base/nsDOMClassInfo.cpp
>===================================================================

>@@ -577,16 +582,18 @@ static nsDOMClassInfoData sClassInfoData
>   NS_DEFINE_CLASSINFO_DATA(HTMLBaseElement, nsHTMLElementSH,
>                            ELEMENT_SCRIPTABLE_FLAGS)
>   NS_DEFINE_CLASSINFO_DATA(HTMLBaseFontElement, nsHTMLElementSH,
>                            ELEMENT_SCRIPTABLE_FLAGS)
>   NS_DEFINE_CLASSINFO_DATA(HTMLBodyElement, nsHTMLElementSH,
>                            ELEMENT_SCRIPTABLE_FLAGS)
>   NS_DEFINE_CLASSINFO_DATA(HTMLButtonElement, nsHTMLElementSH,
>                            ELEMENT_SCRIPTABLE_FLAGS)
>+  NS_DEFINE_CLASSINFO_DATA(HTMLCanvasElement, nsHTMLElementSH,
>+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

ELEMENT_SCRIPTABLE_FLAGS
(Assignee)

Comment 29

12 years ago
Damnit, missed that when I moved the Canvas element to live with the rest of the
elements.  Will fix, thanks!

This landed tonight, much to the chagrin of some tinderboxes; I'll be fixing the
fallout/issues as they occur.
Comment on attachment 181013 [details] [diff] [review]
html-canvas-4.patch

Ian: What should happen when a canvas element is cloned? Should that also clone
the current image?

Vlad: A few things we talked about on irc that's still not changed:

nsHTMLCanvasElement::GetAttributeChangeHint is still there.

nsHTMLCanvasElement::GetWidthHeight no longer needs to deal with attributes of
type eString since you now implement ParseAttribute

Removing the width or height attribute won't clear the image. You need to
implement nsHTMLCanvasElement::UnsetAttr and make
nsHTMLCanvasElement::UpdateImageContainer nuke the current image rather then
just bailing if width/height is 0.

nsHTMLCanvasElement::IsAttributeMapped doesn't return true for width and
height.

Things i found in the new changes:

You might as well make mCurrentContext an
nsCOMPtr<nsICanvasRenderingContextInternal>. Just make sure you QI it (rather
then cast it) to nsISupports before you return it from getContext so that you
return the COM-correct pointer.

I'm not sure it's a great idea to support attribute-mapping for all attributes
that <img> supports. Many of them are just there for backwards compatibility
and there's obviously no such need for canvas. Ian, do you have an oppinion
here? What does apple do.


I also noticed that you need to move the new eDOMClassInfo_HTMLCanvasElement_id
down to the end of the enum. See the comment on line 199.
sicking: Cloning a <canvas> should just do a DOM clone, which always drops the 
non-XML data, as far as I can tell. As far as the DOM interface, according to 
the WHATWG spec HTMLCanvasElement inherits from HTMLImageElement, but 
HTMLImageElement doesn't have any of the presentational crap from yore. Some 
attributes in particular have special meaning for <canvas>, e.g. .src doesn't 
attribute-map to src="", it just returns a data: URI of a PNG that represents 
the current rendering (and does nothing on setting).
I'm not sure that there is any spec that says what should happen with non-DOM
data during a .clone actually. And it's even debatable whether the image is DOM
data in a canvas since you actually create it through DOM. I would think it
could be usefull to have the ability to copy <canvas> images using .clone.

Sorry, should have been more clear what i ment by mapped attributes. I was
talking about attributes that affects style, such as border="5", vspace="10" or
align="left". Should such attributes work on <canvas>. And if so, which ones?
All the ones that work on <img>?
> it could be usefull to have the ability to copy <canvas> images using .clone.

You can drawImage() one on the other.


As for mapping presentational attributes to style, who knows. I'd ideally say
support none of them.
I'm still not convinced that .clone should create an empty image, but this bug
probably isn't the best place to discuss this.

Wrt style-mapped attributes. I'd missed that HTMLCanvasElement inherits
HTMLImageElement. Since that's the case I think we should at least map 'border',
'hspace' and 'vspace' so that those DOM-properties behave like on <img>. As well
as 'width' and 'height' of course. Other then that I agree that we should
probably support as few as possible, meaning none.
(Assignee)

Comment 35

12 years ago
(In reply to comment #30)
> Vlad: A few things we talked about on irc that's still not changed:

Hmm, some of these may have been due to my misunderstanding:

> nsHTMLCanvasElement::GetAttributeChangeHint is still there.

Hmm, how else do I get the frame to reconstruct itself?  I thought we decided
that until things get rewritten to allow the reuse of the existing frame, that
leaving this in should be fine for now.

> nsHTMLCanvasElement::GetWidthHeight no longer needs to deal with attributes of
> type eString since you now implement ParseAttribute

I left that in to avoid rocking the boat; but if it's ensured now that I can't
get non-integers (as, I guess, it is with ParseAttribute) I'll take it out.

> Removing the width or height attribute won't clear the image. You need to
> implement nsHTMLCanvasElement::UnsetAttr and make
> nsHTMLCanvasElement::UpdateImageContainer nuke the current image rather then
> just bailing if width/height is 0.

Whoops, yes; forgot about this part.  Will fix shortly.

> nsHTMLCanvasElement::IsAttributeMapped doesn't return true for width and
> height.

Right; they're not mapped, so that we can support things like what roc suggested
in the future (specifying a different width/height for presentation using CSS).

> Things i found in the new changes:
> 
> You might as well make mCurrentContext an
> nsCOMPtr<nsICanvasRenderingContextInternal>. Just make sure you QI it (rather
> then cast it) to nsISupports before you return it from getContext so that you
> return the COM-correct pointer.

Will do.

> I also noticed that you need to move the new eDOMClassInfo_HTMLCanvasElement_id
> down to the end of the enum. See the comment on line 199.

Argh!  I completely didn't see that comment (in the middle of the enum, no less
;).  And I spent all the time moving it up into its alphabetical location in the
HTML Element block, too... I'll fix asap today.

Thanks!

(In reply to comment #31)
> .src doesn't 
> attribute-map to src="", it just returns a data: URI of a PNG that represents 
> the current rendering (and does nothing on setting).

hm, in combination with drawImage, isn't that a security problem? You can read
images from intranet servers that way and send it where you want...
(In reply to comment #35)
> > nsHTMLCanvasElement::GetAttributeChangeHint is still there.
> 
> Hmm, how else do I get the frame to reconstruct itself?  I thought we decided
> that until things get rewritten to allow the reuse of the existing frame, that
> leaving this in should be fine for now.

By making IsAttributeMapped return true for width/height we'll reflow the frame
as needed when the attribute changes, which I think will be enough for things to
work.

> > nsHTMLCanvasElement::GetWidthHeight no longer needs to deal with attributes 
> > of type eString since you now implement ParseAttribute
> 
> I left that in to avoid rocking the boat; but if it's ensured now that I can't
> get non-integers (as, I guess, it is with ParseAttribute) I'll take it out.

It's ensured that it'll be an integer if it can be parsed as such. But you still
need to check the datatype just in case someone sets width="hello" (in which
case we should treat it as if the attribute isn't set).

> > Removing the width or height attribute won't clear the image. You need to
> > implement nsHTMLCanvasElement::UnsetAttr and make
> > nsHTMLCanvasElement::UpdateImageContainer nuke the current image rather then
> > just bailing if width/height is 0.
> 
> Whoops, yes; forgot about this part.  Will fix shortly.

Actually, I just noticed that the spec says that there are default values for
width and height, so we should just use that if the attribute is missing. You
still need to implement UnsetAttr though, so that we revert to the default then.

You probably want to create the image-frame lazily though, so that we don't
create it a bunch of times during parsing of the element.

> > nsHTMLCanvasElement::IsAttributeMapped doesn't return true for width and
> > height.
> 
> Right; they're not mapped, so that we can support things like what roc 
> suggested in the future (specifying a different width/height for presentation
> using CSS).

Even with rocs suggestion we want the attributes to be mapped so that they act
as a default value if no css is specified. So in other words:

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

The first will give a 100x100 display, but in the second the stylerule will
override the mapped attributes and give a 50x50 display.

Makes sense?
So I'm trying to follow what actually landed here (which I gather is different
from the last patch in this bug?).  A few issues I see offhand:

1)  nsHTMLCanvasElement::GetWidthHeight seems to be doing unnecessary (and
    incorrect) work.  ParseAttr will ensure here that the width/height are
    stored as integers if they're valid nonnegative integers.  So this method
    shouldn't have to mess with strings.  The fact that it does means it'll
    return negative widths/heights if those are set; is that desirable
    Furthermore, since this isn't dealing with nscoords it should be using
    nsIntSize, not nsSize.
2)  If #1 is fixed, it looks like you can just use the standard "int attr"
    macros (NS_IMPL_INT_ATTR_DEFAULT_VALUE, to be exact) to implement
    {Get/Set}{Width/Height}.
3)  Could we please document the idl?  nsIDOMCanvasRenderingContext2D.idl is a
    perfect example of the sort of interface we _don't_ want to be checking in,
    imo.  We have far too many such written years ago and are now paying the
    prince in developer time and effort.  If desired I can write up a summary of
    issues with the documentation of this interface (all of which come down to
    "there isn't any", but I can point out all the places that I think should
    have some).  If this is supposed to be the same as the similarly named
    interface in the whatwg spec, the IDL should say so (and then the whatwg
    spec needs vast improvement; I can't see how what's specified there can
    possibly be implemented in a reasonable way without constantly asking Hixie
    what he means).
4)  Width and height don't seem to be mapped into style in the code that was
    checked in.  That's not good, because it sorta violates the way
    presentational attributes should work in CSS.
5)  Do we really need a framechange on width/height changes?  I would have
    thought a reflow would be sufficient.
6)  In GetContext(), do we really want to throw INVALID_ARG on OOM?
7)  The canvas frame seems to completely ignore CSS for sizing. Is this desired?
8)  The canvas frame should implement GetFrameName (from nsIFrameDebug).
9)  The canvas frame claims to be splittable, and reflows as if it's splittable,
    then completely flubs painting if it's actually split.  Either fix the
    painting, or don't make it splittable, please.
(Assignee)

Comment 39

12 years ago
Created attachment 181053 [details] [diff] [review]
canvas-fix-classinfo.patch

Fix DOM classinfo order
Comment on attachment 181053 [details] [diff] [review]
canvas-fix-classinfo.patch

IMO it'd be fine to just move the enum and let the code be in a more logical
order. r=me either way though.
Attachment #181053 - Flags: review+
Boris, most of the things you bring up is stuff that is already worked on
(though 8 and 9 were new to me).

I do defenetly agree about the context2D interface sucking rocks though. The
optional arguments to drawImage is simply not acceptible, and using nsIVariants
(or actually nsISupports which is what we map DOMObject to) is going to be a
pain in the a** for non-script users. But lets fight that out in bug 290845 that
I just filed.
> hm, in combination with drawImage, isn't that a security problem? You can read
> images from intranet servers that way and send it where you want...

Valid point, drawImage() should probably do a same-origin check.
Regarding what the <img> attributes should do on <canvas>: I really think they
should do nothing. The current text in the spec is:

| The alt DOM attribute shall return the same value as the textContent DOM 
| attribute. On setting it must do nothing.
| 
| The src attribute must return a data: URI containing a representation of the 
| image as a PNG file. [PNG] On setting it must do nothing.
|
| The useMap and isMap DOM attributes inherited from the HTMLImageElement 
| interface shall do the same as for img elements.
|
| All the other DOM attributes — align, border, hspace, vspace, longDesc, and 
| name — must do nothing on setting and return the empty string on getting.

It's not a problem is we don't implement useMap, isMap, and src for the first
(or even later) versions straight away -- but I really don't think we should
encourage the use of the presentational attributes. Those should IMHO just do
nothing. It would be bad to introduce more presentational crap in 2005.
Well, I do agree introducing presentational attributes in 2005 is bad. But i bet
that we'll end up getting bugs filed against us if there are DOM properties on
canvas that don't do anything. The alternative is to not let HTMLCanvasElement
inherit HTMLImageElement, but that'd uglify the nsIDOMCanvasRenderingContext2D
interface even more.
How about making HTMLImageElement and HTMLCanvasElement both inherit from a
common ancestor? Would that work?
Specwise it would, but we couldn't change that in mozilla since HTMLImageElement
is a frozen interface.
Changing the inheritance hierarchy in this way would affect ABI compatibility?
(even if the resulting interface was the same?)

Pity.

Ok, what else can we do? Do we really think people are going to try to use these
DOM attributes and complain when they do nothing?
I'm not sure whether the binary C++ ABI would break (it might depend on
platform). However i'm not sure if that is the only compatibility we're
concerned about. In general we don't want to change frozen interfaces at all and
we're talking about some pretty big surgery here. Even if the net sum of the
changes might land on zero.

But i'm also less happy about chaning W3C defined interfaces like this.

In the end I guess I just think that adding these presentational attributes
won't cause that much harm. HTML (and the current incarnation of XHTML) won't be
able to forbid using presentational attributes anytime soon anyway. And all
implementors should have code to do attribute mapping so it wouldn't be too big
of a burdon.

And yes, I do think that people would use these properites and complain about
them not working.

Comment 49

12 years ago
mozillaZine ( http://www.mozillazine.org/talkback.html?article=6461 ) is
reporting this as having been already checked in, which it doesn’t appear to be
here.

Anyway, is the <canvas> spec stable enough to land this on the trunk, given that
WhatWG spec is still in Working Draft and says "It is very wrong to cite this as
anything other than a work in progress. Do not implement this in a production
product. It is not ready yet! At all!"?
If there is even the slightest possibility of the spec on <canvas> changing, I
don’t think this should land on any of the production builds.
(In reply to comment #49)
> mozillaZine ( http://www.mozillazine.org/talkback.html?article=6461 ) is
> reporting this as having been already checked in, which it doesn’t appear to be
> here.
> 
> Anyway, is the <canvas> spec stable enough to land this on the trunk,

Yes.

We are turning canvas on -- it's already on by now.  Get over it.  Nothing in a
relatively new spec is set in concrete, but that does not prevent useful work
from being built on it.  This is how the Internet protocols, and then the web,
were built.  Welcome back to incremental innovation.  The lull of six years of
monopoly-induced stagnation is over.

/be
Changing the interface hierarchy of frozen interfaces is verboten.

Comment 52

12 years ago
Is it intended that the canvas element cannot be styled (yet)?
<canvas width="200" height="200" style="border: 1px solid black;">
doesn't get a border (using Firefox 2005-04-20 nightly)...
Please file a bug on that

Updated

12 years ago
Depends on: 291216
(Assignee)

Comment 54

12 years ago
Note: I'm going to close this bug shortly as the bulk of the canvas impl landed;
the issues remaining that are in this patch I'm going to split off into separate
bugs, so I can tackle them one at a time.  Please file any new issues into
separate bugs, Layout: Misc component, assign to me.  Thanks!
Please make a note here of all the bugs that you file

Comment 56

12 years ago
Bugs that have been filed so far regarding CANVAS:
 * bug 291216 - canvas should not be an empty element
 * bug 291218 - Canvas 'src' attribute
 * bug 291262 - styling the CANVAS element doesn't work
No longer depends on: 291216

Comment 57

12 years ago
 * bug 291285 - <canvas> with no width/height crashes
Component: Layout: Misc Code → Layout: Canvas
QA Contact: layout.misc-code → layout.canvas

Comment 58

12 years ago
 * bug 293219 - Canvas mixes up RGB order in fillStyle and strokeStyle properties
 * bug 293221 - CanvasRenderingContext2D has wrong default property values

Comment 59

12 years ago
 * bug 293225 - Default height of CANVAS should be 150
 * bug 293226 - Antialiasing uses wrong colors with strokeRect() on Canvas element
* bug 293239 - Canvas misdraws content using style.height
* bug 293244 - canvas drawImage needs sameorigin checks
* bug 293245 - Implement nsIDOMCanvasRenderingContext2D properly
* bug 293249 - Don't pretend to support canvas when cairo isn't built

Comment 64

12 years ago
bug 293248 - <canvas> not working in beast Installer builds

Comment 65

12 years ago
 * bug 293259 - Canvas should not have an opaque white background by default

Comment 66

12 years ago
 * bug 293267 - When called via onclick, canvas rendering is far slower than
called via onload
* Bug 293306 - canvas leaks lots of memory

*  Bug 293353 - alpha transparency renders wrong colors [Win]
*  Bug 293382 - canvas paints unrelated colors at borders of rotated content
(Assignee)

Comment 70

12 years ago
Resolving this bug; Canvas has its own "Core -> Layout: Canvas" component now,
so no need to keep track of canvas bugs here.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 290392
*** Bug 102285 has been marked as a duplicate of this bug. ***
The Reflow() impl checked in in this bug has a GetPrevInFlow() check which will always test false.  You should just remove that code....
You need to log in before you can comment on or make changes to this bug.