[Webkit-unassigned] [Bug 25571] DOM range methods setStartBefore/setEndAfter fails on detached elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25571





--- Comment #24 from Ryosuke Niwa <rniwa at webkit.org>  2014-02-17 21:11:28 PST ---
(In reply to comment #23)
> As per my earlier discussion with Anne, Attr, Entity and Notation nodes no longer exist. In addition to it, if a refNode is of document or ducument fragment type, then the parent will be null which suffices the need.
> 
> > r- because this patch removes some important node type checks and introduces bugs that the DOM4 spec has.
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        DOM2 spec : http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html
> > > +        latest spec : http://dom.spec.whatwg.org/
> 
> I am referring to the latest one. git commit id (33a11bb68544e2ae23d326448a438e7ac46bf640) as there is no change in the methods I am concerned, from the previous methods.

We should mention that in the change log as my point was that the spec could change in the future, not that it may have already changed.

> > Please refer to a specific revision (git commit, publishing date, etc...) of the specification.
> > Otherwise the specification will change in the future and we'll have no idea which specification
> > this change set was referring to.
> 
> Actually, there are few public methods invoked by the methods where this issue lies. But, I didn't touch any of these methods even though the new spec has some changes to these methods. I think it should be tracked separately as an activity to sync with new spec. Let me know your suggestion.

That doesn't matter. This change affects those publicly exposed API's behavior.

> > > Source/WebCore/dom/Range.cpp:1213
> > >      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);
> > 
> > setStart must throw InvalidNodeTypeError when refNode is doc type but it doesn't.
> > So this effectively breaks setStartAfter in that regard as well.
> 
> 
> The ownerdocument check is present in setStart as well as setEnd even though spec doesn't have it. Do we really require this check to be duplicated again in the methods invoking setStart/setEnd?

Ah I see. We should probably mention that in the change log as a per-function inline comment.

See http://www.webkit.org/coding/contributing.html#changelogs for an example of a well written change log.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.