Bug 69179 – Tops of some <select> elements are clipped with a deviceScaleFactor > 1

• Component: Forms
• Status: RESOLVED CONFIGURATION CHANGED

[Beth Dakin]

Visit  data:text/html,%3Cselect%20style=%22font-size:16px%22%3E%3Coption%3Efoo with a deviceScaleFactor > 1. The top of the <select> element is cut off, and the element appears to be shifted up by 1 pixel.

<rdar://problem/10059823>

[Beth Dakin]

Created attachment 109362 [details]
Patch

[Darin Adler]

Comment on attachment 109362 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:786
> +        inflatedRect = inflateRect(inflatedRect, size, popupButtonMargins(), zoomLevel * WebCore::deviceScaleFactor(o->frame()));

Do you really need the WebCore:: prefix?

[Beth Dakin]

Thanks for reviewing, but I am starting to think that this patch is wrong…I'm going to think it over a bit more first…

[Beth Dakin]

Created attachment 109697 [details]
Revised patch

Here's a patch that I feel better about. And here's the deal with why I wanted to pursue a different solution than the one in the patch that Darin already reviewed. The old patch did the same thing for deviceScaleFactor that we currently do for effectiveZoom (which is the CSS zoom, not Page::pageScaleFactor() or Frame::page/textZoomFactor()). However that's not quite right because that code path actually changes the layout size of the button and since deviceScaleFactor happens at the GraphicsContext level, it meant that we were getting slightly-too-big selects with the old patch because we increased the layout size a bit, and then the scale on the GraphicsContext scaled the slightly-bigger select. Due to the lower-level oddities that I allude to in the ChangeLog, making a bigger select is the only way that I was able to fix this bug in WebCore. In the end, I think this is a bug in a different component, and we can work around it by not clipping. This seems better to me than making a bigger select because it's more pixel-perfect in the end.

[Adam Roben (:aroben)]

Comment on attachment 109697 [details]
Revised patch

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

> Source/WebCore/ChangeLog:12
> +        inflatedRect. Since the existing comment indicates that that code was Leopard-
> +        specific anyway, this patch makes it Leopard-only.

The code was added in <http://trac.webkit.org/r24604> inside an #ifndef BUILDING_ON_TIGER block. Since that predates the release of SnowLeopard, presumably the code was meant to be for Leopard and newer. So are we sure that disabling this code doesn't introduce a bug on SnowLeopard?

[Beth Dakin]

(In reply to comment #5)
> (From update of attachment 109697 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109697&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        inflatedRect. Since the existing comment indicates that that code was Leopard-
> > +        specific anyway, this patch makes it Leopard-only.
> 
> The code was added in <http://trac.webkit.org/r24604> inside an #ifndef BUILDING_ON_TIGER block. Since that predates the release of SnowLeopard, presumably the code was meant to be for Leopard and newer. So are we sure that disabling this code doesn't introduce a bug on SnowLeopard?

I have run and analyzed all of the tests and pixel tests, and I haven't found any new bugs. That being said,  the fact that this bug goes away when you don't clip is probably an example of the potential dangers foretold by the comment: "On Leopard, the cell will draw outside of the given rect, so we have to clip to the rect." However, I have yet to find any examples of the cell drawing outside the given rect that result in anything but the cell drawing a non-cut-off select.

[Darin Adler]

Comment on attachment 109697 [details]
Revised patch

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

>>> Source/WebCore/ChangeLog:12
>>> +        specific anyway, this patch makes it Leopard-only.
>> 
>> The code was added in <http://trac.webkit.org/r24604> inside an #ifndef BUILDING_ON_TIGER block. Since that predates the release of SnowLeopard, presumably the code was meant to be for Leopard and newer. So are we sure that disabling this code doesn't introduce a bug on SnowLeopard?
> 
> I have run and analyzed all of the tests and pixel tests, and I haven't found any new bugs. That being said,  the fact that this bug goes away when you don't clip is probably an example of the potential dangers foretold by the comment: "On Leopard, the cell will draw outside of the given rect, so we have to clip to the rect." However, I have yet to find any examples of the cell drawing outside the given rect that result in anything but the cell drawing a non-cut-off select.

In theory, the problem with drawing outside the rectangle is due to repainting. If the part of the page outside the rectangle that was painted over by the menu list needs to repaint later, the other content will be repainted but the menu list won't because it’s geometry information is incorrect.

Generally speaking, that’s why it’s a problem to have code that can draw and touch pixels outside the expected. To make it OK to do that, the code has to somehow understand how many additional pixels will be drawn and take that into account when deciding what to repaint.

So you could construct a test case with, say, a single pixel thick object that appeared and disappeared right next to the select and see what happens.

[Darin Adler]

Comment on attachment 109697 [details]
Revised patch

This patch is definitely wrong. The word “Leopard” in that comment means Leopard and newer, so there is no good argument for special casing Leopard. If we do need to allow drawing outside the rectangle then we almost certainly need to do more work, not just remove the clip.

[Darin Adler]

Another thing to test for is to dynamically remove the select element and see if pixels are left behind.

[Adam Barth]

Comment on attachment 109362 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 109362 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ahmad Saleem]

I changed the test case from Comment 0 in below:

https://jsfiddle.net/tgbch95p/

and then tried to zoom in and then use magnifier functionality to see any top displacement or being cut-off but I am not able to see cut off top but just that the inside is not filled on all corners to touch "black" border. It was tested on macOS 12.6 with Safari Technology Preview 153.

Appreciate if someone confirm whether it was original bug or not. Thanks!

[Aditya Keerthi]

(In reply to Ahmad Saleem from comment #11)
> I changed the test case from Comment 0 in below:
> 
> https://jsfiddle.net/tgbch95p/
> 
> and then tried to zoom in and then use magnifier functionality to see any
> top displacement or being cut-off but I am not able to see cut off top but
> just that the inside is not filled on all corners to touch "black" border.
> It was tested on macOS 12.6 with Safari Technology Preview 153.
> 
> Appreciate if someone confirm whether it was original bug or not. Thanks!

This bug was about display resolution – modern Macs have a 2x display, so it would be very apparent if this bug was still reproducing today.

The bug was actually fixed in another component, over a decade ago :)