Closed Bug 725069 Opened 12 years ago Closed 12 years ago

Html Comments being shown in DOM as text nodes. Regression from firefox 9.0

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
10 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13
Tracking Flags:
Tracking Status
firefox10 - wontfix
firefox11 + verified
firefox12 + verified
firefox13 + verified
firefox-esr10 - wontfix

People

(Reporter: firefox, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression, Whiteboard: [qa+][qa!:11][qa!:12][qa!:13])

Attachments

(4 files)

test case.
288 bytes, text/html
Details
Patch (v1)
3.50 KB, patch
roc
: review+
akeybl
: approval-mozilla-release-
Details | Diff | Splinter Review
Follow-up
1.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
Rolled up patch for branches
3.85 KB, 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]
Version: 8 Branch → 10 Branch
Severity: normal → critical
Attached file test case. — 

        Attachment #595146 -
        Attachment mime type: text/plain → text/html
Component: Untriaged → DOM
Product: Firefox → Core
QA Contact: untriaged → general

    
    

    
  
Only occurs when contenteditable=true blocks.

    
  
Severity: critical → normal

    
  
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

    
    

    
  
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

    
  
Blocks: 688789
Keywords: regressionwindow-wanted → regression
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v1)
        — 
          — Splinter Review
      

    


  
Assignee: nobody → ehsan
Status: NEW → ASSIGNED

        Attachment #595239 -
        Flags: review?(roc)

    
    

    
  
Better get this onto branches...

    
    

    
  
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.

          tracking-firefox-esr10:
          ? → -

          tracking-firefox10:
          ? → -

          tracking-firefox11:
          ? → +

          tracking-firefox12:
          ? → +

          tracking-firefox13:
          ? → +

    
    

    
  
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?

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e246f6074a1
Flags: in-testsuite+
Target Milestone: --- → mozilla13

    
  
Component: DOM → Editor
QA Contact: general → editor

    
    

    
  
Emailed release-drivers about this.

    
    

    
  
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.

    
    

    
  
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 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?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3e246f6074a1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
(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 → ---

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Follow-up
        — 
          — Splinter Review
      

    


  

        Attachment #595515 -
        Flags: review?(roc)

    
    

    
  


  
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?

    
  

        Attachment #595239 -
        Flags: approval-mozilla-beta+

        Attachment #595239 -
        Flags: approval-mozilla-aurora+

    
    

    
  
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?

    
    

    
  
(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 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+

    
    

    
  
(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

          tracking-firefox-esr10:
          ? → -

    
    

    
  
(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?

    
    

    
  
(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: 12 years ago → 12 years ago
Resolution: --- → FIXED

    
    

    
  
(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.

    
    

    
  
(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.

    
  
Whiteboard: [qa+]

    
    

    
  
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

          status-firefox11:
          fixed → verified
Whiteboard: [qa+] → [qa+][qa!:11]

    
    

    
  
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

          status-firefox12:
          fixed → verified
Whiteboard: [qa+][qa!:11] → [qa+][qa!:11][qa!:12]

    
    

    
  
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

          status-firefox13:
          fixed → verified
Whiteboard: [qa+][qa!:11][qa!:12] → [qa+][qa!:11][qa!:12][qa!:13]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: