Bug 134309 - [iOS][WK2] Distant focusable element may not be scrolled into view when focused using keyboard
Summary: [iOS][WK2] Distant focusable element may not be scrolled into view when focus...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-25 15:04 PDT by Daniel Bates
Modified: 2014-06-26 21:31 PDT (History)
9 users (show)

See Also:

Attachments
Example (901 bytes, text/html)
2014-06-25 15:04 PDT, Daniel Bates 		no flags Details
Patch and manual test (9.00 KB, patch)
2014-06-25 15:21 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and manual test (7.30 KB, patch)
2014-06-25 16:54 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and manual test (6.35 KB, patch)
2014-06-26 08:58 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and manual test (6.31 KB, patch)
2014-06-26 13:22 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-06-25 15:04:45 PDT 
Created attachment 233839 [details]
Example

Focusing a distant focusable element using the keyboard accessory may not scroll the newly focused element into view,

1. Open the attached test case, example.html, in Safari for iOS.
2. Follow the instructions in the test case.
Comment 1 Daniel Bates 2014-06-25 15:05:00 PDT 
<rdar://problem/17427385>
Comment 2 Daniel Bates 2014-06-25 15:21:35 PDT 
Created attachment 233841 [details]
Patch and manual test

I'm unclear how to write a DRT test for this change. Maybe we can write a TestWebKitAPI test? I'm open to suggestions. For now, I included a manual test that is similar to example attached to this bug (attachment #233839 [details]).
Comment 3 Daniel Bates 2014-06-25 15:23:45 PDT 
The approach taken in this patch was inspired by an in-person discussion yesterday (06/24) with Enrica Casucci.
Comment 4 Enrica Casucci 2014-06-25 15:40:00 PDT 
Comment on attachment 233841 [details]
Patch and manual test

I don't think you need to do it in the WebProcess. You know in the UIProcess if the < > buttons have been pressed and you can replace the selection rect with an empty rect before calling zoomToFocusNode.
Comment 5 Daniel Bates 2014-06-25 16:54:42 PDT 
Created attachment 233851 [details]
Patch and manual test

Updated patch based on Enrica Casucci's feedback.
Comment 6 Benjamin Poulain 2014-06-26 00:39:49 PDT 
Comment on attachment 233851 [details]
Patch and manual test

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:139
> +    BOOL _didAccessoryTabInitiateFocus;

Don't forget to clear this in the crash handler.
Comment 7 Daniel Bates 2014-06-26 08:58:34 PDT 
Created attachment 233903 [details]
Patch and manual test

Upated patch based on Benjamin Poulain's feedback; clear _didAccessoryTabInitiateFocus in -[WKContentView cleanupInteraction]
Comment 8 Daniel Bates 2014-06-26 13:22:30 PDT 
Created attachment 233929 [details]
Patch and manual test

Rebased patch
Comment 9 WebKit Commit Bot 2014-06-26 13:24:02 PDT 
Attachment 233929 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:628:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Daniel Bates 2014-06-26 21:31:50 PDT 
Comment on attachment 233929 [details]
Patch and manual test

Clearing flags on attachment: 233929

Committed r170518: <http://trac.webkit.org/changeset/170518>
Comment 11 Daniel Bates 2014-06-26 21:31:57 PDT 
All reviewed patches have been landed.  Closing bug.