Bug 193599 - [iOS] Focus ring for checkboxes, radio buttons, buttons and search fields should hug tighter to the contour
Summary: [iOS] Focus ring for checkboxes, radio buttons, buttons and search fields sho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: All iOS 12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2019-01-18 15:49 PST by Daniel Bates
Modified: 2019-02-28 09:46 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.05 KB, patch)
2019-02-18 10:49 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2019-02-18 11:21 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and mismatch ref tests (22.85 KB, patch)
2019-02-18 12:12 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and mismatch ref tests (22.61 KB, patch)
2019-02-18 12:14 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (18.34 MB, application/zip)
2019-02-18 14:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (12.83 MB, application/zip)
2019-02-18 14:26 PST, Build Bot
no flags Details
Patch and mismatched refs (21.58 KB, patch)
2019-02-18 15:04 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (21.76 KB, patch)
2019-02-18 15:20 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-01-18 15:49:52 PST
The focus rings for checkboxes, radio buttons, and maybe even search fields seem loose fitting. We should make them more tightly draw around the contour of these controls.
Comment 1 Radar WebKit Bug Importer 2019-01-18 15:50:28 PST
<rdar://problem/47399602>
Comment 2 Daniel Bates 2019-02-18 10:49:55 PST
Created attachment 362302 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-02-18 11:01:50 PST
Comment on attachment 362302 [details]
Patch

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

Can we make a ref test for this stuff?

> Source/WebCore/rendering/RenderElement.cpp:1908
> +        LayoutRect paintRectToUse { paintRect };
> +#if PLATFORM(IOS_FAMILY)
> +        // Workaround for <rdar://problem/6209763>. Force the painting bounds of checkboxes and radio controls to be square.
> +        if (style().appearance() == CheckboxPart || style().appearance() == RadioPart) {
> +            int width = std::min(paintRect.width(), paintRect.height());
> +            int height = width;
> +            paintRectToUse = IntRect { paintRect.x(), paintRect.y() + (downcast<RenderBox>(*this).height() - height) / 2, width, height }; // Vertically center the checkbox, like on desktop
> +        }
> +#endif
> +        addFocusRingRects(focusRingRects, paintRectToUse.location(), paintInfo.paintContainer);

This really needs to move into RenderTheme code (I guess that's bug 194781).
Comment 4 Daniel Bates 2019-02-18 11:03:31 PST
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 362302 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362302&action=review
> 
> Can we make a ref test for this stuff?
> 
> > Source/WebCore/rendering/RenderElement.cpp:1908
> > +        LayoutRect paintRectToUse { paintRect };
> > +#if PLATFORM(IOS_FAMILY)
> > +        // Workaround for <rdar://problem/6209763>. Force the painting bounds of checkboxes and radio controls to be square.
> > +        if (style().appearance() == CheckboxPart || style().appearance() == RadioPart) {
> > +            int width = std::min(paintRect.width(), paintRect.height());
> > +            int height = width;
> > +            paintRectToUse = IntRect { paintRect.x(), paintRect.y() + (downcast<RenderBox>(*this).height() - height) / 2, width, height }; // Vertically center the checkbox, like on desktop
> > +        }
> > +#endif
> > +        addFocusRingRects(focusRingRects, paintRectToUse.location(), paintInfo.paintContainer);
> 
> This really needs to move into RenderTheme code (I guess that's bug 194781).

Yeah, I debated moving it now. Should have seen my local checkbox. I did do it. I could do still. Kinda wanted to fix in bug #194781. What do you say?
Comment 5 Daniel Bates 2019-02-18 11:21:55 PST
Created attachment 362307 [details]
Patch

Now with a theme...
Comment 6 Daniel Bates 2019-02-18 12:12:29 PST
Created attachment 362311 [details]
Patch and mismatch ref tests
Comment 7 Daniel Bates 2019-02-18 12:14:00 PST
Created attachment 362312 [details]
Patch and mismatch ref tests
Comment 8 zalan 2019-02-18 12:31:12 PST
Comment on attachment 362312 [details]
Patch and mismatch ref tests

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

> Source/WebCore/rendering/RenderTheme.h:125
> +    virtual void adjustPaintRect(const RenderObject&, LayoutRect&) const { }

Could you tighten the type to RenderElement, please?

> Source/WebCore/rendering/RenderThemeIOS.mm:383
> +    int width = std::min(paintRect.width(), paintRect.height());

int? why?

> Source/WebCore/rendering/RenderThemeIOS.mm:385
> +    paintRect = IntRect { paintRect.x(), paintRect.y() + (downcast<RenderBox>(renderer).height() - height) / 2, width, height }; // Vertically center the checkbox, like on desktop.

Is the RenderBox type guaranteed here? (I mean at this point I know it is through the appearance check, but downcasting like this is not super future-proof).
Comment 9 Daniel Bates 2019-02-18 12:43:55 PST
Comment on attachment 362312 [details]
Patch and mismatch ref tests

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

>> Source/WebCore/rendering/RenderTheme.h:125
>> +    virtual void adjustPaintRect(const RenderObject&, LayoutRect&) const { }
> 
> Could you tighten the type to RenderElement, please?

Cannot due to call site restrictions. Anything is possible but have to do type checking and casting at call sites.

>> Source/WebCore/rendering/RenderThemeIOS.mm:383
>> +    int width = std::min(paintRect.width(), paintRect.height());
> 
> int? why?

Because that’s how the code was written. I’ll change it to anything you want so long as you guarantee identical results. What do want? LayoutUnit to hide the int?

>> Source/WebCore/rendering/RenderThemeIOS.mm:385
>> +    paintRect = IntRect { paintRect.x(), paintRect.y() + (downcast<RenderBox>(renderer).height() - height) / 2, width, height }; // Vertically center the checkbox, like on desktop.
> 
> Is the RenderBox type guaranteed here? (I mean at this point I know it is through the appearance check, but downcasting like this is not super future-proof).

A non-box checkbox or radio seems weird. What do you want me to do?
Comment 10 Build Bot 2019-02-18 14:03:30 PST
Comment on attachment 362312 [details]
Patch and mismatch ref tests

Attachment 362312 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11195244

New failing tests:
fast/forms/focus-checked-radio.html
fast/forms/focus-text-field.html
fast/forms/focus-search-field.html
fast/forms/focus-textarea.html
fast/forms/focus-reset-button.html
fast/forms/focus-checked-checkbox.html
fast/forms/focus-checkbox.html
fast/forms/focus-button.html
fast/forms/focus-submit-button.html
fast/forms/focus-radio.html
Comment 11 Build Bot 2019-02-18 14:03:33 PST Comment hidden (obsolete)
Comment 12 Build Bot 2019-02-18 14:25:55 PST Comment hidden (obsolete)
Comment 13 Build Bot 2019-02-18 14:26:08 PST Comment hidden (obsolete)
Comment 14 Simon Fraser (smfr) 2019-02-18 14:35:34 PST
Comment on attachment 362312 [details]
Patch and mismatch ref tests

Alan would prefer to go back to your original patch, with the test cases. We'll figure out the right way to use RenderTheme in future.
Comment 15 Daniel Bates 2019-02-18 15:04:47 PST
Created attachment 362344 [details]
Patch and mismatched refs
Comment 16 Daniel Bates 2019-02-18 15:07:02 PST
(In reply to Build Bot from comment #10)
> Comment on attachment 362312 [details]
> Patch and mismatch ref tests
> 
> Attachment 362312 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11195244
> 
> New failing tests:
> fast/forms/focus-checked-radio.html
> fast/forms/focus-text-field.html
> fast/forms/focus-search-field.html
> fast/forms/focus-textarea.html
> fast/forms/focus-reset-button.html
> fast/forms/focus-checked-checkbox.html
> fast/forms/focus-checkbox.html
> fast/forms/focus-button.html
> fast/forms/focus-submit-button.html
> fast/forms/focus-radio.html

Forgot these tests depend on UIKit changes to support testing tab support AND a build with ENABLE(FULL_KEYBOARD_ACCESS) enabled. We have neither in OpenSource. So, will skip for now.
Comment 17 Daniel Bates 2019-02-18 15:20:44 PST
Created attachment 362347 [details]
To land
Comment 18 Daniel Bates 2019-02-18 16:10:42 PST
Committed r241747: <https://trac.webkit.org/changeset/241747>
Comment 19 Daniel Bates 2019-02-28 09:46:37 PST
Fixed up path to ui-helper.js so that we actually load it and avoid timing out. Committed the fix in <https://trac.webkit.org/changeset/242202>.