[webkit-changes] [WebKit/WebKit] 3d8bc4: REGRESSION: File input button layout looks bad at ...

[Aditya Keerthi]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 3d8bc4adc4366c9b57b2aa50e2c62c3056b9c2a5
      https://github.com/WebKit/WebKit/commit/3d8bc4adc4366c9b57b2aa50e2c62c3056b9c2a5
  Author: Aditya Keerthi <akeerthi at apple.com>
  Date:   2023-01-25 (Wed, 25 Jan 2023)

  Changed paths:
    M LayoutTests/accessibility/node-only-object-element-rect-expected.txt
    M LayoutTests/platform/mac/compositing/contents-opaque/control-layer-expected.txt
    M Source/WebCore/platform/graphics/mac/controls/ButtonMac.mm
    M Source/WebCore/platform/mac/ThemeMac.mm

  Log Message:
  -----------
  REGRESSION: File input button layout looks bad at various zoom levels
https://bugs.webkit.org/show_bug.cgi?id=251065
rdar://104197684

Reviewed by Tim Nguyen.

On macOS, at different zoom levels, the “Choose File” text in the file input
button is poorly aligned relative to the button outline.

This bug is the consequence of three distinct changes, each of which are
described below. However, in order to understand the individual causes, a
baseline understanding of form control rendering on macOS must be established.

Form controls on macOS are painted by drawing `NSCell`s vended from AppKit.
This approach is essential in ensuring that WebKit form controls have a native
look and feel. A major limitation of AppKit controls is that they are almost all
backed by fixed-size bitmaps, whereas form controls on the web have no inherent
sizing limitations.

AppKit defines four fixed sizes, `NSControlSizeRegular`, `NSControlSizeSmall`,
`NSControlSizeMini`, and `NSControlSizeLarge`. WebKit takes various approaches
to reconcile fixed AppKit control sizes with arbitrary element sizes. In all
cases, the `NSControlSize` corresponding to the fixed size closest to the
element's box size (adjusted for zoom) is determined. Buttons have historically
done one of two things with a given `NSControlSize`:

1. Adjust (force) the element's size to match the fixed size.
2. Paint the `NSCell` in the center of the element's box.

Until 258754 at main, `<input type=button>` (and file input buttons) used the first
approach, while `<button>` used the second. 258754 at main unified
`<input type=button>` and `<button`> styling/painting, using the second approach,
to resolve numerous web compatibility issues. In doing so, all bugs with `<button>`
painting were also brought to file input buttons. While 258754 at main regressed
file input buttons, it is not the root cause of the issue.

The root cause of the issue revolves around how the bitmap image vended from
AppKit is centered in the element's box. Specifically, the frame (rect) used
to draw an `NSCell` is not equivalent to the size of the bitmap. The actual
image is inset from the frame. Consequently, in order to paint the button at a
particular size/position, the frame passed to AppKit must be inflated. WebKit
currently achieves this by using a hardcoded list of cell sizes and outsets.
However, these values are long out-of-date, as macOS theme definitions have
evolved.

The result of incorrect outsets is that the rendered position of the button
outline does not align with the text. Some outset values (for a given
`NSControlSize`) are more accurate than others. When the zoom level changes the
used `NSControlSize` can also change, resulting in an effect where it appears
as though the text moves relative to the button. To fix this issue, WebKit needs
to use correct values for the outsets, so that the button outline is drawn at
the intended position.

However, fixing the outsets revealed yet another regression in the logic
responsible for determining the "inflated rect" `NSButtonCell`s. 258492 at main
moved button painting code from `ThemeMac` into `ButtonMac`, but made two
errors:

1. The use of `auto` to store the cell size prior to scaling by the zoom factor.
   Since cell sizes are integer values, the scaled height ends up as an integer,
   which throws off the position of the renderered bitmap. Before 258492 at main,
   the logic explicitly used `FloatSize`.

2. The use of `expand` to inflate the rect. `expand` does not adjust the position
   of the rect – it merely increases the size. Consequently, the position of the
   rect is not adjusted to center it. Before 258492 at main, the logic used `setY`
   and `setHeight`.

In summary, AppKit changes, 258492 at main, and 258754 at main all combined to result
in file input button layout looking bad at various zoom levels.

* LayoutTests/accessibility/node-only-object-element-rect-expected.txt: Rebaseline.
* LayoutTests/platform/mac/compositing/contents-opaque/control-layer-expected.txt: Rebaseline.
* Source/WebCore/platform/graphics/mac/controls/ButtonMac.mm:
(WebCore::ButtonMac::cellSize const):

Use updated values from AppKit.

(WebCore::ButtonMac::cellOutsets const):

Use updated values from AppKit. These should be determined programmatically
once the `ControlPart` refactor is complete, and an additional refactor to use
`NSControl`s (that own `NSCell`s) can be made.

(WebCore::ButtonMac::rectForBounds const):

Fix the two logic errors described above. Explicitly use `FloatSize` to avoid
clamping to integers and use `inflateY` to ensure the position of the inflated
rect is correct.

* Source/WebCore/platform/mac/ThemeMac.mm:

Use updated values from AppKit. This code duplication will eventually be removed,
once the ongoing `ControlPart` refactor is complete, but for now, the values need
to be updated to adjust the repaint rect.

(WebCore::buttonSizes):
(WebCore::buttonMargins):

Canonical link: https://commits.webkit.org/259359@main