Bug 215575 – Do not shrink radio buttons bellow its size

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Do not shrink radio buttons bellow its size

[Sergio Villar Senin]

Created attachment 406726 [details]
Patch

[Darin Adler]

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.

[Sergio Villar Senin]

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

[Darin Adler]

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!

[Radar WebKit Bug Importer]

<rdar://problem/67685837>

[Sergio Villar Senin]

Created attachment 422817 [details]
Patch

[Sergio Villar Senin]

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

[Darin Adler]

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?

[Sergio Villar Senin]

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

[Darin Adler]

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.

[Sergio Villar Senin]

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

[Darin Adler]

My point is not so much that the code is wrong, but that it’s hard to understand.

[Sergio Villar Senin]

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

[Darin Adler]

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.

[Sergio Villar Senin]

Committed r274411 (235278@main): <https://commits.webkit.org/235278@main>