Bug 5870 – Double-clicking on an SVG dies in HTML editing code

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

This assert:
    ASSERT(result != *this);

visible_position.cpp line 132.

Fails when double-clicking on an SVG.


#0	0x018fa45c in khtml::VisiblePosition::previous at visible_position.cpp:132
#1	0x01905844 in khtml::endOfWord at visible_units.cpp:237
#2	0x018f5f60 in khtml::SelectionController::validate at SelectionController.cpp:904
#3	0x018f6d90 in khtml::SelectionController::expandUsingGranularity at SelectionController.cpp:494
#4	0x016b7c68 in KHTMLPart::selectClosestWordFromMouseEvent at khtml_part.cpp:2539
#5	0x016b7ddc in KHTMLPart::handleMousePressEventDoubleClick at khtml_part.cpp:2565
#6	0x016c039c in KHTMLPart::khtmlMousePressEvent at khtml_part.cpp:2677
#7	0x01647ae8 in KWQKHTMLPart::khtmlMousePressEvent at KWQKHTMLPart.mm:2050
#8	0x016b6c4c in KHTMLPart::customEvent at khtml_part.cpp:2465
#9	0x01a11fc0 in KParts::Part::event at KWQKPartsPart.h:56
#10	0x01a22304 in QApplication::sendEvent at KWQApplication.h:39
#11	0x016c92d8 in KHTMLView::viewportMousePressEvent at khtmlview.cpp:646
#12	0x0164cd10 in KWQKHTMLPart::mouseDown at KWQKHTMLPart.mm:2692
#13	0x016a9398 in -[WebCoreBridge mouseDown:] at WebCoreBridge.mm:885
#14	0x0037d16c in -[WebHTMLView mouseDown:] at WebHTMLView.m:2580
#15	0x936ce660 in -[NSWindow sendEvent:]
#16	0x0001d3b0 in ??
#17	0x936776f4 in -[NSApplication sendEvent:]
#18	0x0001a388 in ??
#19	0x9366eb30 in -[NSApplication run]
#20	0x9375f618 in NSApplicationMain
#21	0x0000265c in ??
#22	0x00056d1c in ??

[Eric Seidel (no email)]

I think possibly this method needs to be enhanced:

bool Position::atStart() const
{
    NodeImpl *n = node();
    if (!n)
        return true;
    
    return offset() <= 0 && n->parent() == 0;
}

perhaps if (!n || n->isSVGElement()) ?

Not sure that makes sense.  Selection will eventually make sense in an SVGDocument, or at least text 
selection will:

http://www.w3.org/TR/SVG/text.html#TextSelection

[Justin Garcia]

Created attachment 5576 [details]
patch

Account for replaced elements with children in VisiblePosition::isCandidate and
maxDeepOffset.	
Define and use a single isAtomicNode and maxDeepOffset (they now live in
htmlediting.cpp).
Stop at candidates inside VisiblePosition::deepEquivalent(const Position &). 
If it was allowed to descend, it would return a position that was not visually
equivalent to the input position.
Fixed a regression where the caret rect was incorrect for carets before/after
replaced elements.  SelectionController computed m_caretRect at layout time
using a different position than the one used at paint time to adjust the caret
for scrolling.
Removed VisiblePosition::downstreamDeepEquivalent().  It's just equal to
position().downstream().

Now use a single method for finding the deepest equivalent position that is
visually equivalent to the input position.  That method is
VisiblePosition::deepEquivalent(const Position &).
Renamed VisiblePosition::equivalentDeepPosition() to
VisiblePosition::deepEquivalent().
Renamed VisiblePositions private field for the canonical position from
m_deepPosition to m_position, and renamed the accessor to position().
Now use a single method for finding range compliant positions
(Position::equivalentRangeCompliantPosition).

[Justin Garcia]

Created attachment 5577 [details]
patch

a few tweaks

[Justin Garcia]

I should probably justify the renaming of m_deepPosition to m_position and 
VisiblePosition::deepEquivalent() to VisiblePosition::position():

1) Avoid confusion with Position::deepEquivalent()
2) A VisiblePosition's associated position can be visually equivalent to many positions (think collapsed 
whitespace).  Many of those equivalent positions can be 'deep' in the sense that they are as far down in the 
DOM as it is possible to go.  Asking for 'deepEquivalent' then seems ambiguous, i.e. 'which deep 
equivalent?'.  Yes, I know, 'position' is even more ambiguous.  We could use 
VisiblePosition::canonicalPosition(), but I think it's too verbose.  What if VisiblePosition were a subclass of 
Position?  Then the VisiblePosition itself represents the canonical DOM position + an affinity.  I'd like to 
ask darin since he was the one who gave birth to VisiblePosition.

[Eric Seidel (no email)]

Comment on attachment 5577 [details]
patch

So I looked at the patch briefly, it looks OK.	I think it's best if Darin or
dave look through this one though.

I also think it would be very intersesting to see a patch w/o the renames, as
combining both functional changes and renames into a single patch makes it
harder to read. :(

[Justin Garcia]

Created attachment 5581 [details]
patch

Redid the patch on TOT, w/o the VisiblePosition::deepEquivalent() ->
VisiblePosition::position() rename.  Also, I previously forgot to mention the
small fix to is{First,Last}VisiblePositionInNode (check to see that the
position is actually in the node :-)).

[Justin Garcia]

Retouching my ChangeLog notes from above, there were a few mistakes:

- Account for replaced elements with children in VisiblePosition::isCandidate and maxDeepOffset.
- Define and use a single isAtomicNode and maxDeepOffset (they now live in htmlediting.cpp).

- Stop at candidates inside VisiblePosition::deepEquivalent(const Position &).  If the function is allowed to descend past a candidate, it will return a position 
that is not visually equivalent to the input position.  For example:
<table><tr><td>hello</td></tr></table>
[table, 0] and [text, 0] are two distinct visible positions, but [text, 0] would be reached if deepEquivalent descended past candidates.

- Removed a comment in previousVisiblePosition.  It is no longer correct because deepEquivalent() will not descend past candidates.
- Fixed a regression where the caret rect was incorrect for carets before/after replaced elements.  SelectionController computed m_caretRect at layout time
using a different position than the one used at paint time to adjust the caret for scrolling.
- Removed VisiblePosition::downstreamDeepEquivalent().  It's just equal to deepEquivalent().downstream().
- Now use a single method for finding the deepest equivalent position that is visually equivalent to an input position.  That method is
- VisiblePosition::deepEquivalent(const Position &).  Removed Position::equivalentDeepPosition().
- Now use a single method for finding range compliant positions.  That method is Position::equivalentRangeCompliantPosition().

[Justin Garcia]

Created attachment 5583 [details]
patch

[Justin Garcia]

I was wrong about downstreamDeepEquivalent, its not just the deepEquivalent().downstream().  In the case 
where two candidates correspond to the same VP, we chose the one that's farther upstream.  
downstreamDeepEquivalent returns the one that's farther downstream, or deepEquivalent if there is only 
one candidate.  I'm still not sure if it's necessary, looking into it.

[Justin Garcia]

I'm leaving the removal of downstreamDeepEquivalent in this patch, I filed 6476 to cover the problems 
that occur when two different candidates straddle a line wrap.

[Darin Adler]

Comment on attachment 5583 [details]
patch

This could be improved:

+    // Allows [br, 1] as a position, though that is incorrect
+    if (node->hasTagName(brTag) || (renderer && (renderer->isReplaced() ||
renderer->isWidget())))
+	 return 1;

The isWidget check isn't needed, because all widgets are replaced. Also, the
comment mentions <br>, but the same "wrongness" is true about replaced elements
with offset 1, so the comment should mention both.

[Darin Adler]

Comment on attachment 5583 [details]
patch

This could be improved:

+    // Allows [br, 1] as a position, though that is incorrect
+    if (node->hasTagName(brTag) || (renderer && (renderer->isReplaced() ||
renderer->isWidget())))
+	 return 1;

The isWidget check isn't needed, because all widgets are replaced. Also, the
comment mentions <br>, but the same "wrongness" is true about replaced elements
with offset 1, so the comment should mention both.

[Justin Garcia]

When I double click on an SVG with TOT WebKit (w/o my patch applied), I can't get the ASSERT to fire 
anymore.  Eric, Alexey, Alice, what about you guys?

[Eric Seidel (no email)]

Yeah it stopped ASSERTing a while back.  I'm not sure why, I figured you had fixed some related editing 
bug a while back.

[Justin Garcia]

It looks like the fix for 5354 sent the double click down a different path (one where 
VisiblePosition::previous is never called).  You can still see this assert by following the steps in this bug:
<rdar://problem/4393815> Assertion failure: result != *this (khtml::VisiblePosition::previous())
Which is fixed with this patch.  

I'm going to land this w/o a layout test for the double-click an SVG case b/c I'm not sure what the 
expected behavior should be.  Right now, double clicking on an SVG creates a caret selection after the SVG 
(you can't see it because we don't paint carets in non-editable areas).  This is probably wrong, but we're 
going to address this in 6455.

[Justin Garcia]

landing