| Summary: | [iOS] WKColorPicker's selection indicator doesn't always cover the selected swatch | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aditya Keerthi <pxlcoder> | ||||||||||||||
| Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | cdumez, commit-queue, ews-watchlist, thorton, wenson_hsieh | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | iPhone / iPad | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 188207 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Aditya Keerthi
2018-07-27 14:00:49 PDT
Created attachment 345957 [details]
Patch
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 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 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)` Created attachment 346223 [details]
Patch
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? Created attachment 346234 [details]
Patch
(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 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. Created attachment 346236 [details]
Patch
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. Created attachment 346249 [details]
Patch
Comment on attachment 346249 [details] Patch Clearing flags on attachment: 346249 Committed r234458: <https://trac.webkit.org/changeset/234458> All reviewed patches have been landed. Closing bug. |