Summary: | Do not shrink radio buttons bellow its size | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||
Component: | New Bugs | Assignee: | 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
Sergio Villar Senin
2020-08-17 10:13:20 PDT
Created attachment 406726 [details]
Patch
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. (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 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! Created attachment 422817 [details]
Patch
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 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? (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 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. (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. My point is not so much that the code is wrong, but that it’s hard to understand. (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. 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. Committed r274411 (235278@main): <https://commits.webkit.org/235278@main> |