[Webkit-unassigned] [Bug 30116] WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV at NULL

[bugzilla-daemon at webkit.org]

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





--- Comment #18 from Ojan Vafai <ojan at chromium.org>  2010-06-09 16:08:01 PST ---
(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 58097 [details] [details])
> > Doesn't this make it so that we no longer canonicalize the position we're going to use when we hit enter?
> 
> I'm not sure I understand, which canonicalization isn't happening?

Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right?

> > I wonder if a better (albeit harder) fix would be to make it so that "VisiblePosition caret(selection.visibleStart())" doesn't create a VisiblePosition with a null deepEquivalent. Specifically, VisiblePosition::canonicalPosition would need to not return null. If I understand this all correctly, we return null because we skip over visibility:hidden nodes when looking for the canonical position. This makes sense in general, but I don't think it makes sense when the rootEditableElement is the visibility:hidden node. Maybe we should change that logic. WDYT?
> 
> VisiblePosition::canonicalPosition returns null because PositionIterator checks for visibility.  Changing this would be a big change which seems beyond the scope of this bug.  But maybe you're asking about only changing visibleStart some of the time?  That also seems tricky to implement (e.g. what happens if the rootEditableElement is visible but some child editable node is hidden)?

I think it's OK to special-case the rootEditableElement here. If a child isn't visible, the typing will still go through just fine. It just won't go inside the visibility:hidden element. It'll go in one of it's siblings/parents/cousins. I think it would be sufficient for the visibility check in PositionIterator::isCandidate and Position::isCandidate to not return false there if the renderer is the renderer for the rootEditableElement, but I'm not sure.

> I found another crasher except with contenteditable areas, so I've uploaded a new patch.

This looks like we'll do the wrong thing (use a paragraph instead of a BR) if you hit enter inside a table that's not visible. That's much better than crashing though. Maybe add a FIXME pointing to bug 40342?

> If the goal of this bug is to just fix the crash, then perhaps we should just return early in both cases (or even return in EventHandler::defaultTextInputEventHandler.

I think that would be fine if you include a FIXME pointing to bug 40342. We should probably early return here though, not higher up in defaultTextInputEventHandler as we'll want to fix this someday.

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