Closed
Bug 1282894
Opened 9 years ago
Closed 9 years ago
"Assertion failure: !Failed()" - mozilla::dom::HTMLTableElement::SetCaption
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: jruderman, Assigned: jessica)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
Assertion failure: !Failed(), at dist/include/mozilla/ErrorResult.h:105
Reporter | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Comment 3•9 years ago
|
||
(It would be nice to submit this test case to wpt, rather than just landing the crashtest.)
Assignee | ||
Comment 4•9 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.
Assignee | ||
Comment 5•9 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
Assignee | ||
Updated•9 years ago
|
Attachment #8772678 -
Flags: review?(bzbarsky)
![]() |
||
Comment 6•9 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.
Flags: needinfo?(jjong)
Assignee | ||
Comment 7•9 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
Assignee | ||
Comment 8•9 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
Flags: needinfo?(jjong)
Assignee | ||
Comment 9•9 years ago
|
||
Propagating the error on setting HTMLTableElement.caption and added corresponding wpt.
Attachment #8772678 -
Attachment is obsolete: true
Attachment #8772678 -
Flags: review?(bzbarsky)
Attachment #8773612 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•9 years ago
|
||
Comment on attachment 8773612 [details] [diff] [review]
patch, v2.
r=me. Thank you!
Attachment #8773612 -
Flags: review?(bzbarsky) → review+
Comment 11•9 years ago
|
||
philipj, FYI:
(In reply to Jessica Jong [:jessica] from comment #7)
> // TODO(philipj): The caption, tHead and tFoot setters should never throw.
Comment 12•9 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.
![]() |
||
Comment 13•9 years ago
|
||
Note that the spec now says to throw.
Comment 14•9 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!
Assignee | ||
Comment 15•9 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. :)
Assignee | ||
Comment 16•9 years ago
|
||
Keywords: checkin-needed
Comment 17•9 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b510c6b911a
Propagate exception on setting HTMLTableElement.caption. r=bz
Keywords: checkin-needed
Comment 18•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•