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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mrbkap, Assigned: mrbkap)
References
Details
Attachments
(1 file, 2 obsolete files)
12.58 KB,
patch
|
jst
:
review+
peterv
:
superreview+
asa
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•19 years ago
|
||
Comment on attachment 185374 [details] [diff] [review] patch v1 Johnny, what do you think?
Attachment #185374 -
Flags: review?(jst)
Assignee | ||
Comment 3•19 years ago
|
||
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
Assignee | ||
Comment 4•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #185374 -
Flags: review?(jst)
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
(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.
Assignee | ||
Comment 7•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #185514 -
Flags: review?(jst)
Comment 8•19 years ago
|
||
Comment on attachment 185692 [details] [diff] [review] patch v3 r=jst
Attachment #185692 -
Flags: review?(jst) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #185692 -
Flags: superreview?(peterv)
Updated•19 years ago
|
Attachment #185692 -
Flags: superreview?(peterv) → superreview+
Assignee | ||
Comment 9•19 years ago
|
||
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?
Updated•19 years ago
|
Attachment #185692 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 10•19 years ago
|
||
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.
Description
•