Bug 215428 – [macOS] Zoomed-in search field is clipped out

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

The top and bottom borders, as well as the top and bottom of the focus ring, are clipped when a search field is zoomed in.

[Aditya Keerthi]

<rdar://problem/66161781>

[Aditya Keerthi]

Created attachment 406476 [details]
Patch

[Aditya Keerthi]

Created attachment 406488 [details]
Patch

[Darin Adler]

Comment on attachment 406488 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:1999
> +    NSControlSize controlSize = controlSizeForFont(style);
> +    setFontFromControlSize(style, controlSize);

I know you just moved this, but I suggest doing this in one line without a local variable.

> LayoutTests/ChangeLog:15
> +        * fast/forms/search/search-zoom-rendering-expected.txt: Added.
> +        * fast/forms/search/search-zoom-rendering.html: Added.
> +        * platform/ios-wk2/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> +        * platform/mac-mojave/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> +        * platform/win/fast/forms/search/search-zoom-rendering-expected.txt: Added.

It’s really undesirable to have another render-tree-dumping test like this. Is there any way we can test this without a render tree dump?

One thing to think about is that the render tree dump doesn’t actually check the clipping, only the geometry. Since that’s all we need to check, maybe we can do a reftest that explicitly sets the size? Or perhaps we can do a dumpAsText test that explicitly queries the height?

We can land like this, but these are really hard to maintain over time. I’m particularly puzzled by the seemingly random set of expectation files. For example, why does legacy WebKit testing on iOS use the shared master expectation, but modern WebKit testing on iOS use its own. Maybe that file should be in platform/ios instead of in platform/ios-wk2?

[Aditya Keerthi]

Created attachment 406494 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Darin Adler from comment #4)
> Comment on attachment 406488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406488&action=review
> 
> > Source/WebCore/rendering/RenderThemeMac.mm:1999
> > +    NSControlSize controlSize = controlSizeForFont(style);
> > +    setFontFromControlSize(style, controlSize);
> 
> I know you just moved this, but I suggest doing this in one line without a
> local variable.

👍

> > LayoutTests/ChangeLog:15
> > +        * fast/forms/search/search-zoom-rendering-expected.txt: Added.
> > +        * fast/forms/search/search-zoom-rendering.html: Added.
> > +        * platform/ios-wk2/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> > +        * platform/mac-mojave/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> > +        * platform/win/fast/forms/search/search-zoom-rendering-expected.txt: Added.
> 
> It’s really undesirable to have another render-tree-dumping test like this.
> Is there any way we can test this without a render tree dump?
> 
> One thing to think about is that the render tree dump doesn’t actually check
> the clipping, only the geometry. Since that’s all we need to check, maybe we
> can do a reftest that explicitly sets the size? Or perhaps we can do a
> dumpAsText test that explicitly queries the height?
> 
> We can land like this, but these are really hard to maintain over time. I’m
> particularly puzzled by the seemingly random set of expectation files. For
> example, why does legacy WebKit testing on iOS use the shared master
> expectation, but modern WebKit testing on iOS use its own. Maybe that file
> should be in platform/ios instead of in platform/ios-wk2?

I was unable to write a reftest that explicitly sets the size, since height is overridden when rendering our native search control (same behavior as NSSearchFieldCell). However, I did replace the render-tree-dumping test with a dumpAsText test.

I also moved the expectation from platform/ios-wk2 into platform/ios.

[Aditya Keerthi]

Created attachment 406514 [details]
Patch for landing

[EWS]

Committed r265613: <https://trac.webkit.org/changeset/265613>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406514 [details].

[Aditya Keerthi]

Reopening to attach patch for test fix.

[Aditya Keerthi]

Created attachment 406542 [details]
Rebaseline test

[EWS]

Committed r265625: <https://trac.webkit.org/changeset/265625>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406542 [details].