Closed Bug 296677 Opened 19 years ago Closed 19 years ago

nsParser::ParseFragment screws up setting <textarea>'s innerHTML

Categories

(Core :: DOM: HTML Parser, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(1 file, 2 obsolete files)

As noted in bug 293162 comment 1, <BODY> gets added to the textarea when we
remove the hack that makes us treat the incoming innerHTML. This is because for
bug 283036, we append a <BODY> string to the context to flush tags out of CNavDTD.
Attached patch patch v1 (obsolete) — Splinter Review
This goes directly through the DTD instead of trying to add it at the tokenizer
level, which doesn't work for <textarea>. I almost think a FlushMisplaced()
function on the DTD would be cleaner, but this works too.
Status: NEW → ASSIGNED
Comment on attachment 185374 [details] [diff] [review]
patch v1

Johnny, what do you think?
Attachment #185374 - Flags: review?(jst)
I'm morphing this bug into the bug to make nsParser::ParseFragment() not screw
up on <textarea>. Sorry Johnny :-)
Summary: <BODY> gets added to fragments involving <textarea> → nsParser::ParseFragment screws up setting <textarea>'s innerHTML
Attached patch patch v2 (obsolete) — Splinter Review
This fixes things so that when bug 265367 is fixed, we'll be able to simply
remove nsHTMLTextAreaElement::GetInnerHTML and
nsHTMLTextAreaElement::SetInnerHTML.

This does everything patch v1 does, and adds a IgnoreTags() function to the
HTML fragment content sink interface that tells the interface to ignore N tags
from the content. Then, when we parse the context, we make sure that we got all
the way to the end of the context string, and if we didn't, we make a gigantic
leap of faith, and assume that there's exactly one tag left of the context
stack (note: this invarient actually does hold in the tokenizer, given a well
formed context string, which we'll have).
Attachment #185374 - Attachment is obsolete: true
Attachment #185514 - Flags: review?(jst)
Attachment #185374 - Flags: review?(jst)
Comment on attachment 185514 [details] [diff] [review]
patch v2

+
+  /**
+   * This method is used to tell the content sink that there are tags that are
+   * going to be in the content that should be ignored. This is mainly for
cases
+   * such as <textarea> in HTML, where the actual <textarea> tag gets added
+   * after the call to WillBuildContent().
+   *
+   * @param aDepth The number (depth) of tags that should be ignored.
+   */
+  NS_IMETHOD IgnoreTags(PRInt32 aDepth) = 0;

Maybe name this SetIgnoreTagsDepth()?

- In nsHTMLFragmentContentSink::OpenContainer():

+  if (mIgnoreDepth > 0) {
+    --mIgnoreDepth;
+    return result;
+  }

That seems reasonable, but don't you want to keep track of the current depth
too and make sure things are balanced (i.e. what should happen if we get a
ignore depth of 1 and we pares something like "<a><b>foo</b></a><c>...</c>"
should both a and c be ignored, or just a? I'd say this is a case we don't want
to support but we should have the code to throw the appropriate error if we do
see a case like this, no?

The rest looks fine to me.
(In reply to comment #5)
> That seems reasonable, but don't you want to keep track of the current depth
> too and make sure things are balanced (i.e. what should happen if we get a
> ignore depth of 1 and we pares something like "<a><b>foo</b></a><c>...</c>"
> should both a and c be ignored, or just a? I'd say this is a case we don't 

jst and I discussed this. The function is going to be renamed to
IgnoreFirstContainer() and commented to be the hack that it is.
Attached patch patch v3Splinter Review
As promised, this doesn't even pretend to add a generally useful function to
the fragment content sink.
Attachment #185514 - Attachment is obsolete: true
Attachment #185692 - Flags: review?(jst)
Attachment #185514 - Flags: review?(jst)
Comment on attachment 185692 [details] [diff] [review]
patch v3

r=jst
Attachment #185692 - Flags: review?(jst) → review+
Attachment #185692 - Flags: superreview?(peterv)
Attachment #185692 - Flags: superreview?(peterv) → superreview+
Comment on attachment 185692 [details] [diff] [review]
patch v3

This should probably go in as soon as possible. These are some of the last
changes required to make textarea's innerHTML be happy. They're about
medium-risk because of the dependence on lack of bugs in the tokenizer.
Attachment #185692 - Flags: approval1.8b3?
Attachment #185692 - Flags: approval1.8b3? → approval1.8b3+
Whiteboard: [checkin needed]
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Could this have caused the ~10ms Tp hit in btek?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: