Bug 188322 - [iOS] Caret disappears after resigning and becoming first responder if active focus state is retained
Summary: [iOS] Caret disappears after resigning and becoming first responder if active...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-03 16:38 PDT by Wenson Hsieh
Modified: 2018-10-22 23:54 PDT (History)
14 users (show)

See Also:


Attachments
Patch (27.94 KB, patch)
2018-08-03 17:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix macOS builds (27.10 KB, patch)
2018-08-03 17:53 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.96 MB, application/zip)
2018-08-04 15:25 PDT, EWS Watchlist
no flags Details
Tweak Windows TestExpectations (28.04 KB, patch)
2018-08-04 15:34 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (13.03 MB, application/zip)
2018-08-04 17:07 PDT, EWS Watchlist
no flags Details
Adjust the API test (3.15 KB, patch)
2018-08-06 11:06 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2018-08-06 20:54 PDT, Wenson Hsieh
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-08-03 16:38:06 PDT
<rdar://problem/42455270>
Comment 1 Wenson Hsieh 2018-08-03 16:39:41 PDT
Began happening after r230745. Causes the caret to disappear in iOS Safari on iPad if the user taps the URL bar, and then the page.
Comment 2 Wenson Hsieh 2018-08-03 17:22:51 PDT
Created attachment 346566 [details]
Patch
Comment 3 Wenson Hsieh 2018-08-03 17:53:29 PDT
Created attachment 346569 [details]
Fix macOS builds
Comment 4 EWS Watchlist 2018-08-04 15:24:56 PDT
Comment on attachment 346569 [details]
Fix macOS builds

Attachment 346569 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8763987

New failing tests:
editing/selection/ios/selection-handles-after-touch-end.html
imported/blink/transitions/unprefixed-transform.html
Comment 5 EWS Watchlist 2018-08-04 15:25:08 PDT Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2018-08-04 15:34:04 PDT
Created attachment 346596 [details]
Tweak Windows TestExpectations
Comment 7 EWS Watchlist 2018-08-04 17:07:21 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-08-04 17:07:32 PDT Comment hidden (obsolete)
Comment 9 Tim Horton 2018-08-05 21:53:44 PDT
Comment on attachment 346596 [details]
Tweak Windows TestExpectations

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

> LayoutTests/ChangeLog:10
> +        Adds a new layout test to verify that (1) selection handles are not shown when selecting a word by long
> +        pressing prior to ending the touch, and (2) selection handles are shown after ending the touch.

V. nice.
Comment 10 Wenson Hsieh 2018-08-06 07:09:00 PDT
Comment on attachment 346596 [details]
Tweak Windows TestExpectations

Thanks for the review!
Comment 11 WebKit Commit Bot 2018-08-06 07:14:49 PDT
Comment on attachment 346596 [details]
Tweak Windows TestExpectations

Clearing flags on attachment: 346596

Committed r234600: <https://trac.webkit.org/changeset/234600>
Comment 12 WebKit Commit Bot 2018-08-06 07:14:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Wenson Hsieh 2018-08-06 10:56:11 PDT
Comment on attachment 346596 [details]
Tweak Windows TestExpectations

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

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:127
> +    auto expectedCaretRect = CGRectMake(16, 13, 3, 15);

Oops, this test expectation is actually wrong (I missed updating the expected width after adding some logic to convert the caret rect to content view coordinates). I'll put out a fix for this in a bit...
Comment 14 Wenson Hsieh 2018-08-06 11:06:29 PDT
Reopening to attach new patch.
Comment 15 Wenson Hsieh 2018-08-06 11:06:30 PDT
Created attachment 346635 [details]
Adjust the API test
Comment 16 WebKit Commit Bot 2018-08-06 12:31:13 PDT
Comment on attachment 346635 [details]
Adjust the API test

Clearing flags on attachment: 346635

Committed r234614: <https://trac.webkit.org/changeset/234614>
Comment 17 WebKit Commit Bot 2018-08-06 12:31:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Darin Adler 2018-08-06 19:13:18 PDT
Comment on attachment 346635 [details]
Adjust the API test

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

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:41
> +    return CGRectMake(roundl(rect.origin.x), roundl(rect.origin.y), roundl(rect.size.width), roundl(rect.size.height));

The use of roundl here is peculiar and almost certainly wrong; that function is the version that takes and returns a long double. Why are we converting these doubles to long doubles? I suggest using std::round here, which is overloaded for various types rather than the C round where you have to use the correct name for each type.

Separately, I am really unclear why rounding to integers is the right thing to do. But it’s in a test so I won’t probe too hard on that question.
Comment 19 Wenson Hsieh 2018-08-06 20:47:06 PDT
Comment on attachment 346635 [details]
Adjust the API test

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

>> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:41
>> +    return CGRectMake(roundl(rect.origin.x), roundl(rect.origin.y), roundl(rect.size.width), roundl(rect.size.height));
> 
> The use of roundl here is peculiar and almost certainly wrong; that function is the version that takes and returns a long double. Why are we converting these doubles to long doubles? I suggest using std::round here, which is overloaded for various types rather than the C round where you have to use the correct name for each type.
> 
> Separately, I am really unclear why rounding to integers is the right thing to do. But it’s in a test so I won’t probe too hard on that question.

Got it — I'll change this to std::round.

I'm rounding here because the caret view's frame converted to content view coordinates is very close to (but not exactly) {{ 16, 13 }, { 2, 15 }}.
Comment 20 Wenson Hsieh 2018-08-06 20:54:23 PDT
Reopening to attach new patch.
Comment 21 Wenson Hsieh 2018-08-06 20:54:24 PDT
Created attachment 346682 [details]
Patch
Comment 22 Sam Weinig 2018-08-07 09:10:39 PDT
Comment on attachment 346682 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:36
> +#import <wtf/MathExtras.h>

I think <cmath> would be fine here.
Comment 23 Wenson Hsieh 2018-08-07 09:21:50 PDT
Comment on attachment 346682 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:36
>> +#import <wtf/MathExtras.h>
> 
> I think <cmath> would be fine here.

👍
Comment 24 Wenson Hsieh 2018-08-07 10:22:45 PDT
https://trac.webkit.org/changeset/234657/webkit(In reply to Sam Weinig from comment #22)
> Comment on attachment 346682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346682&action=review
> 
> > Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:36
> > +#import <wtf/MathExtras.h>
> 
> I think <cmath> would be fine here.

Committed r234657: <https://trac.webkit.org/changeset/234657/webkit>