1717556 - The arrows in some menus are cropped

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[tgn-ff]

Attachment: "View" on the left; "Bookmarks" on the right.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

1. Set your UI font to ubuntu regular at 10pt.
2. Launch Firefox and observe the arrows in "View" and "Bookmarks" menus.

Actual results:

The arrows in the "View" menu look normal. The arrows in the "Bookmarks" menu are cropped. It can, also, affect the arrows in bookmark folders, depending on their titles. The GTK theme doesn't seem to be important, but I only tried Breeze and Adwaita.

Changing the font, or its size, "fixes" the issue. (Of course, ubuntu-regular@10pt might be the default for some distributions).

Expected results:

mozregression result:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c6004cc4ec83d87129a3193a6fb49bbc5c4ba4c7&tochange=43c6edbfc3dac8013830c4737c051cfc00ae5968

Comment 1

[Jonathan Kew [:jfkthame]]

Odd... comparing the rendering in the two menus, it looks like there's a fractional-pixel difference in the vertical positioning of the arrow glyph relative to the device pixel grid; in the case of the Bookmarks menu version, this results in an asymmetrical rendering. I'll see if I can reproduce this on my machine and figure out what triggered it.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1711479

Comment 3

[Jonathan Kew [:jfkthame]]

I've tried setting the UI font to Ubuntu 10 on my Linux machine, but I'm not seeing this result. I suspect it's very sensitive to various font- and graphics-related settings. What font hinting/antialiasing settings do you have? What display scaling factor?

Comment 4

[tgn-ff]

I have the following settings:

• hinting: slight
• subpixel: rgb
• lcddefault filter
• scaling to 100% on a 1080p monitor
• font DPI to 96

Changing some settings (eg. DPI, hinting) fixes some menus and breaks other.

I tested this on default ubuntu 21.04 gnome using the "live usb", and I can reproduce it there as well. However, because they use the ubuntu font at 11pt, it's different menus that exhibit the issue. Other than that, they seem to have the same settings, except for the subpixel aa which is disabled.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I can see the cropped arrows on my Ubuntu 20.04 LTS with the Ubuntu Regular 10pt font.

Comment 6

[Jonathan Kew [:jfkthame]]

I'm not sure this is really a Layout: Text & Fonts bug in Gecko. AFAICS the submenu arrows are rendered by moz_gtk_menu_arrow_paint which calls in to GTK to draw them, so exactly what it then does is out of our control.

Investigating a bit, it seems that the problem arises in nsNativeThemeGTK::DrawWidgetBackground when the original rect of the widget is not exactly aligned with device pixels. We call gfxContext::UserToDevicePixelSnapped to snap the widget rect we're painting to integer pixel positions, but this may distort its shape. Specifically, in this case the element has width and height both 1ex, so the frame's rect should be square -- and indeed it is. But UserToDevicePixelSnapped may break this and result in a non-square gdk_rect being passed to DrawThemeWithCairo; this non-square rect is what ultimately causes a problem, because gtk_render_arrow expects to work with a square rendering area.

Specifically, with the Ubuntu 10 font, we end up with 1ex being fractionally under 7 device pixels, but after we've snapped the widget rect, we sometimes get a 7 x 7 pixel square, and sometimes 7 x 6 pixels, depending where the edges of the "ideal" square fall relative to the device pixel grid.

There are several places we could fix this:

(a) The most targeted fix: pass std::min(rect->width, rect->height) instead of just rect->width as the size parameter to gtk_render_arrow, to ensure it will render within the smaller dimension of the rect. The disadvantage of this option is that the size of the arrows may vary by 1px from one menu to another, depending on pixel-grid alignment.

(b) When DrawWidgetBackground calls UserToDevicePixelSnapped, check if the widget rect was originally square, and if so, force the snapped result to be square again. This should have no side-effects elsewhere, and gives us consistent menu arrows.

(c) Modify gfxContext::UserToDevicePixelSnapped so that it does not distort squares in this way, by prioritizing pixel-snapping of the rect's dimensions over snapping each side individually. Unfortunately, simply making this change breaks things for a bunch of other callers (lots of orange on https://treeherder.mozilla.org/jobs?repo=try&revision=bb4e283f7166dc70e0ed69c1170c83a52aed438d), but perhaps we could make it an optional mode, and maintain existing behavior for other call-sites.

Comment 7

[Jonathan Kew [:jfkthame]]

And the reason this "regressed" in bug 1711479 is that we now have more accurate font metrics. It used to be that on Linux, the ex unit derived from the current font would (inadvertently) be rounded to an integer number of device pixels, which had the result that the widget background rect here, although it might not be device-pixel-aligned, would be an integer number of pixels in size. Therefore, UserToDevicePixelSnapped would snap both left and right edges in the same direction, and both top and bottom in the same direction, and so squareness would be maintained.

Since bug 1711479 we have more precise metrics (reflected in tests that started passing there), but this means that we no longer necessarily have 1ex being an exact number of device pixels (which was never a well-founded expectation, just an accident of implementation). This exposes the fragility of the widget-rendering code here, as pixel-snapping the square may now "squash" it and result in the clipping we're seeing.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1717556 - Give gfxContext::UserToDevicePixelSnapped an option to prioritize the rect dimensions over snapping each individual edge, and use this for GTK widget painting. r=karlt

Comment 9

[Karl Tomlinson (:karlt)]

gtk_render_arrow() may have problems with non-square dimensions, but the intuitive remedy in such a function would be to consider the minimum dimension. That would however still render arrows of different sizes depending on position, which would presumably also affect other platforms.

I wonder why menus are not rendering arrows at native sizes?
I assume the intention is that they be rendered like native menus.

(In reply to Jonathan Kew (:jfkthame) from comment #7)

And the reason this "regressed" in bug 1711479 is that we now have more accurate font metrics. It used to be that on Linux, the ex unit derived from the current font would (inadvertently) be rounded to an integer number of device pixels,

Where precisely was that happening?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (:karlt) from comment #9)

gtk_render_arrow() may have problems with non-square dimensions, but the intuitive remedy in such a function would be to consider the minimum dimension. That would however still render arrows of different sizes depending on position, which would presumably also affect other platforms.

I wonder why menus are not rendering arrows at native sizes?
I assume the intention is that they be rendered like native menus.

I don't know the history here; it looks like the size has been specified as 1ex in themes/linux/global/menu.css since "forever".

(In reply to Jonathan Kew (:jfkthame) from comment #7)

And the reason this "regressed" in bug 1711479 is that we now have more accurate font metrics. It used to be that on Linux, the ex unit derived from the current font would (inadvertently) be rounded to an integer number of device pixels,

Where precisely was that happening?

gfx/thebes/gfxFT2FontBase.cpp used to set the xHeight metric by measuring the height of an 'x' glyph, which gave us a result that was snapped to device pixels by FreeType's hinting/rasterization. (The result may even have been dependent on the font hinting mode in effect, although from a layout point of view it would be rather unexpected for the system hinting mode to alter CSS units.)

We now prefer to use the "ideal" metric from the font's OS/2 table (because this gives more accurate scaling measurements for font-size-adjust, and means that ex scales smoothly with font-size rather than having unexpected "bumps"); we only fall back to measuring a specific glyph's extents if the OS/2 metric was missing.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)

I don't know the history here; it looks like the size has been specified as 1ex in themes/linux/global/menu.css since "forever".

Thanks for looking. FWIW the rendering may have changed at some point from simply centering the arrow to scaling. The minimum widget size could also be a factor depending on theme and point in history.

(The result may even have been dependent on the font hinting mode in effect, although from a layout point of view it would be rather unexpected for the system hinting mode to alter CSS units.)

Hinting does change the height of the characters though.

From a layout point of view it is often helpful to have units with integer device pixel counts. This was intentionally provided for metrics contributing to line height, for example, so that lines are equally spaced after snapping. I don't recall whether such behavior was intentionally provided for other units.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8612d5d0312
Give gfxContext::UserToDevicePixelSnapped an option to prioritize the rect dimensions over snapping each individual edge, and use this for GTK widget painting. r=karlt

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d8612d5d0312