Skip to content

Commit

Permalink
[macOS] Fix rendering of <select> in vertical writing mode
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264457
rdar://118147979

Reviewed by Tim Nguyen.

`NSPopUpButton` is not height resizable, resulting in a squished appearance
for `<select>` in vertical writing mode. To fix, fall back to the
`MenuListButton` path and use custom rendering when using a vertical writing
mode. Update the painting code for menu list buttons to support vertical
writing mode.

* Source/WebCore/platform/graphics/mac/controls/MenuListButtonMac.mm:
(WebCore::MenuListButtonMac::draw):

Remove incorrect logic to draw a separator next to the arrows. Currently, and
dating back to at least Safari 13, the separators do not show up (even in
horizontal writing mode). Fixing the logic to get separators working does not
result in an improvement in appearance. As a result, rather than re-writing the
logic, simply remove it.

* Source/WebCore/rendering/RenderThemeMac.mm:
(WebCore::RenderThemeMac::isControlStyled const):

Apply our fallback for zoomed `<select>` to `<select>`s that have a
vertical writing mode. In both cases, the fallback is necessary as the
native control only renders at fixed sizes.

Canonical link: https://commits.webkit.org/270458@main
  • Loading branch information
pxlcoder committed Nov 9, 2023
1 parent 51e09bb commit 2efb1a7
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 29 deletions.
44 changes: 18 additions & 26 deletions Source/WebCore/platform/graphics/mac/controls/MenuListButtonMac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,12 @@ static void drawMenuListBackground(GraphicsContext& context, const FloatRect& re
static constexpr int arrowPaddingBefore = 6;
static constexpr int arrowPaddingAfter = 6;

bool isVerticalWritingMode = style.states.contains(ControlStyle::State::VerticalWritingMode);
auto logicalBounds = isVerticalWritingMode ? bounds.transposedRect() : bounds;

// Since we actually know the size of the control here, we restrict the font scale to make sure the arrows will fit vertically in the bounds
float fontScale = std::min(style.fontSize / baseFontSize, bounds.height() / (baseArrowHeight * 2 + baseSpaceBetweenArrows));
float centerY = bounds.y() + bounds.height() / 2.0f;
float centerY = logicalBounds.y() + logicalBounds.height() / 2.0f;
float arrowHeight = baseArrowHeight * fontScale;
float arrowWidth = baseArrowWidth * fontScale;
float spaceBetweenArrows = baseSpaceBetweenArrows * fontScale;
Expand All @@ -161,50 +164,39 @@ static void drawMenuListBackground(GraphicsContext& context, const FloatRect& re

float leftEdge;
if (isRightToLeft)
leftEdge = bounds.x() + arrowPaddingAfter * style.zoomFactor;
leftEdge = logicalBounds.x() + arrowPaddingAfter * style.zoomFactor;
else
leftEdge = bounds.maxX() - arrowPaddingAfter * style.zoomFactor - arrowWidth;
leftEdge = logicalBounds.maxX() - arrowPaddingAfter * style.zoomFactor - arrowWidth;

GraphicsContextStateSaver stateSaver(context);

context.setFillColor(style.textColor);
context.setStrokeStyle(StrokeStyle::NoStroke);

// Draw the top arrow
Vector<FloatPoint> arrow1 = {
Vector<FloatPoint> topArrow = {
{ leftEdge, centerY - spaceBetweenArrows / 2.0f },
{ leftEdge + arrowWidth, centerY - spaceBetweenArrows / 2.0f },
{ leftEdge + arrowWidth / 2.0f, centerY - spaceBetweenArrows / 2.0f - arrowHeight }
};
context.fillPath(Path(arrow1));

// Draw the bottom arrow
Vector<FloatPoint> arrow2 = {
Vector<FloatPoint> bottomArrow = {
{ leftEdge, centerY + spaceBetweenArrows / 2.0f },
{ leftEdge + arrowWidth, centerY + spaceBetweenArrows / 2.0f },
{ leftEdge + arrowWidth / 2.0f, centerY + spaceBetweenArrows / 2.0f + arrowHeight }
};
context.fillPath(Path(arrow2));

constexpr auto leftSeparatorColor = Color::black.colorWithAlphaByte(40);
constexpr auto rightSeparatorColor = Color::white.colorWithAlphaByte(40);
// Rotate the arrows for vertical writing mode since the popup appears to the side of the control instead of under it.
if (isVerticalWritingMode) {
auto transposePoint = [](const FloatPoint& point) {
return point.transposedPoint();
};

// FIXME: Should the separator thickness and space be scaled up by fontScale?
int separatorSpace = 2; // Deliberately ignores zoom since it looks nicer if it stays thin.
int leftEdgeOfSeparator;
if (isRightToLeft)
leftEdgeOfSeparator = static_cast<int>(roundf(leftEdge + arrowWidth + arrowPaddingBefore * style.zoomFactor));
else
leftEdgeOfSeparator = static_cast<int>(roundf(leftEdge - arrowPaddingBefore * style.zoomFactor));

// Draw the separator to the left of the arrows
context.setStrokeThickness(1); // Deliberately ignores zoom since it looks nicer if it stays thin.
context.setStrokeStyle(StrokeStyle::SolidStroke);
context.setStrokeColor(leftSeparatorColor);
context.drawLine(IntPoint(leftEdgeOfSeparator, bounds.y()), IntPoint(leftEdgeOfSeparator, bounds.maxY()));
topArrow = topArrow.map(transposePoint);
bottomArrow = bottomArrow.map(transposePoint);
}

context.setStrokeColor(rightSeparatorColor);
context.drawLine(IntPoint(leftEdgeOfSeparator + separatorSpace, bounds.y()), IntPoint(leftEdgeOfSeparator + separatorSpace, bounds.maxY()));
context.fillPath(Path(topArrow));
context.fillPath(Path(bottomArrow));
}

} // namespace WebCore
Expand Down
7 changes: 4 additions & 3 deletions Source/WebCore/rendering/RenderThemeMac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -764,11 +764,12 @@ static Color activeButtonTextColor()
if (appearance == StyleAppearance::TextField || appearance == StyleAppearance::TextArea || appearance == StyleAppearance::SearchField || appearance == StyleAppearance::Listbox)
return style.border() != userAgentStyle.border();

// FIXME: This is horrible, but there is not much else that can be done. Menu lists cannot draw properly when
// FIXME: This is horrible, but there is not much else that can be done. Menu lists cannot draw properly when
// scaled. They can't really draw properly when transformed either. We can't detect the transform case at style
// adjustment time so that will just have to stay broken. We can however detect that we're zooming. If zooming
// is in effect we treat it like the control is styled.
if (appearance == StyleAppearance::Menulist && style.effectiveZoom() != 1.0f)
// is in effect we treat it like the control is styled. Additionally, treat the control like it is styled when
// using a vertical writing mode, since the AppKit control is not height resizable.
if (appearance == StyleAppearance::Menulist && (style.effectiveZoom() != 1.0f || !style.isHorizontalWritingMode()))
return true;

return RenderTheme::isControlStyled(style, userAgentStyle);
Expand Down

0 comments on commit 2efb1a7

Please sign in to comment.