Bug 149528 – Rewrite Range::insertNode() as per the latest DOM specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Rewrite Range::insertNode() as per the latest DOM specification:
https://dom.spec.whatwg.org/#concept-range-insert

Our current implementation seems outdated as we fail a lot of W3C tests that Firefox / Chrome are passing.

[Chris Dumez]

Created attachment 261894 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 261894 [details]
Patch

Clearing flags on attachment: 261894

Committed r190229: <http://trac.webkit.org/changeset/190229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 261894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261894&action=review

> Source/WebCore/dom/Range.cpp:873
> +    if (referenceNode.get() == node.get())

Surprised these calls to get() are needed. Normally we implement operator==.

[Chris Dumez]

Comment on attachment 261894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261894&action=review

>> Source/WebCore/dom/Range.cpp:873
>> +    if (referenceNode.get() == node.get())
> 
> Surprised these calls to get() are needed. Normally we implement operator==.

You are right, we do implement operator==() for RefPtr. I will drop them in a follow-up patch.

[Radar WebKit Bug Importer]

<rdar://problem/22882437>

[besworks]

Did you intend to delete the createContextualFragment method from the Range object with this patch? This change has crippled some of my projects which rely on this method.

[Chris Dumez]

(In reply to comment #7)
> Did you intend to delete the createContextualFragment method from the Range
> object with this patch? This change has crippled some of my projects which
> rely on this method.

I did NOT delete Range.createContextualFragment(), it is still there and unchanged. The Change log does say that the method was removed but that's just not true, just a bug with the script generating the change log.

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > Did you intend to delete the createContextualFragment method from the Range
> > object with this patch? This change has crippled some of my projects which
> > rely on this method.
> 
> I did NOT delete Range.createContextualFragment(), it is still there and
> unchanged. The Change log does say that the method was removed but that's
> just not true, just a bug with the script generating the change log.

You can try a WebKit nightly build to confirm your projects are still working:
http://nightly.webkit.org

If you find I broke things that work in other browsers, please let me know.

[besworks]

(In reply to comment #9)
> You can try a WebKit nightly build to confirm your projects are still
> working:
> http://nightly.webkit.org
> 
> If you find I broke things that work in other browsers, please let me know.

Thanks for the reply. I tried the latest nightly as you suggested but the same issue existed. I've figured out what's happening though.

If I create a Range object using the constructor I can see that the createContextualFragment method does actually exist in the Range's prototype but calling this method would throw a NotSupportedError: DOM Exception 9 rather than returning a DocumentFragment object as expected.

If I call Range.setStart and Range.setEnd then createContextualFragment works as it should.

Looking at Range.cpp I can see that createContextualFragment checks if startContainer is an HTMLElement but the Range constructor sets startContainer as the global document object which fails this check.

Sorry for jumping to conclusions about your patch. I'll create new bug report for this issue.