Bug 149528 - Rewrite Range::insertNode() as per the latest DOM specification
Summary: Rewrite Range::insertNode() as per the latest DOM specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#concept-...
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2015-09-24 10:05 PDT by Chris Dumez
Modified: 2016-02-23 10:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (91.46 KB, patch)
2015-09-24 14:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-09-24 10:05:15 PDT
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.
Comment 1 Chris Dumez 2015-09-24 14:03:31 PDT
Created attachment 261894 [details]
Patch
Comment 2 WebKit Commit Bot 2015-09-24 16:33:55 PDT
Comment on attachment 261894 [details]
Patch

Clearing flags on attachment: 261894

Committed r190229: <http://trac.webkit.org/changeset/190229>
Comment 3 WebKit Commit Bot 2015-09-24 16:34:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2015-09-25 09:16:50 PDT
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==.
Comment 5 Chris Dumez 2015-09-25 09:18:38 PDT
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.
Comment 6 Radar WebKit Bug Importer 2015-09-28 11:37:56 PDT
<rdar://problem/22882437>
Comment 7 besworks 2016-02-22 11:44:39 PST
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.
Comment 8 Chris Dumez 2016-02-23 08:54:22 PST
(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.
Comment 9 Chris Dumez 2016-02-23 08:55:28 PST
(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.
Comment 10 besworks 2016-02-23 10:17:05 PST
(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.