Bug 188124 – [iOS] WKColorPicker's selection indicator doesn't always cover the selected swatch

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

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.

[Aditya Keerthi]

Created attachment 345957 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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)`

[Aditya Keerthi]

Created attachment 346223 [details]
Patch

[Wenson Hsieh]

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?

[Aditya Keerthi]

Created attachment 346234 [details]
Patch

[Aditya Keerthi]

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

[Aditya Keerthi]

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.

[Aditya Keerthi]

Created attachment 346236 [details]
Patch

[Wenson Hsieh]

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.

[Aditya Keerthi]

Created attachment 346249 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346249 [details]
Patch

Clearing flags on attachment: 346249

Committed r234458: <https://trac.webkit.org/changeset/234458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42840274>