Commit 817909f9 authored by darin's avatar darin

Reviewed by Adele and Justin.

        - fix http://bugzilla.opendarwin.org/show_bug.cgi?id=8298
          REGRESSION: Crash occurs when attempting to drag selection into
          Depart/Return input fields at http://www.travelocity.com/
        - remove the mutation event listener that's installed all the time,
          since it slows things down a bit

        Calling SelectionController::nodeWillBeRemoved from Document::notifyBeforeNodeRemoval
        fixes the crash, which was happening because the call that was removing the text
        node, removeChildren, does not send a "node removed" mutation event (it sends a
        "subtree modified" mutation event instead). So this change alone fixes the crash.

        But I also changed setInnerText to not blow away the text node each time the value
        is changed, and that makes the test case behave even better -- you don't even lose
        the selection; it works as it did with the NSTextField-based text field.

        * manual-tests/input-empty-on-focus.html: Added.

        * page/Frame.h: Tweaked a few comments and functions related to selection.
        * page/Frame.cpp: (WebCore::Frame::dragCaret): Made non-const.

        * dom/Document.cpp: (WebCore::Document::notifyBeforeNodeRemoval):
        Call nodeWillBeRemoved on the two selection controllers before removing
        a node from the document.

        * editing/SelectionController.h: Tweak formatting. Remove MutationListener
        class and m_mutationListener field.
        * editing/SelectionController.cpp:
        (WebCore::SelectionController::SelectionController): Remove code to set up
        the mutation event listener.
        (WebCore::SelectionController::setSelection): Remove code to maintain the
        mutation event listener.

        * html/HTMLElement.cpp:
        (WebCore::HTMLElement::setInnerHTML): In cases where the container has only a
        single child use replaceChild, and in cases where the HTML being inserted
        also has only a single child and both are text nodes use setData. It's common
        to use setInnerHTML to set something that's just text.
        (WebCore::HTMLElement::setInnerText): Same as above, but simpler since the
        thing we're replacing with is always text.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@13889 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent dd042e53
2006-04-16 Darin Adler <darin@apple.com>
Reviewed by Adele and Justin.
- fix http://bugzilla.opendarwin.org/show_bug.cgi?id=8298
REGRESSION: Crash occurs when attempting to drag selection into
Depart/Return input fields at http://www.travelocity.com/
- remove the mutation event listener that's installed all the time,
since it slows things down a bit
Calling SelectionController::nodeWillBeRemoved from Document::notifyBeforeNodeRemoval
fixes the crash, which was happening because the call that was removing the text
node, removeChildren, does not send a "node removed" mutation event (it sends a
"subtree modified" mutation event instead). So this change alone fixes the crash.
But I also changed setInnerText to not blow away the text node each time the value
is changed, and that makes the test case behave even better -- you don't even lose
the selection; it works as it did with the NSTextField-based text field.
* manual-tests/input-empty-on-focus.html: Added.
* page/Frame.h: Tweaked a few comments and functions related to selection.
* page/Frame.cpp: (WebCore::Frame::dragCaret): Made non-const.
* dom/Document.cpp: (WebCore::Document::notifyBeforeNodeRemoval):
Call nodeWillBeRemoved on the two selection controllers before removing
a node from the document.
* editing/SelectionController.h: Tweak formatting. Remove MutationListener
class and m_mutationListener field.
* editing/SelectionController.cpp:
(WebCore::SelectionController::SelectionController): Remove code to set up
the mutation event listener.
(WebCore::SelectionController::setSelection): Remove code to maintain the
mutation event listener.
* html/HTMLElement.cpp:
(WebCore::HTMLElement::setInnerHTML): In cases where the container has only a
single child use replaceChild, and in cases where the HTML being inserted
also has only a single child and both are text nodes use setData. It's common
to use setInnerHTML to set something that's just text.
(WebCore::HTMLElement::setInnerText): Same as above, but simpler since the
thing we're replacing with is always text.
2006-04-16 Kevin Ollivier <kevino@theolliviers.com>
Reviewed by Darin.
......@@ -34,6 +34,7 @@
#include "DocumentFragment.h"
#include "DocumentType.h"
#include "EditingText.h"
#include "EventListener.h"
#include "EventNames.h"
#include "ExceptionCode.h"
#include "Frame.h"
......@@ -2174,6 +2175,10 @@ void Document::detachNodeIterator(NodeIterator *ni)
void Document::notifyBeforeNodeRemoval(Node *n)
{
if (Frame* f = frame()) {
f->selection().nodeWillBeRemoved(n);
f->dragCaret().nodeWillBeRemoved(n);
}
DeprecatedPtrListIterator<NodeIterator> it(m_nodeIterators);
for (; it.current(); ++it)
it.current()->notifyBeforeNodeRemoval(n);
......
......@@ -43,18 +43,8 @@ namespace WebCore {
using namespace EventNames;
void MutationListener::handleEvent(Event *event, bool isWindowEvent)
{
if (!m_selectionController)
return;
if (event->type() == DOMNodeRemovedEvent)
m_selectionController->nodeWillBeRemoved(event->target());
}
SelectionController::SelectionController()
: m_needsLayout(true)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection());
}
......@@ -62,7 +52,6 @@ SelectionController::SelectionController()
SelectionController::SelectionController(const Selection &sel)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(sel);
}
......@@ -70,7 +59,6 @@ SelectionController::SelectionController(const Selection &sel)
SelectionController::SelectionController(const Position &pos, EAffinity affinity)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection(pos, pos, affinity));
}
......@@ -78,7 +66,6 @@ SelectionController::SelectionController(const Position &pos, EAffinity affinity
SelectionController::SelectionController(const Range *r, EAffinity affinity)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection(startPosition(r), endPosition(r), affinity));
}
......@@ -86,7 +73,6 @@ SelectionController::SelectionController(const Range *r, EAffinity affinity)
SelectionController::SelectionController(const Position &base, const Position &extent, EAffinity affinity)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection(base, extent, affinity));
}
......@@ -94,7 +80,6 @@ SelectionController::SelectionController(const Position &base, const Position &e
SelectionController::SelectionController(const VisiblePosition &visiblePos)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection(visiblePos.deepEquivalent(), visiblePos.deepEquivalent(), visiblePos.affinity()));
}
......@@ -102,7 +87,6 @@ SelectionController::SelectionController(const VisiblePosition &visiblePos)
SelectionController::SelectionController(const VisiblePosition &base, const VisiblePosition &extent)
: m_needsLayout(true)
, m_modifyBiasSet(false)
, m_mutationListener(new MutationListener(this))
{
setSelection(Selection(base.deepEquivalent(), extent.deepEquivalent(), base.affinity()));
}
......@@ -110,7 +94,6 @@ SelectionController::SelectionController(const VisiblePosition &base, const Visi
SelectionController::SelectionController(const SelectionController &o)
: m_needsLayout(o.m_needsLayout)
, m_modifyBiasSet(o.m_modifyBiasSet)
, m_mutationListener(new MutationListener(this))
{
setSelection(o.m_sel);
// Only copy the coordinates over if the other object
......@@ -124,14 +107,6 @@ SelectionController::SelectionController(const SelectionController &o)
}
}
SelectionController::~SelectionController()
{
if (!isNone()) {
Document *doc = m_sel.start().node()->document();
doc->removeEventListener(DOMNodeRemovedEvent, m_mutationListener.get(), false);
}
}
SelectionController &SelectionController::operator=(const SelectionController &o)
{
setSelection(o.m_sel);
......@@ -188,19 +163,8 @@ void SelectionController::moveTo(const Position &base, const Position &extent, E
m_needsLayout = true;
}
void SelectionController::setSelection(const Selection &newSelection)
void SelectionController::setSelection(const Selection& newSelection)
{
Selection oldSelection = m_sel;
Document *oldDocument = oldSelection.start().node() ? oldSelection.start().node()->document() : 0;
Document *newDocument = newSelection.start().node() ? newSelection.start().node()->document() : 0;
if (oldDocument != newDocument) {
if (oldDocument)
oldDocument->removeEventListener(DOMNodeRemovedEvent, m_mutationListener.get(), false);
if (newDocument)
newDocument->addEventListener(DOMNodeRemovedEvent, m_mutationListener.get(), false);
}
m_sel = newSelection;
}
......@@ -209,10 +173,10 @@ void SelectionController::nodeWillBeRemoved(Node *node)
if (isNone())
return;
Node *base = m_sel.base().node();
Node *extent = m_sel.extent().node();
Node *start = m_sel.start().node();
Node *end = m_sel.end().node();
Node* base = m_sel.base().node();
Node* extent = m_sel.extent().node();
Node* start = m_sel.start().node();
Node* end = m_sel.end().node();
bool baseRemoved = node == base || base->isAncestor(node);
bool extentRemoved = node == extent || extent->isAncestor(node);
......@@ -222,11 +186,13 @@ void SelectionController::nodeWillBeRemoved(Node *node)
if (startRemoved || endRemoved) {
// FIXME (6498): This doesn't notify the editing delegate of a selection change.
// FIXME: When endpoints are removed, we should just alter the selection, instead of blowing it away.
// FIXME: The SelectionController should be responsible for scheduling a repaint,
// but it can't do a proper job of it until it handles the other types of DOM mutations.
// For now, we'll continue to let RenderObjects handle it when they are destroyed.
setSelection(Selection());
} else if (baseRemoved || extentRemoved) {
......
......@@ -26,7 +26,6 @@
#ifndef KHTML_EDITING_SELECTIONCONTROLLER_H
#define KHTML_EDITING_SELECTIONCONTROLLER_H
#include "EventListener.h"
#include "IntRect.h"
#include "Selection.h"
#include "Range.h"
......@@ -37,21 +36,6 @@ class Frame;
class GraphicsContext;
class RenderObject;
class VisiblePosition;
class SelectionController;
class MutationListener : public EventListener
{
public:
MutationListener() { m_selectionController = 0; }
MutationListener(SelectionController *s) { m_selectionController = s; }
SelectionController *selectionController() const { return m_selectionController; }
void setSelectionController(SelectionController *s) { m_selectionController = s; }
virtual void handleEvent(Event*, bool isWindowEvent);
private:
SelectionController *m_selectionController;
};
class SelectionController
{
......@@ -61,32 +45,30 @@ public:
#define SEL_DEFAULT_AFFINITY DOWNSTREAM
SelectionController();
SelectionController(const Selection &sel);
SelectionController(const Range *, EAffinity affinity);
SelectionController(const VisiblePosition &);
SelectionController(const VisiblePosition &, const VisiblePosition &);
SelectionController(const Position &, EAffinity affinity);
SelectionController(const Position &, const Position &, EAffinity);
SelectionController(const SelectionController &);
SelectionController(const Selection&);
SelectionController(const Range*, EAffinity);
SelectionController(const VisiblePosition&);
SelectionController(const VisiblePosition&, const VisiblePosition&);
SelectionController(const Position&, EAffinity);
SelectionController(const Position&, const Position&, EAffinity);
SelectionController(const SelectionController&);
~SelectionController();
SelectionController &operator=(const SelectionController &o);
SelectionController &operator=(const VisiblePosition &r) { moveTo(r); return *this; }
SelectionController& operator=(const SelectionController&);
SelectionController& operator=(const VisiblePosition& r) { moveTo(r); return *this; }
Element* rootEditableElement() const { return selection().rootEditableElement(); }
bool isContentEditable() const { return selection().isContentEditable(); }
bool isContentRichlyEditable() const { return selection().isContentRichlyEditable(); }
void moveTo(const Range *, EAffinity affinity);
void moveTo(const VisiblePosition &);
void moveTo(const VisiblePosition &, const VisiblePosition &);
void moveTo(const Position &, EAffinity);
void moveTo(const Position &, const Position &, EAffinity);
void moveTo(const SelectionController &);
void moveTo(const Range*, EAffinity);
void moveTo(const VisiblePosition&);
void moveTo(const VisiblePosition&, const VisiblePosition&);
void moveTo(const Position&, EAffinity);
void moveTo(const Position&, const Position&, EAffinity);
void moveTo(const SelectionController&);
const Selection &selection() const { return m_sel; }
void setSelection(const Selection &);
const Selection& selection() const { return m_sel; }
void setSelection(const Selection&);
Selection::EState state() const { return m_sel.state(); }
......@@ -98,10 +80,10 @@ public:
void clear();
void setBase(const VisiblePosition &);
void setBase(const Position &pos, EAffinity affinity);
void setExtent(const VisiblePosition &);
void setExtent(const Position &pos, EAffinity affinity);
void setBase(const VisiblePosition& );
void setBase(const Position&, EAffinity);
void setExtent(const VisiblePosition&);
void setExtent(const Position&, EAffinity);
Position base() const { return m_sel.base(); }
Position extent() const { return m_sel.extent(); }
......@@ -127,33 +109,33 @@ public:
Frame* frame() const;
void nodeWillBeRemoved(Node *);
void nodeWillBeRemoved(Node*);
// Safari Selection Object API
Node *baseNode() const { return m_sel.base().node(); }
Node *extentNode() const { return m_sel.extent().node(); }
Node* baseNode() const { return m_sel.base().node(); }
Node* extentNode() const { return m_sel.extent().node(); }
int baseOffset() const { return m_sel.base().offset(); }
int extentOffset() const { return m_sel.extent().offset(); }
String type() const;
void setBaseAndExtent(Node *baseNode, int baseOffset, Node *extentNode, int extentOffset);
void setPosition(Node *node, int offset);
bool modify(const String &alterString, const String &directionString, const String &granularityString);
void setBaseAndExtent(Node* baseNode, int baseOffset, Node* extentNode, int extentOffset);
void setPosition(Node*, int offset);
bool modify(const String& alterString, const String& directionString, const String& granularityString);
// Mozilla Selection Object API
// In FireFox, anchor/focus are the equal to the start/end of the selection,
// but reflect the direction in which the selection was made by the user. That does
// not mean that they are base/extent, since the base/extent don't reflect
// expansion.
Node *anchorNode() const { return m_sel.isBaseFirst() ? m_sel.start().node() : m_sel.end().node(); }
Node* anchorNode() const { return m_sel.isBaseFirst() ? m_sel.start().node() : m_sel.end().node(); }
int anchorOffset() const { return m_sel.isBaseFirst() ? m_sel.start().offset() : m_sel.end().offset(); }
Node *focusNode() const { return m_sel.isBaseFirst() ? m_sel.end().node() : m_sel.start().node(); }
Node* focusNode() const { return m_sel.isBaseFirst() ? m_sel.end().node() : m_sel.start().node(); }
int focusOffset() const { return m_sel.isBaseFirst() ? m_sel.end().offset() : m_sel.start().offset(); }
bool isCollapsed() const { return !isRange(); }
String toString() const;
void collapse(Node *node, int offset);
void collapse(Node*, int offset);
void collapseToEnd();
void collapseToStart();
void extend(Node *node, int offset);
void extend(Node*, int offset);
PassRefPtr<Range> getRangeAt(int index) const;
//void deleteFromDocument();
//bool containsNode(Node *node, bool entirelyContained);
......@@ -169,7 +151,7 @@ public:
//TextRange *createRange();
#ifndef NDEBUG
void formatForDebugger(char *buffer, unsigned length) const;
void formatForDebugger(char* buffer, unsigned length) const;
void showTreeForThis() const;
#endif
......@@ -200,15 +182,14 @@ private:
bool m_needsLayout : 1; // true if the caret and expectedVisible rectangles need to be calculated
bool m_modifyBiasSet : 1; // true if the selection has been horizontally
// modified with EAlter::EXTEND
RefPtr<MutationListener> m_mutationListener;
};
inline bool operator==(const SelectionController &a, const SelectionController &b)
inline bool operator==(const SelectionController& a, const SelectionController& b)
{
return a.start() == b.start() && a.end() == b.end() && a.affinity() == b.affinity();
}
inline bool operator!=(const SelectionController &a, const SelectionController &b)
inline bool operator!=(const SelectionController& a, const SelectionController& b)
{
return !(a == b);
}
......
......@@ -296,37 +296,52 @@ void HTMLElement::setInnerHTML(const String &html, ExceptionCode& ec)
return;
}
Node* child = firstChild();
if (child && !child->nextSibling()) {
// Optimize the case where the element has exactly one text node as a child,
// and what we're replacing with is also exactly one text node.
if (child->isTextNode()) {
Node* fragmentChild = fragment->firstChild();
if (fragmentChild && !fragmentChild->nextSibling() && fragmentChild->isTextNode()) {
static_cast<Text*>(child)->setData(static_cast<Text*>(fragmentChild)->data(), ec);
return;
}
}
// Optimize the case where the element has exactly one child.
replaceChild(fragment.release(), child, ec);
return;
}
removeChildren();
appendChild(fragment.release(), ec);
}
void HTMLElement::setOuterHTML(const String &html, ExceptionCode& ec)
{
Node *p = parent();
Node* p = parent();
if (!p || !p->isHTMLElement()) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
}
HTMLElement *parent = static_cast<HTMLElement *>(p);
RefPtr<DocumentFragment> fragment = parent->createContextualFragment(html);
HTMLElement* parent = static_cast<HTMLElement*>(p);
RefPtr<DocumentFragment> fragment = parent->createContextualFragment(html);
if (!fragment) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
}
parent->replaceChild(fragment.release(), this, ec);
}
void HTMLElement::setInnerText(const String &text, ExceptionCode& ec)
void HTMLElement::setInnerText(const String& text, ExceptionCode& ec)
{
// following the IE specs.
// follow the IE specs about when this is allowed
if (endTagRequirement() == TagStatusForbidden) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
}
if (hasLocalName(colTag) || hasLocalName(colgroupTag) || hasLocalName(framesetTag) ||
hasLocalName(headTag) || hasLocalName(htmlTag) || hasLocalName(tableTag) ||
hasLocalName(tbodyTag) || hasLocalName(tfootTag) || hasLocalName(theadTag) ||
......@@ -335,18 +350,30 @@ void HTMLElement::setInnerText(const String &text, ExceptionCode& ec)
return;
}
Node* child = firstChild();
if (child && !child->nextSibling()) {
// Optimize the case where the element already has exactly one text node as a child.
if (child->isTextNode()) {
static_cast<Text*>(child)->setData(text, ec);
return;
}
// Optimize the case where the element has exactly one child.
replaceChild(new Text(document(), text), child, ec);
return;
}
removeChildren();
appendChild(new Text(document(), text), ec);
}
void HTMLElement::setOuterText(const String &text, ExceptionCode& ec)
{
// following the IE specs.
// follow the IE specs about when this is allowed
if (endTagRequirement() == TagStatusForbidden) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
}
if (hasLocalName(colTag) || hasLocalName(colgroupTag) || hasLocalName(framesetTag) ||
hasLocalName(headTag) || hasLocalName(htmlTag) || hasLocalName(tableTag) ||
hasLocalName(tbodyTag) || hasLocalName(tfootTag) || hasLocalName(theadTag) ||
......@@ -355,8 +382,7 @@ void HTMLElement::setOuterText(const String &text, ExceptionCode& ec)
return;
}
Node *parent = parentNode();
Node* parent = parentNode();
if (!parent) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
......@@ -369,9 +395,9 @@ void HTMLElement::setOuterText(const String &text, ExceptionCode& ec)
return;
// is previous node a text node? if so, merge into it
Node *prev = t->previousSibling();
Node* prev = t->previousSibling();
if (prev && prev->isTextNode()) {
Text *textPrev = static_cast<Text *>(prev);
Text* textPrev = static_cast<Text*>(prev);
textPrev->appendData(t->data(), ec);
if (ec)
return;
......@@ -382,9 +408,9 @@ void HTMLElement::setOuterText(const String &text, ExceptionCode& ec)
}
// is next node a text node? if so, merge it in
Node *next = t->nextSibling();
Node* next = t->nextSibling();
if (next && next->isTextNode()) {
Text *textNext = static_cast<Text *>(next);
Text* textNext = static_cast<Text*>(next);
t->appendData(textNext->data(), ec);
if (ec)
return;
......
<div><img width="200" height="200" src="resources/200x200.png"></div>
<div><input onfocus="this.value = ''" value="drop image here go boom"></div>
<p>Drag the image above down into the text field.
If the test succeeds there will be no crash and you'll end up with an empty text field with a blinking caret.
It's arguably a bug that the text field accepts an image at all, so some day we might fix that and make this
test obsolete. Maybe we can find some other way to test the same code path.</p>
Property changes on: manual-tests/input-empty-on-focus.html
___________________________________________________________________
Name: svn:mime-type
+ text/html
Name: svn:eol-style
+ native
......@@ -1092,7 +1092,7 @@ void Frame::setSelectionGranularity(TextGranularity granularity) const
d->m_selectionGranularity = granularity;
}
const SelectionController& Frame::dragCaret() const
SelectionController& Frame::dragCaret() const
{
return d->m_dragCaret;
}
......
......@@ -320,34 +320,17 @@ public:
*/
void setSelectionGranularity(TextGranularity granularity) const;
/**
* Returns the drag caret of the HTML.
*/
const SelectionController& dragCaret() const;
/**
* Sets the current selection.
*/
// FIXME: Replace these with functions on the selection controller.
void setSelection(const SelectionController&, bool closeTyping = true, bool keepTypingStyle = false);
/**
* Returns whether selection can be changed.
*/
bool shouldChangeSelection(const SelectionController&) const;
/**
* Returns a mark, to be used as emacs uses it.
*/
const Selection& mark() const;
/**
* Returns the mark.
*/
void setMark(const Selection&);
/**
* Sets the current drag caret.
*/
// FIXME: Replace this with a function on the selection controller or change it to Selection instead?
void setDragCaret(const SelectionController&);
/**
......@@ -797,7 +780,8 @@ public:
// split out controller objects
FrameTree* tree() const;
SelectionController& selection() const;
SelectionController& selection() const; // FIXME: Change to pointer?
SelectionController& dragCaret() const; // FIXME: Change to pointer?
DOMWindow* domWindow() const;
private:
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment