Bug 52241 – REGRESSION(r69831): focus() in onkeypress discards input (affects chaseonline.chase.com)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Gentilcore]

Created attachment 78581 [details]
Testcase

Changing focus during the onkeypress handler causes the pressed key to be dropped (see attached testcase). This makes it impossible to enter a social security number on the Chase bank website.

In debug builds, I'm getting:
ASSERTION FAILED: !ec
(/Volumes/work/WebKit/Source/WebCore/dom/Range.cpp:86 WebCore::Range::Range(WTF::PassRefPtr<WebCore::Document>, WTF::PassRefPtr<WebCore::Node>, int, WTF::PassRefPtr<WebCore::Node>, int))

Copying a few people who have modified Range.cpp recently just in case they have a better idea about this.

Filed in chromium as http://crbug.com/69126

[Alexey Proskuryakov]

This is a regression from Safari 5. WebKit2 also fails, but with a different assertion.

[Ryosuke Niwa]

Works:
r68833
r69429
r69026
r69221
r69647
r69728

Does not:
r69836
r70098

[Ryosuke Niwa]

Between these two revisions, WebCore/editing had the following changesets:
http://trac.webkit.org/changeset/69831
http://trac.webkit.org/changeset/69836

[Ryosuke Niwa]

I confirmed that the bug reproduces on r69831 and doesn't reproduce on r69830.

[Tony Gentilcore]

(In reply to comment #4)
> I confirmed that the bug reproduces on r69831 and doesn't reproduce on r69830.

+Justin

It is also worth noting that patch is merged onto 517 and 552.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > I confirmed that the bug reproduces on r69831 and doesn't reproduce on r69830.
> 
> +Justin
> 
> It is also worth noting that patch is merged onto 517 and 552.

Actually, the original patch was written by me.  Justin just committed it.  I should be able to post a patch tonight or within the next couple of days.

[Ryosuke Niwa]

Created attachment 78758 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #7)
> Created an attachment (id=78758) [details]
> fixes the bug

It turned out that the root cause of this bug was present before r69831 but VisiblePosition was more "forgiving" for historical reasons (e.g. legacy editing position).

[Eric Seidel (no email)]

Comment on attachment 78758 [details]
fixes the bug

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

> Source/WebCore/rendering/RenderTextControl.cpp:311
> +        if (offset <= start && start <= offset + length) {
> +            if (node->isTextNode()) {
> +            } else {
> +            }
> +        if (offset <= end && end <= offset + length) {
> +                end -= offset;
> +                endNode = node->parentNode();
> +            }
> +            break;
> +        }

These two blocks are basically identical.  Seems we should use an inline function here instead.

[Ryosuke Niwa]

Created attachment 78764 [details]
fixed per Eric's comment

[Eric Seidel (no email)]

Comment on attachment 78764 [details]
fixed per Eric's comment

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

Seems sane.

> LayoutTests/fast/forms/focus-change-on-keypress.html:36
> +    if (shouldChangeSelection)
> +        expected = name == 'textarea' ? 'ac\n' : 'acb';
> +    else
> +        expected = name == 'textarea' ? 'a\nc' : 'abc';

This seems slightly odd way to write this if, but oK. :)

[Ryosuke Niwa]

(In reply to comment #11)
> (From update of attachment 78764 [details])
> > LayoutTests/fast/forms/focus-change-on-keypress.html:36
> > +    if (shouldChangeSelection)
> > +        expected = name == 'textarea' ? 'ac\n' : 'acb';
> > +    else
> > +        expected = name == 'textarea' ? 'a\nc' : 'abc';
> 
> This seems slightly odd way to write this if, but oK. :)

Maybe I should do:

if (name == 'textarea')
    expected = shouldChangeSelection ? 'ac\n' : 'a\nc'
else
    expected = shouldChangeSelection ? 'acb' : 'abc'

instead?

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 78764 [details] [details])
> > > LayoutTests/fast/forms/focus-change-on-keypress.html:36
> > > +    if (shouldChangeSelection)
> > > +        expected = name == 'textarea' ? 'ac\n' : 'acb';
> > > +    else
> > > +        expected = name == 'textarea' ? 'a\nc' : 'abc';
> > 
> > This seems slightly odd way to write this if, but oK. :)
> 
> Maybe I should do:
> 
> if (name == 'textarea')
>     expected = shouldChangeSelection ? 'ac\n' : 'a\nc'
> else
>     expected = shouldChangeSelection ? 'acb' : 'abc'
> 
> instead?

Sorry I read those both as 'abc', now I understand why the complexity is needed.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > Maybe I should do:
> > 
> > if (name == 'textarea')
> >     expected = shouldChangeSelection ? 'ac\n' : 'a\nc'
> > else
> >     expected = shouldChangeSelection ? 'acb' : 'abc'
> > 
> > instead?
> 
> Sorry I read those both as 'abc', now I understand why the complexity is needed.

No problem.  I'll use new one nonetheless because it reads better.

[David Kilzer (:ddkilzer)]

<rdar://problem/8858076>

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/75677.