Bug 188124 - [iOS] WKColorPicker's selection indicator doesn't always cover the selected swatch
Summary: [iOS] WKColorPicker's selection indicator doesn't always cover the selected s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 188207
  Show dependency treegraph
 
Reported: 2018-07-27 14:00 PDT by Aditya Keerthi
Modified: 2018-08-01 20:26 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.06 KB, patch)
2018-07-27 14:45 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (13.14 MB, application/zip)
2018-07-28 11:47 PDT, Build Bot
no flags Details
Patch (8.10 KB, patch)
2018-07-31 16:48 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2018-07-31 17:27 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (8.26 KB, patch)
2018-07-31 17:41 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2018-07-31 20:26 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2018-07-27 14:00:49 PDT
On iPhone, the selection indicator should resize appropriately on rotation.

On iPad, the selection indicator should have rounded corners if it's at the corner of the color picker.
Comment 1 Aditya Keerthi 2018-07-27 14:45:13 PDT
Created attachment 345957 [details]
Patch
Comment 2 Build Bot 2018-07-28 11:46:51 PDT
Comment on attachment 345957 [details]
Patch

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

New failing tests:
imported/blink/transitions/unprefixed-transform.html
Comment 3 Build Bot 2018-07-28 11:47:03 PDT
Created attachment 345996 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 4 Wenson Hsieh 2018-07-31 16:07:56 PDT
Comment on attachment 345957 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:271
> +    WKColorButton *colorButton = _selectedColorButton.get().get();

Nit - these two lines can just be `if ([_selectedColorButton superview] == matrixView)`
Comment 5 Aditya Keerthi 2018-07-31 16:48:48 PDT
Created attachment 346223 [details]
Patch
Comment 6 Wenson Hsieh 2018-07-31 17:15:26 PDT
Comment on attachment 346223 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:216
> +    _selectedColorButton = colorButton;

This looks like it'll get called many times, for every tick of the pan gesture recognizer. Is it possible to bail here if the selected color button hasn't actually changed, or do we always need to do this work?
Comment 7 Aditya Keerthi 2018-07-31 17:27:17 PDT
Created attachment 346234 [details]
Patch
Comment 8 Aditya Keerthi 2018-07-31 17:28:17 PDT
(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 346223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346223&action=review
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:216
> > +    _selectedColorButton = colorButton;
> 
> This looks like it'll get called many times, for every tick of the pan
> gesture recognizer. Is it possible to bail here if the selected color button
> hasn't actually changed, or do we always need to do this work?

We can bail early. Also makes sense to not redraw the selection if the same button is tapped twice.
Comment 9 Aditya Keerthi 2018-07-31 17:33:54 PDT
Comment on attachment 346234 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:216
> +    if (_selectedColorButton.get().get() == colorButton)

Wait. I can't put it here because it will break the behavior from colorMatrixViewDidLayoutSubviews.
Comment 10 Aditya Keerthi 2018-07-31 17:41:11 PDT
Created attachment 346236 [details]
Patch
Comment 11 Wenson Hsieh 2018-07-31 17:50:10 PDT
Comment on attachment 346236 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:225
> +        bool minXEqual = CGFAbs(CGRectGetMinX(frame) - CGRectGetMinX(colorPickerBounds)) < FLT_EPSILON;

Nit - we generally prefer using std::abs here, instead of the CG equivalent.
Comment 12 Aditya Keerthi 2018-07-31 20:26:13 PDT
Created attachment 346249 [details]
Patch
Comment 13 WebKit Commit Bot 2018-08-01 08:40:54 PDT
Comment on attachment 346249 [details]
Patch

Clearing flags on attachment: 346249

Committed r234458: <https://trac.webkit.org/changeset/234458>
Comment 14 WebKit Commit Bot 2018-08-01 08:40:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-08-01 20:26:11 PDT
<rdar://problem/42840274>