Title: [167210] trunk
Revision
167210
Author
da...@apple.com
Date
2014-04-13 23:55:15 -0700 (Sun, 13 Apr 2014)

Log Message

REGRESSION (r158617): Find on Page can get stuck in a loop when the search string occurs in an <input> in a <fieldset>
https://bugs.webkit.org/show_bug.cgi?id=126322

Reviewed by Ryosuke Niwa.

Source/WebCore:

* dom/Element.cpp:
(WebCore::Element::canContainRangeEndPoint): Now returns false when the role of the element
is "img". This is the same rule that's hard-coded in isRenderReplacedElement for the same
reason. Need more test coverage to make sure this role feature works consistently.

* dom/Element.h: Made canContainRangeEndPoint no longer inline since it's not just a
return statement any more.

* dom/Position.cpp:
(WebCore::Position::isCandidate): Took out code that calls isRendererReplacedElement
that was added in r158617; not needed now that we updated canContainRangeEndPoint.

* dom/Range.cpp:
(WebCore::Range::firstNode): Removed code here that called isRendererReplacedElement.
This was the wrong level to be adding editing logic, and there's a FIXME here to that
effect, which we are now deleting. This was the change that broke Find.

* editing/TextIterator.cpp: Added a comment about the redundancy between the
isRendererReplacedElement and editingIgnoresContent functions.

* html/HTMLHRElement.cpp:
(WebCore::HTMLHRElement::canContainRangeEndPoint): Call through to base class instead
of just returning true when we have child nodes. Lets Element::canContainRangeEndPoint
do its thing.
* html/HTMLHRElement.h: Ditto.

* html/HTMLObjectElement.cpp:
(WebCore::HTMLObjectElement::canContainRangeEndPoint): Call through to base class instead
of just returning true when we have fallback content. Lets Element::canContainRangeEndPoint
do its thing.
* html/HTMLObjectElement.h: Ditto.

* testing/Internals.cpp:
(WebCore::Internals::countMatchesForText): Set the limit to 1000 instead of infinite.

LayoutTests:

* editing/text-iterator/count-matches-in-form-expected.txt: Added.
* editing/text-iterator/count-matches-in-form.html: Added.

* fast/text/window-find.html: Tweaked the test a bit, making it a little easier to
see if the test hasn't even run.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (167209 => 167210)


--- trunk/LayoutTests/ChangeLog	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/LayoutTests/ChangeLog	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1,3 +1,16 @@
+2014-04-13  Darin Adler  <da...@apple.com>
+
+        REGRESSION (r158617): Find on Page can get stuck in a loop when the search string occurs in an <input> in a <fieldset>
+        https://bugs.webkit.org/show_bug.cgi?id=126322
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/text-iterator/count-matches-in-form-expected.txt: Added.
+        * editing/text-iterator/count-matches-in-form.html: Added.
+
+        * fast/text/window-find.html: Tweaked the test a bit, making it a little easier to
+        see if the test hasn't even run.
+
 2014-04-08  Oliver Hunt  <oli...@apple.com>
 
         Rewrite Function.bind as a builtin

Added: trunk/LayoutTests/editing/text-iterator/count-matches-in-form-expected.txt (0 => 167210)


--- trunk/LayoutTests/editing/text-iterator/count-matches-in-form-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/count-matches-in-form-expected.txt	2014-04-14 06:55:15 UTC (rev 167210)
@@ -0,0 +1,5 @@
+PASS internals.countMatchesForText('rule', 23, '') is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Property changes on: trunk/LayoutTests/editing/text-iterator/count-matches-in-form-expected.txt
___________________________________________________________________

Added: svn:eol-style

Added: trunk/LayoutTests/editing/text-iterator/count-matches-in-form.html (0 => 167210)


--- trunk/LayoutTests/editing/text-iterator/count-matches-in-form.html	                        (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/count-matches-in-form.html	2014-04-14 06:55:15 UTC (rev 167210)
@@ -0,0 +1,8 @@
+<head>
+<script src=""
+<body>
+<fieldset><input value="rule"></fieldset>
+<script>
+shouldBe("internals.countMatchesForText('rule', 23, '')", "1");
+</script>
+<script src=""
Property changes on: trunk/LayoutTests/editing/text-iterator/count-matches-in-form.html
___________________________________________________________________

Added: svn:mime-type

Added: svn:eol-style

Modified: trunk/LayoutTests/fast/text/window-find.html (167209 => 167210)


--- trunk/LayoutTests/fast/text/window-find.html	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/LayoutTests/fast/text/window-find.html	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1,50 +1,45 @@
 <html>
 <head>
 <script>
-if (window.testRunner) {
-  testRunner.dumpAsText();
-  testRunner.waitUntilDone();
-}
 
+if (window.testRunner)
+    testRunner.dumpAsText();
+
 function fail(s) {
-  document.body.innerHTML = "FAIL: " + s;
+    document.body.innerHTML = "FAIL: " + s;
 }
 
 function runTest() {
-  // Need to force layout for window.find() to operate correctly.
-  var forceLayout = document.body.offsetHeight;
-  if (window.find('nonsense')) fail('found: nonsense');
-  // https://bugs.webkit.org/show_bug.cgi?id=53654 -- failure when flipping
-  // case sensitivity back-to-back.
-  if (window.find('nonsense', true)) fail('found: nonsense');
-  if (window.find('nonsense', false)) fail('found: nonsense');
-  if (!window.find('for')) fail('not found: for');
-  if (window.find('for')) fail('found: for');
-  // Go backwards.
-  if (!window.find('test', true, true, false)) fail('not found: test');
-  if (window.find('for', true, true, false)) fail('found: for');
-  // Backwards and case sensitivity.
-  if (window.find('this', true, true, false)) fail('found: this');
-  if (!window.find('This', true, true, false)) fail('not found: This');
-  // Wrap-around forwards.
-  if (!window.find('for', true, false, true)) fail('not found: for');
-  if (!window.find('for', true, false, true)) fail('not found: for');
-  // Wrap-around backwards.
-  if (!window.find('for', true, true, true)) fail('not found: for');
-  if (!window.find('for', true, true, true)) fail('not found: for');
-  // Case sensitivity, forwards.
-  if (!window.find('for', true, false, true)) fail('not found: for');
-  if (!window.find('fOR', false, false, true)) fail('not found: for');
-  if (!window.find('for', false, false, true)) fail('not found: for');
-  if (!window.find('for', true, false, true)) fail('not found: for');
-  if (window.find('FOR', true, false, true)) fail('found: FOR');
+    if (window.find('nonsense')) fail('found: nonsense');
+    if (window.find('nonsense', true)) fail('found: nonsense');
+    if (window.find('nonsense', false)) fail('found: nonsense');
+    if (!window.find('for')) fail('not found: for');
+    if (window.find('for')) fail('found: for');
+    // Go backwards.
+    if (!window.find('test', true, true, false)) fail('not found: test');
+    if (window.find('for', true, true, false)) fail('found: for');
+    // Backwards and case sensitivity.
+    if (window.find('this', true, true, false)) fail('found: this');
+    if (!window.find('This', true, true, false)) fail('not found: This');
+    // Wrap-around forwards.
+    if (!window.find('for', true, false, true)) fail('not found: for');
+    if (!window.find('for', true, false, true)) fail('not found: for');
+    // Wrap-around backwards.
+    if (!window.find('for', true, true, true)) fail('not found: for');
+    if (!window.find('for', true, true, true)) fail('not found: for');
+    // Case sensitivity, forwards.
+    if (!window.find('for', true, false, true)) fail('not found: for');
+    if (!window.find('fOR', false, false, true)) fail('not found: for');
+    if (!window.find('for', false, false, true)) fail('not found: for');
+    if (!window.find('for', true, false, true)) fail('not found: for');
+    if (window.find('FOR', true, false, true)) fail('found: FOR');
 
-  if (window.testRunner)
-    testRunner.notifyDone();
+    document.body.innerHTML = "This is a test for window.find(). SUCCESS!";
 }
+
 </script>
 </head>
 <body _onload_="runTest()">
-This is a test for window.find(). SUCCESS!
+This is a test for window.find(); the test has not run yet.
 </body>
 </html>

Modified: trunk/Source/WebCore/ChangeLog (167209 => 167210)


--- trunk/Source/WebCore/ChangeLog	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/ChangeLog	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1,3 +1,45 @@
+2014-04-13  Darin Adler  <da...@apple.com>
+
+        REGRESSION (r158617): Find on Page can get stuck in a loop when the search string occurs in an <input> in a <fieldset>
+        https://bugs.webkit.org/show_bug.cgi?id=126322
+
+        Reviewed by Ryosuke Niwa.
+
+        * dom/Element.cpp:
+        (WebCore::Element::canContainRangeEndPoint): Now returns false when the role of the element
+        is "img". This is the same rule that's hard-coded in isRenderReplacedElement for the same
+        reason. Need more test coverage to make sure this role feature works consistently.
+
+        * dom/Element.h: Made canContainRangeEndPoint no longer inline since it's not just a
+        return statement any more.
+
+        * dom/Position.cpp:
+        (WebCore::Position::isCandidate): Took out code that calls isRendererReplacedElement
+        that was added in r158617; not needed now that we updated canContainRangeEndPoint.
+
+        * dom/Range.cpp:
+        (WebCore::Range::firstNode): Removed code here that called isRendererReplacedElement.
+        This was the wrong level to be adding editing logic, and there's a FIXME here to that
+        effect, which we are now deleting. This was the change that broke Find.
+
+        * editing/TextIterator.cpp: Added a comment about the redundancy between the
+        isRendererReplacedElement and editingIgnoresContent functions.
+
+        * html/HTMLHRElement.cpp:
+        (WebCore::HTMLHRElement::canContainRangeEndPoint): Call through to base class instead
+        of just returning true when we have child nodes. Lets Element::canContainRangeEndPoint
+        do its thing.
+        * html/HTMLHRElement.h: Ditto.
+
+        * html/HTMLObjectElement.cpp:
+        (WebCore::HTMLObjectElement::canContainRangeEndPoint): Call through to base class instead
+        of just returning true when we have fallback content. Lets Element::canContainRangeEndPoint
+        do its thing.
+        * html/HTMLObjectElement.h: Ditto.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::countMatchesForText): Set the limit to 1000 instead of infinite.
+
 2014-04-12  Darin Adler  <da...@apple.com>
 
         Use unique_ptr for FillLayer::m_next

Modified: trunk/Source/WebCore/accessibility/AccessibilityObject.cpp (167209 => 167210)


--- trunk/Source/WebCore/accessibility/AccessibilityObject.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/accessibility/AccessibilityObject.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1069,7 +1069,7 @@
     // when iterating text.
     return listItem->markerTextWithSuffix();
 }
-    
+
 String AccessibilityObject::stringForVisiblePositionRange(const VisiblePositionRange& visiblePositionRange) const
 {
     if (visiblePositionRange.isNull())

Modified: trunk/Source/WebCore/dom/Element.cpp (167209 => 167210)


--- trunk/Source/WebCore/dom/Element.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/dom/Element.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -2939,4 +2939,9 @@
     ensureElementRareData().setHasPendingResources(false);
 }
 
+bool Element::canContainRangeEndPoint() const
+{
+    return !equalIgnoringCase(fastGetAttribute(roleAttr), "img");
+}
+
 } // namespace WebCore

Modified: trunk/Source/WebCore/dom/Element.h (167209 => 167210)


--- trunk/Source/WebCore/dom/Element.h	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/dom/Element.h	2014-04-14 06:55:15 UTC (rev 167210)
@@ -484,7 +484,7 @@
     virtual bool isFrameElementBase() const { return false; }
     virtual bool isSearchFieldCancelButtonElement() const { return false; }
 
-    virtual bool canContainRangeEndPoint() const override { return true; }
+    virtual bool canContainRangeEndPoint() const override;
 
     // Used for disabled form elements; if true, prevents mouse events from being dispatched
     // to event listeners, and prevents DOMActivate events from being sent at all.

Modified: trunk/Source/WebCore/dom/Position.cpp (167209 => 167210)


--- trunk/Source/WebCore/dom/Position.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/dom/Position.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -224,6 +224,7 @@
             return positionInParentBeforeNode(m_anchorNode.get());
         return Position(m_anchorNode.get(), 0, PositionIsOffsetInAnchor);
     }
+
     if (!m_anchorNode->offsetInCharacters()
         && (m_anchorType == PositionIsAfterAnchor || m_anchorType == PositionIsAfterChildren || static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount())
         && (editingIgnoresContent(m_anchorNode.get()) || isRenderedTable(m_anchorNode.get()))
@@ -935,9 +936,6 @@
     if (m_anchorNode->hasTagName(htmlTag))
         return false;
         
-    if (isRendererReplacedElement(renderer))
-        return !nodeIsUserSelectNone(deprecatedNode()) && atFirstEditingPositionForNode();
-
     if (renderer->isRenderBlockFlow()) {
         RenderBlockFlow& block = toRenderBlockFlow(*renderer);
         if (block.logicalHeight() || m_anchorNode->hasTagName(bodyTag)) {

Modified: trunk/Source/WebCore/dom/Range.cpp (167209 => 167210)


--- trunk/Source/WebCore/dom/Range.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/dom/Range.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1570,10 +1570,6 @@
         return 0;
     if (m_start.container()->offsetInCharacters())
         return m_start.container();
-    // FIXME: A renderer-based rule is completely out of place here.
-    // For one thing, the renderer could be out of date if the DOM was recently changed.
-    if (isRendererReplacedElement(m_start.container()->renderer()))
-        return m_start.container();
     if (Node* child = m_start.container()->childNode(m_start.offset()))
         return child;
     if (!m_start.offset())

Modified: trunk/Source/WebCore/editing/TextIterator.cpp (167209 => 167210)


--- trunk/Source/WebCore/editing/TextIterator.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/editing/TextIterator.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -242,6 +242,8 @@
     ASSERT(stack.size() == 1 + depthCrossingShadowBoundaries(node));
 }
 
+// FIXME: editingIgnoresContent and isRendererReplacedElement try to do the same job.
+// It's not good to have both of them.
 bool isRendererReplacedElement(RenderObject* renderer)
 {
     if (!renderer)

Modified: trunk/Source/WebCore/html/HTMLHRElement.cpp (167209 => 167210)


--- trunk/Source/WebCore/html/HTMLHRElement.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/html/HTMLHRElement.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -100,4 +100,9 @@
         HTMLElement::collectStyleForPresentationAttribute(name, value, style);
 }
 
+bool HTMLHRElement::canContainRangeEndPoint() const
+{
+    return hasChildNodes() && HTMLElement::canContainRangeEndPoint();
 }
+
+}

Modified: trunk/Source/WebCore/html/HTMLHRElement.h (167209 => 167210)


--- trunk/Source/WebCore/html/HTMLHRElement.h	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/html/HTMLHRElement.h	2014-04-14 06:55:15 UTC (rev 167210)
@@ -32,13 +32,12 @@
     static PassRefPtr<HTMLHRElement> create(Document&);
     static PassRefPtr<HTMLHRElement> create(const QualifiedName&, Document&);
 
-    virtual bool canContainRangeEndPoint() const override { return hasChildNodes(); }
-
 private:
     HTMLHRElement(const QualifiedName&, Document&);
 
     virtual bool isPresentationAttribute(const QualifiedName&) const override;
     virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) override;
+    virtual bool canContainRangeEndPoint() const override;
 };
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/html/HTMLObjectElement.cpp (167209 => 167210)


--- trunk/Source/WebCore/html/HTMLObjectElement.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/html/HTMLObjectElement.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -511,4 +511,9 @@
     return FormAssociatedElement::form();
 }
 
+bool HTMLObjectElement::canContainRangeEndPoint() const
+{
+    return m_useFallbackContent && HTMLPlugInImageElement::canContainRangeEndPoint();
 }
+
+}

Modified: trunk/Source/WebCore/html/HTMLObjectElement.h (167209 => 167210)


--- trunk/Source/WebCore/html/HTMLObjectElement.h	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/html/HTMLObjectElement.h	2014-04-14 06:55:15 UTC (rev 167210)
@@ -97,7 +97,7 @@
     virtual bool isEnumeratable() const override { return true; }
     virtual bool appendFormData(FormDataList&, bool) override;
 
-    virtual bool canContainRangeEndPoint() const override { return useFallbackContent(); }
+    virtual bool canContainRangeEndPoint() const override;
 
     bool m_docNamedItem : 1;
     bool m_useFallbackContent : 1;

Modified: trunk/Source/WebCore/testing/Internals.cpp (167209 => 167210)


--- trunk/Source/WebCore/testing/Internals.cpp	2014-04-14 05:29:47 UTC (rev 167209)
+++ trunk/Source/WebCore/testing/Internals.cpp	2014-04-14 06:55:15 UTC (rev 167210)
@@ -1435,7 +1435,7 @@
         return 0;
 
     bool mark = markMatches == "mark";
-    return document->frame()->editor().countMatchesForText(text, nullptr, findOptions, std::numeric_limits<unsigned>::max(), mark, nullptr);
+    return document->frame()->editor().countMatchesForText(text, nullptr, findOptions, 1000, mark, nullptr);
 }
 
 const ProfilesArray& Internals::consoleProfiles() const
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to