Jesse Ruderman Reporter
	Description • 8 years ago

Assertion failure: !Failed(), at dist/include/mozilla/ErrorResult.h:105

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 2 • 8 years ago

Jessica, could you please take a look?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 8 years ago

(It would be nice to submit this test case to wpt, rather than just landing the crashtest.)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 4 • 8 years ago

This assertion was added in Bug 1224007 part 6.

In this case, it fails in SetCaption -> AppendChild because the caption is not allowed as child due to 'host-including inclusive ancestor'. Since ErrorResult is not used originally, I changed it to IgnoredErrorResult.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 5 • 8 years ago

I am not sure if we should add the test case to wpt, since it's an assertion that we added. The spec doesn't say much about failure on setting the caption. Please correct me if I am wrong.


[1] https://html.spec.whatwg.org/multipage/tables.html#dom-table-caption

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 8 years ago

Comment on attachment 8772678 [details] [diff] [review]
patch, v1.

The spec should definitely be clearer here (please file a spec issue).  What do other browsers do?  Do they silently no-op, or do they throw?  Because rethrowing the append failure would make a _lot_ of sense.....

Either way, there should be a wpt test here: either this operation should throw or it should not, and either way the caption should NOT end up as the child of the table, and this all should be tested in wpt.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 7 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8772678 [details] [diff] [review]
> patch, v1.
> 
> The spec should definitely be clearer here (please file a spec issue).  What
> do other browsers do?  Do they silently no-op, or do they throw?  Because
> rethrowing the append failure would make a _lot_ of sense.....

Tested Chrome, and it does throw, but interestingly, it has a comment in its IDL [1] saying:
// TODO(philipj): The caption, tHead and tFoot setters should never throw. 

In our case, we only throw a TypeError when the caption is not null nor a HTMLTableCaptionElement (webidl bindings check this).

I'll file a spec issue to clarify this a bit. Thanks.

> 
> Either way, there should be a wpt test here: either this operation should
> throw or it should not, and either way the caption should NOT end up as the
> child of the table, and this all should be tested in wpt.

I see, will add the test after clarifying whether we should throw or not.

[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/html/HTMLTableElement.idl#24

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 8 • 8 years ago

(In reply to Jessica Jong [:jessica] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > Comment on attachment 8772678 [details] [diff] [review]
> > patch, v1.
> > 
> > The spec should definitely be clearer here (please file a spec issue).  What
> > do other browsers do?  Do they silently no-op, or do they throw?  Because
> > rethrowing the append failure would make a _lot_ of sense.....
> 
> Tested Chrome, and it does throw, but interestingly, it has a comment in its
> IDL [1] saying:
> // TODO(philipj): The caption, tHead and tFoot setters should never throw. 
> 
> In our case, we only throw a TypeError when the caption is not null nor a
> HTMLTableCaptionElement (webidl bindings check this).
> 
> I'll file a spec issue to clarify this a bit. Thanks.

Filed https://github.com/whatwg/html/issues/1582

> 
> > 
> > Either way, there should be a wpt test here: either this operation should
> > throw or it should not, and either way the caption should NOT end up as the
> > child of the table, and this all should be tested in wpt.
> 
> I see, will add the test after clarifying whether we should throw or not.
> 
> [1]
> https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/
> Source/core/html/HTMLTableElement.idl#24

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 9 • 8 years ago

Propagating the error on setting HTMLTableElement.caption and added corresponding wpt.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 8 years ago

Comment on attachment 8773612 [details] [diff] [review]
patch, v2.

r=me.  Thank you!

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 11 • 8 years ago

philipj, FYI:

(In reply to Jessica Jong [:jessica] from comment #7)
> // TODO(philipj): The caption, tHead and tFoot setters should never throw.

	Philip Jägenstedt [:foolip]
	Comment 12 • 8 years ago

Thanks for pointing this out, I just added the TODO because I noticed it wasn't per spec at the time, I never investigated what would really make sense.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 8 years ago

Note that the spec now says to throw.

	Philip Jägenstedt [:foolip]
	Comment 14 • 8 years ago

It looks like the spec hasn't changed here since I added that TODO, so I was just wrong at the time, or maybe I just looked at the definition of the caption setter. Anyway, always rethrowing makes sense thanks for filing https://github.com/whatwg/html/issues/1582 Jessica!

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 15 • 8 years ago

(In reply to Philip Jägenstedt from comment #14)
> It looks like the spec hasn't changed here since I added that TODO, so I was
> just wrong at the time, or maybe I just looked at the definition of the
> caption setter. Anyway, always rethrowing makes sense thanks for filing
> https://github.com/whatwg/html/issues/1582 Jessica!

No problem. :)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 16 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=959d7dc042cc

	Pulsebot
	Comment 17 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b510c6b911a
Propagate exception on setting HTMLTableElement.caption. r=bz

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8b510c6b911a