Closed
Bug 125746
Opened 23 years ago
Closed 22 years ago
[FIX]HTMLStyleElement.innerHTML is horked
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.1beta
People
(Reporter: bc, Assigned: bzbarsky)
References
Details
(Keywords: testcase)
Attachments
(4 files, 3 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
sicking
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
Setting the innerHTML property of an HTMLStyleELement to 'foo' results in
innerHTML containing '<style></style>foo'
|
Reporter | |
Comment 1•23 years ago
|
||
|
Assignee | |
Comment 2•23 years ago
|
||
I think the most reasonable course of action is to drop the old sheet if any and
treat the new "html" as just stylesheet data... Will look into doing that.
In IE,innerHTML is read only on some elements.
STYLE is one of those elements.
Even having it as read only does not make much sense on STYLE
|
|
Assignee | |
Comment 4•23 years ago
|
||
Yeah, but this will be simple to fix once bug 34849 is fixed. :) Marking
dependent, pushing out to 1.0
Depends on: 34849
OS: Windows 2000 → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.0
|
|
Assignee | |
Comment 5•23 years ago
|
||
I'm going out of town in two days and not getting back till after 1.0 freeze.
Setting realistic milestones for all 1.0 bugs that depend on other bugs or on
possible interface work.
Any help on getting these fixed would be much appreciated...
Keywords: helpwanted
Target Milestone: mozilla1.0 → mozilla1.1alpha
|
||
Comment 6•23 years ago
|
||
Bug 34849 didn't fix this. The content model looks wrong though. After the
script has run I see
STYLE
|
+--STYLE
|
+--textnode with the style rule
That second STYLE element shouldn't be there I think. If we can fix that, the
style will be applied.
|
|
Assignee | |
Comment 7•22 years ago
|
||
1.1alpha is frozen. Unsetting milestone and will retriage in a few days when I
can make a realistic assessment of the situation.
Target Milestone: mozilla1.1alpha → ---
|
|
Assignee | |
Comment 8•22 years ago
|
||
Looks like an issue in nsHTMLFragmentContentSink::AddLeaf at first glance...
|
|
Assignee | |
Comment 9•22 years ago
|
||
OK. So there's a number of issues here:
1) <style>, <textarea>, <xmp>, <script>, <plaintext> are considered _leaf_
nodes. So the fragment sink unconditionally appends them....
2) The <endnote> mess used to denote the location of the context.
So the parser parses something like "<html><head><style><endnote>foo" (the
original testcase). It ends up calling AddLeaf() on the "<style>" and then
OpenContainer() on the <endnote> and AddLeaf() on the "foo", since apparently
the <endnote> causes the <style> to be closed out!
The <textarea> case is similar, except there <endnote> does not close out the
textarea but is instead treated as text inside the textarea.
So what do we want to do? We can disable setting innerHTML on the elements in
question, possibly (the container elements treated as leaves by the parser).
But createContextualFragment would still be broken for such elements.... Do we
want to try and fix that? Ccing akkana, since that's where the CVS blame points
for the initial ParseFragment() impl.
It looks like editor's cut/paste is looks like the only real user of
CreateContextualFragment.... Therefore, simply disabling it for the elements in
question should be OK, since no one will be pasting into any of the elements in
question in our editor, as far as I can tell (though not so sure about <xmp> and
<plaintext>).
Thoughts?
|
|
Assignee | |
Comment 10•22 years ago
|
||
Sets up the future XMLFragmentSink stuff a bit too...
|
|
Assignee | |
Comment 11•22 years ago
|
||
Comment on attachment 88281 [details] [diff] [review]
Proposed fix v 1.0
This breaks some copy/paste stuff in composer.... Will tweak to fix that.
Attachment #88281 -
Flags: needs-work+
|
|
Assignee | |
Comment 12•22 years ago
|
||
|
|
Assignee | |
Comment 13•22 years ago
|
||
This should be much better
Attachment #88281 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•22 years ago
|
Comment on attachment 89390 [details] [diff] [review]
Proposed patch v 2.0
>+nsresult
>+nsGenericHTMLContainerElement::ReplaceContentsWithText(const nsAString& aText,
>+ PRBool aNotify) {
>+ PRInt32 children;
>+ nsresult rv = ChildCount(children);
>+ if (NS_SUCCEEDED(rv)){
Do NS_ENSURE_SUCCESS(rv, rv) instead.
>+ PRInt32 i;
>+ for (i = children - 1; i >= 0; --i) {
>+ RemoveChildAt(i, aNotify);
>+ }
>+ }
>+
>+ if (!aText.IsEmpty()) {
>+ nsCOMPtr<nsIContent> text;
>+ rv = NS_NewTextNode(getter_AddRefs(text));
>+ if (NS_SUCCEEDED(rv)) {
use NS_ENSURE_TRUE here too.
>+ nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text);
>+ if (tc) {
IMHO you don't need the if(tc) check.
It might be nice to make nsHTMLScriptElement::(S|G)etText and
nsHTMLTextAreaElement::(S|G)etDefaultValue use your new functions. Have a look
at the implementation of nsHTMLTextAreaElement::SetDefaultValue which is a bit
more optimized then your implementation.
We could also use GetContentsAsText in nsStyleLinkElement, which doesn't
inherit anything except a couple of interfaces. So if that function lived as a
static in some helperclass then we'd be able to use it there too. Feel free to
do as you please.
>+nsresult
>+nsGenericHTMLContainerElement::GetContentsAsText(nsAString& aText)
>+{
>+ aText.Truncate();
>+ PRInt32 children;
>+ nsresult rv = ChildCount(children);
>+ nsCOMPtr<nsIDOMText> tc;
>+ nsCOMPtr<nsIContent> child;
>+ nsAutoString textData;
>+ if (NS_SUCCEEDED(rv)){
NS_ENSURE_TRUE :)
>+ PRInt32 i;
>+ for (i = 0; i < children; ++i) {
>+ ChildAt(i, *getter_AddRefs(child));
>+ tc = do_QueryInterface(child);
>+ // XXX we should really consider asserting that tc is non-null here..
IMHO no, someone could add a non-textchild using the DOM, we don't want to
assert in that case. assertions should be for fatal conditions and we defenetly
don't wanna be fatal then.
>+protected:
>+ nsresult ReplaceContentsWithText(const nsAString& aText, PRBool aNotify);
>+ nsresult GetContentsAsText(nsAString& aText);
please add a comment to the functions. Especially comment on the fact that
GetContentsAsText doesn't do a deep walk.
>+NS_IMETHODIMP
>+nsHTMLScriptElement::GetInnerHTML(nsAString& aInnerHTML)
>+{
>+ return GetText(aInnerHTML);
>+}
>+
>+NS_IMETHODIMP
>+nsHTMLScriptElement::SetInnerHTML(const nsAString& aInnerHTML)
>+{
>+ return SetText(aInnerHTML);
>+}
For the sake of consistency it might be nice to use
GetContentsAsText/ReplaceContentsWithText (same thing in textarea)
|
|
Assignee | |
Comment 15•22 years ago
|
||
Attachment #89390 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 16•22 years ago
|
||
Attachment #89859 -
Attachment is obsolete: true
Comment on attachment 89867 [details] [diff] [review]
Pre-address some more comments
r=me
Attachment #89867 -
Flags: review+
|
|
Assignee | |
Updated•22 years ago
|
Summary: HTMLStyleElement.innerHTML is horked → [FIX]HTMLStyleElement.innerHTML is horked
|
||
Comment 18•22 years ago
|
||
Comment on attachment 89867 [details] [diff] [review]
Pre-address some more comments
- In nsGenericHTMLContainerElement::ReplaceContentsWithText():
+ if (!textChild) {
...
+ } else {
+ rv = textChild->SetData(aText);
This ignores the aNotify flag, do you want to QI to nsITextContent and call
SetText() on that so that you can pass in the aNotify flag?
Other than that, sr=jst
Attachment #89867 -
Flags: superreview+
|
|
Assignee | |
Comment 19•22 years ago
|
||
Per discussion with jst, leaving that part as-is and checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 20•22 years ago
|
||
Also per discussion filed bug 155723
|
||
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
|
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•