Bug 193599 – [iOS] Focus ring for checkboxes, radio buttons, buttons and search fields should hug tighter to the contour

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/47399602>

[Daniel Bates]

Created attachment 362302 [details]
Patch

[Simon Fraser (smfr)]

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

[Daniel Bates]

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

[Daniel Bates]

Created attachment 362307 [details]
Patch

Now with a theme...

[Daniel Bates]

Created attachment 362311 [details]
Patch and mismatch ref tests

[Daniel Bates]

Created attachment 362312 [details]
Patch and mismatch ref tests

[zalan]

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

[Daniel Bates]

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?

[EWS Watchlist]

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

[EWS Watchlist]

Created attachment 362330 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362312 [details]
Patch and mismatch ref tests

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

New failing tests:
fast/forms/focus-submit-button.html
fast/forms/focus-reset-button.html
fast/forms/focus-button.html

[EWS Watchlist]

Created attachment 362333 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Simon Fraser (smfr)]

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.

[Daniel Bates]

Created attachment 362344 [details]
Patch and mismatched refs

[Daniel Bates]

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

[Daniel Bates]

Created attachment 362347 [details]
To land

[Daniel Bates]

Committed r241747: <https://trac.webkit.org/changeset/241747>

[Daniel Bates]

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