Closed
Bug 725069
Opened 13 years ago
Closed 13 years ago
Html Comments being shown in DOM as text nodes. Regression from firefox 9.0
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: firefox, Assigned: ehsan.akhgari)
References
Details
(Keywords: regression, Whiteboard: [qa+][qa!:11][qa!:12][qa!:13])
Attachments
(4 files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
roc
:
review+
akeybl
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.77 Safari/535.7
Steps to reproduce:
Example test page that alerts the body's firstChild value node, and alerts the body's second child.
<html>
<head>
<title></title>
<script>
function outputDocument() {
alert(document.body.firstChild.nodeValue);
alert(document.body.firstChild.nextSibling);
};
</script>
</head>
<body contenteditable="true" onload="outputDocument()">abc<!--helloworld-->bcd</body>
</html>
Actual results:
displays 'abchelloworldbcd' , null
Expected results:
should display 'abc', [object comment]
|
Reporter | |
Updated•13 years ago
|
Version: 8 Branch → 10 Branch
|
|
Reporter | |
Updated•13 years ago
|
Severity: normal → critical
|
|
Reporter | |
Comment 1•13 years ago
|
||
|
||
Updated•13 years ago
|
Attachment #595146 -
Attachment mime type: text/plain → text/html
|
|
||
Updated•13 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
QA Contact: untriaged → general
|
|
Reporter | |
Comment 2•13 years ago
|
||
Only occurs when contenteditable=true blocks.
|
|
Reporter | |
Updated•13 years ago
|
Severity: critical → normal
|
||
Updated•13 years ago
|
Keywords: regressionwindow-wanted
|
|
Reporter | |
Updated•13 years ago
|
Summary: Html Comments being down in DOM as text nodes. Regression from firefox 9.0 → Html Comments being shown in DOM as text nodes. Regression from firefox 9.0
|
||
Comment 3•13 years ago
|
||
Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/9fa62f76f1cf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111019 Firefox/10.0a1 ID:20111019031031
Fails:
http://hg.mozilla.org/mozilla-central/rev/311fdb9b38b7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111020 Firefox/10.0a1 ID:20111020031025
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9fa62f76f1cf&tochange=311fdb9b38b7
Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a86a80a91234
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111018 Firefox/10.0a1 ID:20111018125055
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb1c23bd837b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111018 Firefox/10.0a1 ID:20111018140655
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a86a80a91234&tochange=cb1c23bd837b
Suspected:
7ec3daabbf47 Ehsan Akhgari — Bug 688789 - Stop touching the frame tree to determine whether a node is editable or not; r=roc
|
|
||
Updated•13 years ago
|
status-firefox-esr10:
--- → affected
status-firefox10:
--- → affected
status-firefox11:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox10:
--- → ?
tracking-firefox11:
--- → ?
tracking-firefox12:
--- → ?
tracking-firefox13:
--- → ?
|
|
||
Updated•13 years ago
|
Blocks: 688789
Keywords: regressionwindow-wanted → regression
|
|
||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
Assignee | |
Comment 4•13 years ago
|
||
|
|
Assignee | |
Comment 5•13 years ago
|
||
Attachment #595239 -
Flags: review?(roc) → review+
Better get this onto branches...
|
||
Comment 7•13 years ago
|
||
Tracking for FF11 and up. We are going to build with our 10.0.1 chemspill tonight, so if there's any reason to think that this should be included, please email release-drivers ASAP.
To my knowledge, we don't know of any major website regressions caused by this regression. From reading this bug, my understanding is that any site running into this issue could change the HTML on their end.
|
|
Assignee | |
Comment 8•13 years ago
|
||
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)
[Approval Request Comment]
Regression caused by (bug #): 688789
User impact if declined: HTML comments might show up on websites.
Testing completed (on m-c, etc.): Landing on m-c right now, tested locally.
Risk to taking this patch (and alternatives if risky): It's extremely low risk. The change is well understood.
Attachment #595239 -
Flags: approval-mozilla-release?
Attachment #595239 -
Flags: approval-mozilla-beta?
Attachment #595239 -
Flags: approval-mozilla-aurora?
|
|
Assignee | |
Comment 9•13 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → mozilla13
|
|
Assignee | |
Updated•13 years ago
|
Component: DOM → Editor
QA Contact: general → editor
|
|
Assignee | |
Comment 10•13 years ago
|
||
Emailed release-drivers about this.
|
|
||
Comment 11•13 years ago
|
||
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)
[Triage Comment]
If this patch had baked on m-c for at least a couple of days prior to our go-to-build, I would be more in favor of taking it. Our bar for taking fixes on mozilla-release is typically low risk and well tested, with high reward. In this case, we haven't gotten any related reports from major sites, only contenteditable blocks with HTML comments inside are affected, the site can make a change if necessary, and we haven't gotten any regression testing around this yet. I'd consider that non-zero risk with little testing and low reward.
Approving for mozilla-aurora and mozilla-beta, however.
Attachment #595239 -
Flags: approval-mozilla-release?
Attachment #595239 -
Flags: approval-mozilla-release-
Attachment #595239 -
Flags: approval-mozilla-beta?
Attachment #595239 -
Flags: approval-mozilla-beta+
Attachment #595239 -
Flags: approval-mozilla-aurora?
Attachment #595239 -
Flags: approval-mozilla-aurora+
Why was tracking-esr10 minused? Seems like a very bad situation to have a bug of this level of brokenness in ESR for the whole cycle. Especially when the fix looks super-safe even for a chemspill.
|
||
Comment 13•13 years ago
|
||
Try run for 8bbbccba56b7 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=8bbbccba56b7
Results (out of 210 total builds):
exception: 1
success: 186
warnings: 21
failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8bbbccba56b7
Timed out after 06 hours without completing.
|
|
||
Comment 14•13 years ago
|
||
Comment on attachment 595239 [details] [diff] [review]
Patch (v1)
>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>@@ -3631,16 +3631,18 @@ nsEditor::IsEditable(nsIContent *aNode)
> // see if it has a frame. If so, we'll edit it.
> // special case for textnodes: frame must have width.
> if (aNode->IsElement() && !IsElementVisible(aNode->AsElement())) {
> // If the element has no frame, it's not editable. Note that we
> // need to check IsElement() here, because some of our tests
> // rely on frameless textnodes being visible.
> return false;
> }
>+ if (aNode->NodeType() == nsIDOMNode::COMMENT_NODE)
>+ return false;
> if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
> return true; // not a text node; not invisible
What about PI? Should it be handled like comment node?
Or do we never handle PI nodes here?
|
||
Comment 15•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
|
||
Updated•13 years ago
|
|
|
Assignee | |
Comment 16•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 595239 [details] [diff] [review]
> Patch (v1)
>
>
> >diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
> >--- a/editor/libeditor/base/nsEditor.cpp
> >+++ b/editor/libeditor/base/nsEditor.cpp
> >@@ -3631,16 +3631,18 @@ nsEditor::IsEditable(nsIContent *aNode)
> > // see if it has a frame. If so, we'll edit it.
> > // special case for textnodes: frame must have width.
> > if (aNode->IsElement() && !IsElementVisible(aNode->AsElement())) {
> > // If the element has no frame, it's not editable. Note that we
> > // need to check IsElement() here, because some of our tests
> > // rely on frameless textnodes being visible.
> > return false;
> > }
> >+ if (aNode->NodeType() == nsIDOMNode::COMMENT_NODE)
> >+ return false;
> > if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
> > return true; // not a text node; not invisible
>
> What about PI? Should it be handled like comment node?
> Or do we never handle PI nodes here?
Not sure... Maybe it's best to code defensively and only handle things which we knwow how to handle? Patch coming up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
|
|
Assignee | |
Comment 17•13 years ago
|
||
Attachment #595515 -
Flags: review?(roc)
Attachment #595515 -
Flags: review?(roc) → review+
|
|
Assignee | |
Comment 18•13 years ago
|
||
|
|
Assignee | |
Comment 19•13 years ago
|
||
This patch incorporates the follow-up patch to the one which is already approved on this bug.
Attachment #595552 -
Flags: approval-mozilla-beta?
Attachment #595552 -
Flags: approval-mozilla-aurora?
|
|
Assignee | |
Updated•13 years ago
|
Attachment #595239 -
Flags: approval-mozilla-beta+
Attachment #595239 -
Flags: approval-mozilla-aurora+
|
|
||
Comment 20•13 years ago
|
||
Comment on attachment 595552 [details] [diff] [review]
Rolled up patch for branches
>- if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
>- return true; // not a text node; not invisible
>-
>- return IsTextInDirtyFrameVisible(aNode);
>+ switch (aNode->NodeType()) {
>+ case nsIDOMNode::ELEMENT_NODE:
>+ return true; // not a text node; not invisible
>+ case nsIDOMNode::TEXT_NODE:
>+ return IsTextInDirtyFrameVisible(aNode);
>+ default:
>+ return false;
>+ }
> }
What about CDATA_SECTION?
|
|
||
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/319914797e70
Leaving open for comment 20.
|
|
Assignee | |
Comment 22•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 595552 [details] [diff] [review]
> Rolled up patch for branches
>
>
> >- if (aNode->NodeType() != nsIDOMNode::TEXT_NODE)
> >- return true; // not a text node; not invisible
> >-
> >- return IsTextInDirtyFrameVisible(aNode);
> >+ switch (aNode->NodeType()) {
> >+ case nsIDOMNode::ELEMENT_NODE:
> >+ return true; // not a text node; not invisible
> >+ case nsIDOMNode::TEXT_NODE:
> >+ return IsTextInDirtyFrameVisible(aNode);
> >+ default:
> >+ return false;
> >+ }
> > }
>
> What about CDATA_SECTION?
I don't think that we need to handle those. The parser will convert them to textnodes, right?
|
|
||
Comment 23•13 years ago
|
||
Comment on attachment 595552 [details] [diff] [review]
Rolled up patch for branches
[Triage Comment]
Deemed low risk and prevents comments from incorrectly being displayed - approving for Aurora/Beta.
Attachment #595552 -
Flags: approval-mozilla-beta?
Attachment #595552 -
Flags: approval-mozilla-beta+
Attachment #595552 -
Flags: approval-mozilla-aurora?
Attachment #595552 -
Flags: approval-mozilla-aurora+
|
|
||
Comment 24•13 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Why was tracking-esr10 minused? Seems like a very bad situation to have a
> bug of this level of brokenness in ESR for the whole cycle. Especially when
> the fix looks super-safe even for a chemspill.
This bug doesn't meet the criteria as outlined in [1] (under Assumptions) and [2], and hasn't been brought to our attention as a major pain point by enterprise users. We're being strict with our accepted changes to prevent the floodgates from opening on a product with minimal beta test support.
[1] https://wiki.mozilla.org/Enterprise/Firefox/ExtendedSupport:Proposal#Assumptions
[2] https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
|
|
||
Comment 25•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > What about CDATA_SECTION?
>
> I don't think that we need to handle those. The parser will convert them to
> textnodes, right?
In XHTML? Or is this code only for HTML?
|
|
Assignee | |
Comment 26•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #25)
> (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > > What about CDATA_SECTION?
> >
> > I don't think that we need to handle those. The parser will convert them to
> > textnodes, right?
> In XHTML? Or is this code only for HTML?
HTML. Our editor is pretty broken on XHTML as it's making tons of different assumptions which won't hold in XHTML documents.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 27•13 years ago
|
||
|
||
Comment 28•13 years ago
|
||
(In reply to Alex Keybl [:akeybl] from comment #11)
> Comment on attachment 595239 [details] [diff] [review]
> Patch (v1)
>
> [Triage Comment]
> If this patch had baked on m-c for at least a couple of days prior to our
> go-to-build, I would be more in favor of taking it. Our bar for taking
> fixes on mozilla-release is typically low risk and well tested, with high
> reward. In this case, we haven't gotten any related reports from major
> sites, only contenteditable blocks with HTML comments inside are affected,
> the site can make a change if necessary, and we haven't gotten any
> regression testing around this yet. I'd consider that non-zero risk with
> little testing and low reward.
>
> Approving for mozilla-aurora and mozilla-beta, however.
This bug affects all rich text editors that are v(In reply to Alex Keybl [:akeybl] from comment #7)
> Tracking for FF11 and up. We are going to build with our 10.0.1 chemspill
> tonight, so if there's any reason to think that this should be included,
> please email release-drivers ASAP.
>
> To my knowledge, we don't know of any major website regressions caused by
> this regression. From reading this bug, my understanding is that any site
> running into this issue could change the HTML on their end.
This is probably a duplicate of bug 726431. That bug potentially affects all rich texts editors. These editors are widely used for editing existing data and text via web frontends. This bug does introduce errors into existing data (e.g. when editing Wikipedia and other wiki texts with wikEd). Also, there is no way for a site to fix this issue server-side. Therefore, this bug has a very high risk and an immediate fix in Firefox 10 would be of high importance.
|
|
||
Comment 29•13 years ago
|
||
(In reply to Cacycle from comment #28)
> This is probably a duplicate of bug 726431. That bug potentially affects all
> rich texts editors. These editors are widely used for editing existing data
> and text via web frontends. This bug does introduce errors into existing
> data (e.g. when editing Wikipedia and other wiki texts with wikEd). Also,
> there is no way for a site to fix this issue server-side. Therefore, this
> bug has a very high risk and an immediate fix in Firefox 10 would be of high
> importance.
If/when this is confirmed as a significant pain point for the FF10 ESR, we'll consider landing on that branch. For non-security issues, that's the bar we've set based upon the testing coverage we receive prior to release.
|
||
Comment 30•13 years ago
|
||
Actual results:
'abc', [object comment]
Verified fixed on Firefox 11b4:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0
|
|
||
Updated•13 years ago
|
Whiteboard: [qa+] → [qa+][qa!:11]
|
|
||
Comment 31•13 years ago
|
||
Verified fixed on Firefox 12b1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Whiteboard: [qa+][qa!:11] → [qa+][qa!:11][qa!:12]
|
|
||
Comment 33•13 years ago
|
||
Verified fixed on Firefox 13b2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
Whiteboard: [qa+][qa!:11][qa!:12] → [qa+][qa!:11][qa!:12][qa!:13]
|
||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•