Bug 215575

Summary: Do not shrink radio buttons bellow its size
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+, ews-feeder: commit-queue-

Description Sergio Villar Senin 2020-08-17 10:13:20 PDT
Do not shrink radio buttons bellow its size
Comment 1 Sergio Villar Senin 2020-08-17 10:43:13 PDT
Created attachment 406726 [details]
Patch
Comment 2 Darin Adler 2020-08-17 13:54:53 PDT
Comment on attachment 406726 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:183
>          // Min-Width / Min-Height
>          LengthSize minControlSize = Theme::singleton().minimumControlSize(part, style.fontCascade(), { style.minWidth(), style.minHeight() }, style.effectiveZoom());
> -        if (minControlSize.width != style.minWidth())
> -            style.setMinWidth(WTFMove(minControlSize.width));
> -        if (minControlSize.height != style.minHeight())
> -            style.setMinHeight(WTFMove(minControlSize.height));
> +        if (part == ControlPart::RadioPart) {
> +            style.setMinWidth(Length(style.width()));
> +            style.setMinHeight(Length(style.height()));
> +        } else {
> +            if (minControlSize.width != style.minWidth())
> +                style.setMinWidth(WTFMove(minControlSize.width));
> +            if (minControlSize.height != style.minHeight())
> +                style.setMinHeight(WTFMove(minControlSize.height));
> +        }

Why are we doing this here instead of inside the minimumControlSize function?

I’m not saying this is wrong, but I want to understand how we made the call.
Comment 3 Sergio Villar Senin 2020-08-18 00:42:00 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 406726 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406726&action=review
> 
> > Source/WebCore/rendering/RenderTheme.cpp:183
> >          // Min-Width / Min-Height
> >          LengthSize minControlSize = Theme::singleton().minimumControlSize(part, style.fontCascade(), { style.minWidth(), style.minHeight() }, style.effectiveZoom());
> > -        if (minControlSize.width != style.minWidth())
> > -            style.setMinWidth(WTFMove(minControlSize.width));
> > -        if (minControlSize.height != style.minHeight())
> > -            style.setMinHeight(WTFMove(minControlSize.height));
> > +        if (part == ControlPart::RadioPart) {
> > +            style.setMinWidth(Length(style.width()));
> > +            style.setMinHeight(Length(style.height()));
> > +        } else {
> > +            if (minControlSize.width != style.minWidth())
> > +                style.setMinWidth(WTFMove(minControlSize.width));
> > +            if (minControlSize.height != style.minHeight())
> > +                style.setMinHeight(WTFMove(minControlSize.height));
> > +        }
> 
> Why are we doing this here instead of inside the minimumControlSize function?
> 
> I’m not saying this is wrong, but I want to understand how we made the call.

For two reasons:
1) by doing it here we don't need to reimplement it for all platforms
2) the minimumControlSize function only receives the style.minHeight/minWidth but we also need the style.height/width

None of them are indeed blockers to implement it in minimumControlSize (we could pass the style.width/height) which I agree would look much better, but reimplementing it for all the platforms didn't appeal much to me.
Comment 4 Darin Adler 2020-08-18 08:09:03 PDT
Comment on attachment 406726 [details]
Patch

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

>>> Source/WebCore/rendering/RenderTheme.cpp:183
>>> +        }
>> 
>> Why are we doing this here instead of inside the minimumControlSize function?
>> 
>> I’m not saying this is wrong, but I want to understand how we made the call.
> 
> For two reasons:
> 1) by doing it here we don't need to reimplement it for all platforms
> 2) the minimumControlSize function only receives the style.minHeight/minWidth but we also need the style.height/width
> 
> None of them are indeed blockers to implement it in minimumControlSize (we could pass the style.width/height) which I agree would look much better, but reimplementing it for all the platforms didn't appeal much to me.

The special case for RadioPart doesn’t feel consistent with the style of the rest of this function. In future (does not need to block this patch) think we should strive for a better factoring. But we should definitely *not* refactor so every platform has to do it separately!
Comment 5 Radar WebKit Bug Importer 2020-08-24 10:14:13 PDT
<rdar://problem/67685837>
Comment 6 Sergio Villar Senin 2021-03-10 04:26:52 PST
Created attachment 422817 [details]
Patch
Comment 7 Sergio Villar Senin 2021-03-10 04:28:04 PST
Darin, instead of landing the previous patch (which got r+) I finally decided to refactor a bit the code so that it feels consistent with the rest of the method. Let me know what you think
Comment 8 Darin Adler 2021-03-10 09:49:08 PST
Comment on attachment 422817 [details]
Patch

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

Code looks fine. Argument name not so good.

> Source/WebCore/platform/Theme.h:60
> +    LengthSize minimumControlSize(ControlPart, const FontCascade&, const LengthSize& zoomedSize, const LengthSize& nonShrinkableZoomedSize, float zoomFactor) const;

Not sure the name is quite right here. I don’t really understand what a "non-shrinkable size" is. Would it be larger, or smaller, than zoomed size? Is this a "preferred size" or a "default size" or something?
Comment 9 Sergio Villar Senin 2021-03-10 12:23:46 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 422817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422817&action=review
> 
> Code looks fine. Argument name not so good.
> 
> > Source/WebCore/platform/Theme.h:60
> > +    LengthSize minimumControlSize(ControlPart, const FontCascade&, const LengthSize& zoomedSize, const LengthSize& nonShrinkableZoomedSize, float zoomFactor) const;
> 
> Not sure the name is quite right here. I don’t really understand what a
> "non-shrinkable size" is. Would it be larger, or smaller, than zoomed size?
> Is this a "preferred size" or a "default size" or something?

It's more a minimum size.
Comment 10 Darin Adler 2021-03-10 12:46:27 PST
Comment on attachment 422817 [details]
Patch

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

>>> Source/WebCore/platform/Theme.h:60
>>> +    LengthSize minimumControlSize(ControlPart, const FontCascade&, const LengthSize& zoomedSize, const LengthSize& nonShrinkableZoomedSize, float zoomFactor) const;
>> 
>> Not sure the name is quite right here. I don’t really understand what a "non-shrinkable size" is. Would it be larger, or smaller, than zoomed size? Is this a "preferred size" or a "default size" or something?
> 
> It's more a minimum size.

Wait, that means we pass minWidth/Height() as the size and width/height() as the minimum size? Sounds backwards.
Comment 11 Sergio Villar Senin 2021-03-11 01:07:38 PST
(In reply to Darin Adler from comment #10)
> Comment on attachment 422817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422817&action=review
> 
> >>> Source/WebCore/platform/Theme.h:60
> >>> +    LengthSize minimumControlSize(ControlPart, const FontCascade&, const LengthSize& zoomedSize, const LengthSize& nonShrinkableZoomedSize, float zoomFactor) const;
> >> 
> >> Not sure the name is quite right here. I don’t really understand what a "non-shrinkable size" is. Would it be larger, or smaller, than zoomed size? Is this a "preferred size" or a "default size" or something?
> > 
> > It's more a minimum size.
> 
> Wait, that means we pass minWidth/Height() as the size and width/height() as
> the minimum size? Sounds backwards.

We pass the minWidth/Height as the size indeed which is correct for most of the cases. However for controls like radio buttons we'd like to ensure that they don't shrink to things like 0 (which is the default value for min-width/height). That's why we set they're preferred (width/height) size as the minimum instead of 0.
Comment 12 Darin Adler 2021-03-11 09:41:46 PST
My point is not so much that the code is wrong, but that it’s hard to understand.
Comment 13 Sergio Villar Senin 2021-03-11 10:47:33 PST
(In reply to Darin Adler from comment #12)
> My point is not so much that the code is wrong, but that it’s hard to
> understand.

If it's a naming issue I'm fine with whatever suggestion you can come up with.
Comment 14 Darin Adler 2021-03-11 10:51:41 PST
As the confused person, I am not sure that I am the one who can fix the name!

I don’t object to you landing the patch.

I have registered my confusion with this, but it’s a small local issue so if you can’t think of a way to make it better, let’s just drop the matter.
Comment 15 Sergio Villar Senin 2021-03-15 04:42:27 PDT
Committed r274411 (235278@main): <https://commits.webkit.org/235278@main>