Bug 89918

Created attachment 149380 [details] Repro

I've investigated this bug. This is actually a bug of comparePositions(). In this test case, we have a call comparePosition(a, b) where deprecatedNode() and deprecatedEditingOffset() of a and b are the same but anchoryType() of a and b are different. It's time to fix comparePosition() correctly.... We have to fix Bug 54535 issue. As I mentioned in the issue, I would like to have an alternative version of comparePosition that distinguishes "position after img" from "position before br" (See the Bug 54535 comment). To compare m_start and m_end, we should use the alternative version, I believe.

Created attachment 149556 [details] Patch

Let me try to introduce PositionComparisonFineness to comparePosition(). I hope this patch does not degrade any tests. We should argue we're going on the right way or not though...

Comment on attachment 149556 [details] Patch This patch is obsolete now after http://trac.webkit.org/changeset/121303

After http://trac.webkit.org/changeset/121303, this assertion is not triggered anymore. Let mark this as INVALID.

Do we have a test case that we can add?

(In reply to comment #7) > Do we have a test case that we can add? Yes. I'll upload a patch to add a testcase.

(In reply to comment #8) > (In reply to comment #7) > > Do we have a test case that we can add? > > Yes. I'll upload a patch to add a testcase. Great. Because of the inherent complexity, it's crucial for us to add more tests for bugs like this :)

Created attachment 149769 [details] Patch

Comment on attachment 149769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149769&action=review > LayoutTests/ChangeLog:12 > + However, we don't have any test case of selection from Shadow DOM to elements outside of shadow host. > + So let's add a testcase here. I don't think you need to mention this. It's pretty obvious that the reason we're adding a regression test is because we hadn't had one already. > LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:6 > +<script src="../../fast/dom/resources/event-sender-util.js"></script> > +<script src="../../fast/js/resources/js-test-pre.js"></script> I don't think you need a event-sender-util.js here. You're not using any functions in there. Also, I don't see a point in making this a script test. You're not using any features in there. > LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:14 > + <div id="host">host</div> > + <div id="dst">after host</div> Please spell out destination. > LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:29 > +var src = shadowRoot.getElementById('src'); > +var dst = document.getElementById('dst'); Ditto about spelling out source and destination. > LayoutTests/editing/shadow/breaking-editing-boundaries-2.html:37 > + container.innerHTML = "PASS"; This is sufficient without any of js-test-*.js files.

Created attachment 149772 [details] Patch for landing

Comment on attachment 149772 [details] Patch for landing Clearing flags on attachment: 149772 Committed r121350: <http://trac.webkit.org/changeset/121350>

All reviewed patches have been landed. Closing bug.