Bug 126322 – REGRESSION (r158617): Find on Page can get stuck in a loop when the search string occurs in an <input> in a <fieldset>

• Component: HTML Editing
• Status: RESOLVED FIXED

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.

<rdar://problem/15733725>

[Darin Adler]

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.

[Darin Adler]

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?

[chris fleizach]

(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

[Darin Adler]

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.

[Darin Adler]

Created attachment 229236 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 229259 [details]
Patch

[Ryosuke Niwa]

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?

[Darin Adler]

(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.

[Darin Adler]

Committed r167210: <http://trac.webkit.org/changeset/167210>

[Alexey Proskuryakov]

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

[Darin Adler]

Committed r167291: <http://trac.webkit.org/changeset/167291>

[Darin Adler]

The followup fixes the problem seen on the bots/flakiness dashboard.

[Alexey Proskuryakov]

Confirmed, thank you!