Bug 126322 - REGRESSION (r158617): Find on Page can get stuck in a loop when the search string occurs in an <input> in a <fieldset>
Summary: REGRESSION (r158617): Find on Page can get stuck in a loop when the search st...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Darin Adler
URL: data:text/html,<fieldset><input%20val...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2013-12-30 21:05 PST by mitz
Modified: 2014-04-14 23:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2014-04-13 11:22 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (12.78 KB, patch)
2014-04-13 21:23 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2013-12-30 21:05:12 PST
To reproduce: navigate to the URL in Safari, choose Edit > Find > Find, and in the Find bar, type “rule” (without the quotes). Notice how Safari reports more than 1,000 matches.

This was caused by <http://trac.webkit.org/r158617>, the fix for bug 123697.
Comment 1 mitz 2013-12-30 21:54:22 PST
<rdar://problem/15733725>
Comment 2 Darin Adler 2014-04-13 01:11:04 PDT
The problem with http://trac.webkit.org/r158617 is that it changes the behavior of the low-level function Range::firstNode. I’m going to roll that part of the patch out and then see what happens to the accessibility behavior.
Comment 3 Darin Adler 2014-04-13 01:17:04 PDT
I just tested Safari 7.0.3 and it also shows the "more then 1,000 matches" behavior. Does that make sense given that http://trac.webkit.org/r158617 should not be there in the WebKit used by Safari 7.0.3?
Comment 4 chris fleizach 2014-04-13 01:24:08 PDT
(In reply to comment #3)
> I just tested Safari 7.0.3 and it also shows the "more then 1,000 matches" behavior. Does that make sense given that http://trac.webkit.org/r158617 should not be there in the WebKit used by Safari 7.0.3?

I think this change went into the 7.0.3 update
Comment 5 Darin Adler 2014-04-13 10:08:05 PDT
Yes, removing the incorrect change to Range::firstNode does indeed fix the find problem right away.

But it reintroduces the problem from bug 123697.

The core of the problem is trying to make elements with rendered children work the way that <img> elements work with our editing code. Unfortunately, for <img> elements, we are still stuck with the old model where we represent “before the image” as (<img>, 0) in a lot of code; parent node <img>, offset 0. Which in actual Range semantics means “inside the image”, not “before the image“.

We have a long term plan to get rid of this, but since it’s still around it causes us major headaches.

Here, the problem is that for the <div role="image">, (<div>, 0) can mean either “before the image” or “inside the image”. The hacks that make this almost work are getting out of hand at this point.

Instead of modifying firstNode, we need to modify Position::parentAnchoredEquivalent to handle this case.
Comment 6 Darin Adler 2014-04-13 11:22:12 PDT
Created attachment 229236 [details]
Patch
Comment 7 Darin Adler 2014-04-13 11:23:02 PDT
The patch is ready to go and passes all the existing tests. But before landing it we should really add a new test for the actual problem with find. That’s the only thing preventing me from putting this up for review and landing it.
Comment 8 Darin Adler 2014-04-13 21:23:30 PDT
Created attachment 229259 [details]
Patch
Comment 9 Ryosuke Niwa 2014-04-13 21:54:14 PDT
Comment on attachment 229259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229259&action=review

> Source/WebCore/dom/Element.cpp:2944
> +    return !equalIgnoringCase(fastGetAttribute(roleAttr), "img");

Should other element subclasses that override this method check this condition as well?
Comment 10 Darin Adler 2014-04-13 21:58:58 PDT
(In reply to comment #9)
> Should other element subclasses that override this method check this condition as well?

The only element classes that override this for any reason other than to unconditionally return false are HTMLObjectElement and HTMLHRElement. I’m not sure that either of them really needs to support role="image", but I could change them to call through to the base class.
Comment 11 Darin Adler 2014-04-13 23:55:40 PDT
Committed r167210: <http://trac.webkit.org/changeset/167210>
Comment 12 Alexey Proskuryakov 2014-04-14 12:53:57 PDT
This change broke a test, editing/editability/ignored-content.html either fails or asserts: http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Feditability%2Fignored-content.html
Comment 13 Darin Adler 2014-04-14 20:06:46 PDT
Committed r167291: <http://trac.webkit.org/changeset/167291>
Comment 14 Darin Adler 2014-04-14 20:07:23 PDT
The followup fixes the problem seen on the bots/flakiness dashboard.
Comment 15 Alexey Proskuryakov 2014-04-14 23:26:54 PDT
Confirmed, thank you!