Fix Range.createContextualFragment for non-Element contexts.

Fix Range.createContextualFragment for non-HTMLElement contexts.

This implements the algorithm below for HTML, XHTML and SVG documents.
http://domparsing.spec.whatwg.org/#extensions-to-the-range-interface

BUG=311289, 107982
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176569

[pwnall-personal, 2013-12-22 20:25:05]

Can you please take a look?

I saw a GoodFirstBug related to a bug a filed a while ago, and I (almost)
rewrote Range.createContextualFragment to follow the latest WHATWG spec and fix
both bugs.

The implementation is significantly more complex than the old one, so I added a
bunch of tests to make up for that.

[mtomasz, 2014-04-08 04:13:29]

On 2013/12/22 20:25:05, pwnall wrote:
> Can you please take a look?
> 
> I saw a GoodFirstBug related to a bug a filed a while ago, and I (almost)
> rewrote Range.createContextualFragment to follow the latest WHATWG spec and
fix
> both bugs.
> 
> The implementation is significantly more complex than the old one, so I added
a
> bunch of tests to make up for that.

Same here. Out of curiosity, any update on this patch?

[mtomasz, 2014-04-08 04:13:50]

On 2014/04/08 04:13:29, mtomasz wrote:
> On 2013/12/22 20:25:05, pwnall wrote:
> > Can you please take a look?
> > 
> > I saw a GoodFirstBug related to a bug a filed a while ago, and I (almost)
> > rewrote Range.createContextualFragment to follow the latest WHATWG spec and
> fix
> > both bugs.
> > 
> > The implementation is significantly more complex than the old one, so I
added
> a
> > bunch of tests to make up for that.
> 
> Same here. Out of curiosity, any update on this patch?

@adamk: Ping.

[eseidel, 2014-04-10 04:28:50]

https://codereview.chromium.org/115693010/diff/180001/Source/core/editing/mar...
File Source/core/editing/markup.h (right):

https://codereview.chromium.org/115693010/diff/180001/Source/core/editing/mar...
Source/core/editing/markup.h:58: PassRefPtr<DocumentFragment>
createContextualFragment(const String&, SVGElement*, ParserContentPolicy,
ExceptionState&);
Why is SVG special?  Can we just have one for Element*?

[eseidel, 2014-05-28 22:29:33]

This is a good review for either yosin or pdr.

[pwnall-personal, 2014-05-28 22:35:48]

On 2014/05/28 22:29:33, eseidel wrote:
> This is a good review for either yosin or pdr.

Thank you! I'll rebase and ask for a new review.

[yosin_UTC9, 2014-05-29 03:53:06]

https://codereview.chromium.org/115693010/diff/180001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/180001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1156: ASSERT(element && (element->isHTMLElement() ||
element->isSVGElement()));
Is this ASSERT always true?
We can set any node to document.documentElement, e.g.
document.replaceChild(someNode, document.documentElement);

[pwnall-personal, 2014-06-02 06:57:57]

I rebased the patch and cleaned it up a bit. Can you please take another look?

https://codereview.chromium.org/115693010/diff/180001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/180001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1156: ASSERT(element && (element->isHTMLElement() ||
element->isSVGElement()));
On 2014/05/29 03:53:06, yosi wrote:
> Is this ASSERT always true?
> We can set any node to document.documentElement, e.g.
> document.replaceChild(someNode, document.documentElement);

The patch uses document.body() for HTML/XHTML documents. I think that returns
null in the case that you mentioned. Also, I think it's an interesting edge
case, so I added a test to make sure it stays covered.

https://codereview.chromium.org/115693010/diff/180001/Source/core/editing/mar...
File Source/core/editing/markup.h (right):

https://codereview.chromium.org/115693010/diff/180001/Source/core/editing/mar...
Source/core/editing/markup.h:58: PassRefPtr<DocumentFragment>
createContextualFragment(const String&, SVGElement*, ParserContentPolicy,
ExceptionState&);
On 2014/04/10 04:28:51, eseidel wrote:
> Why is SVG special?  Can we just have one for Element*?

Done. Thank you!

[eseidel, 2014-06-02 16:51:27]

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1010: if (document.isSVGDocument()) {
I would have just written this as a ternary to save space:

element = document.isSVGDocument() ? document.documentElement() :
document.body();

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1019: ASSERT(document.isHTMLDocument() ||
(document.isXHTMLDocument() && !document.isSVGDocument()));
Why not move this ASSERT into the document.documentElement() branch above (then
you wouldn't use a ternary of course).

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1021: element = fakeBody.get();
It's generally dangerous to put RefCounted objects in raw pointers.  It would be
easy for someone to refactor this method and have fakeBody die early.  I would
make "element" a RefPtr. also, why is it called "element" if it's a Node*?

https://codereview.chromium.org/115693010/diff/580041/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/115693010/diff/580041/Source/core/editing/mar...
Source/core/editing/markup.cpp:983: if (htmlElement->ieForbidsInsertHTML() ||
htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) ||
htmlElement->hasLocalName(framesetTag)
I probably would have pulled this into a little helper function, but this is
also OK.

static bool isSupportedContainer(Element* element) {
  if (!element->isHTMLElement())
      return true;

   if () // long cascade of hasLocalName checks.

   // finall cast to HTMLElement and check ieForbidsInsertHTML.
}

[pwnall-personal, 2014-06-03 00:09:00]

Thank you very much for the feedback! Can you please take another look?

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1010: if (document.isSVGDocument()) {
On 2014/06/02 16:51:28, eseidel wrote:
> I would have just written this as a ternary to save space:
> 
> element = document.isSVGDocument() ? document.documentElement() :
> document.body();

I was worried about that because the comment about reusing <body> only applies
to the 2nd part of the branch. The comment seems superfluous as it is, I tried
to make it clearer that it matters because we're being a bit more clever than
the spec algorithm here, to avoid an unnecessary Element creation.

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1019: ASSERT(document.isHTMLDocument() ||
(document.isXHTMLDocument() && !document.isSVGDocument()));
On 2014/06/02 16:51:28, eseidel wrote:
> Why not move this ASSERT into the document.documentElement() branch above
(then
> you wouldn't use a ternary of course).

I revised the asserts to express my assumptions that might not immediately come
out from the code. Is this better?

https://codereview.chromium.org/115693010/diff/580041/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1021: element = fakeBody.get();
On 2014/06/02 16:51:28, eseidel wrote:
> It's generally dangerous to put RefCounted objects in raw pointers.  It would
be
> easy for someone to refactor this method and have fakeBody die early.  I would
> make "element" a RefPtr. also, why is it called "element" if it's a Node*?

Done and done. Thank you!

https://codereview.chromium.org/115693010/diff/580041/Source/core/editing/mar...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/115693010/diff/580041/Source/core/editing/mar...
Source/core/editing/markup.cpp:983: if (htmlElement->ieForbidsInsertHTML() ||
htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) ||
htmlElement->hasLocalName(framesetTag)
On 2014/06/02 16:51:28, eseidel wrote:
> I probably would have pulled this into a little helper function, but this is
> also OK.
> 
> static bool isSupportedContainer(Element* element) {
>   if (!element->isHTMLElement())
>       return true;
> 
>    if () // long cascade of hasLocalName checks.
> 
>    // finall cast to HTMLElement and check ieForbidsInsertHTML.
> }

Done. Thank you!

[eseidel, 2014-06-03 00:27:01]

The if cascade still feels a little tortured, but the idea of this CL is fine.
:)

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1004: if (document.isSVGDocument()) {
I think you want to reverse this, no?  If it's an HTML doc, then check for the
body, otherwise assume documentElement?

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1013: // SVG documents always have an <svg> root
element, should never need a fake node.
I'm not sure that's true.  an empty .svg file might not have a documentElement? 
I'm not sure.

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1019:
exceptionState.throwDOMException(NotSupportedError, "The range's container must
be an Element, Document, or DocumentFragment.");
We throw this in multiple places?  Why even bother with the "else" here?  We
could unconditionally check "if (!element)" since element is only set if those
conditions are met, no?

[eseidel, 2014-06-03 00:32:45]

We're worrying about very small details here.  As long as the big-picture is OK
(that this CL makes us more like IE and FF) than I'm basically OK to land this
as-is.  We could detangle the ifs a bit more, but most important in any of these
compat-changes is the test cases and that the change brings us closer to other
browsers.

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:995:
exceptionState.throwDOMException(NotSupportedError, "The range's container must
be an Element, Document, or DocumentFragment.");
We could be more specific here.  It's just that Range is rooted in a detached
node?

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1015: element = HTMLBodyElement::create(document);
I don't really understand what this is going to do.  Is the idea that this would
return the string "<body></body>"?  Can we just return that explicitly instead
of creating a fake element?

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1023: ASSERT(element && (element->isHTMLElement() ||
element->isSVGElement()));
If you bumped the other if (!element) out of the else block above then we could
just remove this.

[pwnall-personal, 2014-06-04 06:52:46]

On 2014/06/03 00:32:45, eseidel wrote:
> We're worrying about very small details here.  As long as the big-picture is
OK
> (that this CL makes us more like IE and FF) than I'm basically OK to land this
> as-is.  We could detangle the ifs a bit more, but most important in any of
these
> compat-changes is the test cases and that the change brings us closer to other
> browsers.

I'm grateful for the feedback and for the opportunity to learn to code better. I
really like the clarity and conciseness in Blink's code, and I hope I will
eventually be able to produce code of the same quality.

[pwnall-personal, 2014-06-04 06:54:38]

I read up and cleaned up the code a bit more.

I also added a few extra tests, so every branch should be covered now. Thank you
very much for pushing me to do this by asking questions that I didn't have solid
answers for :)

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:995:
exceptionState.throwDOMException(NotSupportedError, "The range's container must
be an Element, Document, or DocumentFragment.");
On 2014/06/03 00:32:46, eseidel wrote:
> We could be more specific here.  It's just that Range is rooted in a detached
> node?

I re-read the spec, and I'm pretty sure we can get away without throwing here. I
updated the code and added test cases.

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1004: if (document.isSVGDocument()) {
On 2014/06/03 00:27:02, eseidel wrote:
> I think you want to reverse this, no?  If it's an HTML doc, then check for the
> body, otherwise assume documentElement?

Done.

Sorry, for some reason I confused XML and XHTML documents, and was worried that
an SVG document will return true for isXHTMLDocument(). Live and learn :/

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1013: // SVG documents always have an <svg> root
element, should never need a fake node.
On 2014/06/03 00:27:02, eseidel wrote:
> I'm not sure that's true.  an empty .svg file might not have a
documentElement? 
> I'm not sure.

You're right. Thank you very much for pointing that out!
Test added, code fixed.

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1015: element = HTMLBodyElement::create(document);
On 2014/06/03 00:32:46, eseidel wrote:
> I don't really understand what this is going to do.  Is the idea that this
would
> return the string "<body></body>"?  Can we just return that explicitly instead
> of creating a fake element?

At least as far as the spec is concerned, the fake <body> serves as a context
for the HTML parser.

I admit that I haven't looked at the parser to see whether this makes a
difference or not. Do you want me to dig deeper here?

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1019:
exceptionState.throwDOMException(NotSupportedError, "The range's container must
be an Element, Document, or DocumentFragment.");
On 2014/06/03 00:27:02, eseidel wrote:
> We throw this in multiple places?  Why even bother with the "else" here?  We
> could unconditionally check "if (!element)" since element is only set if those
> conditions are met, no?

Now the exception is only thrown in one place, if the range's document isn't
HTML / SVG. As far as I know, this implies the document is XML. I added a couple
of tests that cover this code.

FWIW, according to the spec, an exception should never be thrown. At the same
time, I'd rather not try to tackle the XML case in the same patch. It's already
one small step in the right direction.

https://codereview.chromium.org/115693010/diff/600001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1023: ASSERT(element && (element->isHTMLElement() ||
element->isSVGElement()));
On 2014/06/03 00:32:46, eseidel wrote:
> If you bumped the other if (!element) out of the else block above then we
could
> just remove this.
> 

Done.

[eseidel, 2014-06-04 15:00:22]

I'm about to get on a plane, but I'm happy to look more later.

https://codereview.chromium.org/115693010/diff/640001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/640001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1006: element = HTMLBodyElement::create(document);
It's a bit odd to pass in a detached contextElement.  Currently I don't think
that's a problem.  Its used for a few tagName checks and then here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[pwnall-personal, 2014-06-05 22:59:50]

https://codereview.chromium.org/115693010/diff/640001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/115693010/diff/640001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:1006: element = HTMLBodyElement::create(document);
On 2014/06/04 15:00:22, eseidel wrote:
> It's a bit odd to pass in a detached contextElement.  Currently I don't think
> that's a problem.  Its used for a few tagName checks and then here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

FWIW, in my interpretation of the spec, the Range's context can be a detached
node. Does that make the approach more palatable? :)

Also, I already have a test covering detached text nodes, and I can add more if
it's desirable.

[pwnall-personal, 2014-06-19 19:55:20]

On 2014/06/04 15:00:22, eseidel wrote:
> I'm about to get on a plane, but I'm happy to look more later.

eseidel: can you please take another look?

Thank you!

[eseidel, 2014-06-19 21:33:08]

lgtm

[commit-bot: I haz the power, 2014-06-19 21:34:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/115693010/640001

[commit-bot: I haz the power, 2014-06-19 21:34:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 21:34:58]

Failed to apply patch for Source/core/dom/Range.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Range.cpp
  Hunk #2 FAILED at 56.
  Hunk #3 succeeded at 961 (offset -22 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/dom/Range.cpp.rej

Patch:       Source/core/dom/Range.cpp
Index: Source/core/dom/Range.cpp
diff --git a/Source/core/dom/Range.cpp b/Source/core/dom/Range.cpp
index
75b7fd88993465bc3f2977676ec2844ad8ec8c91..e95e9bf2b9fc6f6a4a075c22a167c9e9beb60a8a
100644
--- a/Source/core/dom/Range.cpp
+++ b/Source/core/dom/Range.cpp
@@ -40,9 +40,11 @@
 #include "core/editing/VisibleUnits.h"
 #include "core/editing/markup.h"
 #include "core/events/ScopedEventQueue.h"
+#include "core/html/HTMLBodyElement.h"
 #include "core/html/HTMLElement.h"
 #include "core/rendering/RenderBoxModelObject.h"
 #include "core/rendering/RenderText.h"
+#include "core/svg/SVGSVGElement.h"
 #include "platform/geometry/FloatQuad.h"
 #include "wtf/RefCountedLeakCounter.h"
 #include "wtf/text/CString.h"
@@ -54,7 +56,6 @@
 namespace WebCore {
 
 using namespace std;
-using namespace HTMLNames;
 
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, rangeCounter, ("Range"));
 
@@ -981,13 +982,42 @@ String Range::text() const
 
 PassRefPtrWillBeRawPtr<DocumentFragment> Range::createContextualFragment(const
String& markup, ExceptionState& exceptionState)
 {
-    Node* element = m_start.container()->isElementNode() ? m_start.container()
: m_start.container()->parentNode();
-    if (!element || !element->isHTMLElement()) {
-        exceptionState.throwDOMException(NotSupportedError, "The range's
container must be an HTML element.");
+    // Algorithm:
http://domparsing.spec.whatwg.org/#extensions-to-the-range-interface
+
+    Node* node = m_start.container();
+
+    // Step 1.
+    RefPtrWillBeRawPtr<Element> element;
+    if (!m_start.offset() && (node->isDocumentNode() ||
node->isDocumentFragment()))
+        element = nullptr;
+    else if (node->isElementNode())
+        element = toElement(node);
+    else
+        element = node->parentElement();
+
+    // Step 2.
+    if (!element || isHTMLHtmlElement(element)) {
+        Document& document = node->document();
+
+        if (document.isHTMLDocument() || document.isXHTMLDocument()) {
+            // Optimization over spec: try to reuse the existing <body>
element, if it is available.
+            element = document.body();
+            if (!element)
+                element = HTMLBodyElement::create(document);
+        } else if (document.isSVGDocument()) {
+            element = document.documentElement();
+            if (!element)
+                element = SVGSVGElement::create(document);
+        }
+    }
+
+    if (!element || (!element->isHTMLElement() && !element->isSVGElement())) {
+        exceptionState.throwDOMException(NotSupportedError, "The range's
container must be an HTML or SVG Element, Document, or DocumentFragment.");
         return nullptr;
     }
 
-    RefPtrWillBeRawPtr<DocumentFragment> fragment =
WebCore::createContextualFragment(markup, toHTMLElement(element),
AllowScriptingContentAndDoNotMarkAlreadyStarted, exceptionState);
+    // Steps 3, 4, 5.
+    RefPtrWillBeRawPtr<DocumentFragment> fragment =
WebCore::createContextualFragment(markup, element.get(),
AllowScriptingContentAndDoNotMarkAlreadyStarted, exceptionState);
     if (!fragment)
         return nullptr;

[pwnall-personal, 2014-06-20 00:19:36]

The CQ bit was checked by costan@gmail.com

[commit-bot: I haz the power, 2014-06-20 00:19:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/115693010/660001

[pwnall-personal, 2014-06-20 00:20:27]

On 2014/06/19 21:33:08, eseidel wrote:
> lgtm

Thank you for the quick feedback! I know you're swamped with reviews, and I
really appreciate it!

[commit-bot: I haz the power, 2014-06-20 01:40:51]

Change committed as 176569