REGRESSION (NativePopUp): Rightmost character cut off in pop-up menu

• Component: Forms
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Summary:

Native pop-up menu has the right-most character cut off.

Steps to reproduce:

1. Open Safari/WebKit.
2. Open test case.

Expected results:

The right-most character should not be cut off.

Actual results:

The right-most character is cut off.

Regression:

This is a regression from shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

Notes:

Tested with a local debug build of WebKit r20699 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

[David Kilzer (:ddkilzer)]

Created attachment 13952 [details]
Test case

[David Kilzer (:ddkilzer)]

Created attachment 13955 [details]
Test case v2

It appears that the proper width is not being calculated for dashes!

This test case should have seven dashes then the number eight:  -------8

There's an inconsistency between how option text is measured and how it's drawn:

void RenderMenuList::updateOptionsWidth()
{
    // FIXME: There is no longer any reason to use a text style with the font hacks disabled.
    // It is a leftover from when the text was not drawn with the engine -- now that we render
    // with the engine, we can just use the default style as the engine does.
    TextStyle textStyle(0, 0, 0, false, false, false, false);

Using a plain TextStyle instead fixes the bug but has the side effect of not matching the popup menu's width, since AppKit obviously doesn't perform the rounding hack. So that just moves the inconsistency to a different place. A better solution would be to disable the rounding hack also when drawing the button text.

[Dave Hyatt]

Should force it to be left-aligned all the time too. :)

Created attachment 13980 [details]
Make RenderMenuList draw the text directly, disabling rounding if necessary

This patch removes most of the RenderButton-derived code from RenderMenuList, instead making it simply paint its own text. This gives it control over text rounding, allowing it to match the platform's popup implementation.

I am not sure that a PopupMenu static will be sufficient for deciding what to do (it's hard to tell currently). Maybe that part should be put off until it's needed.

Comment on attachment 13980 [details]
Make RenderMenuList draw the text directly, disabling rounding if necessary

I have the binaries to go with this patch but I'm having trouble uploading them. I'll figure it out if I get a r+.

[Darin Adler]

Comment on attachment 13980 [details]
Make RenderMenuList draw the text directly, disabling rounding if necessary

This patch looks great. I'd like Adele and maybe Hyatt to review too. I don't think it's too helpful to have a setText function if it's private and is just an inline that sets m_text, so I probably would have removed it.

Comment on attachment 13980 [details]
Make RenderMenuList draw the text directly, disabling rounding if necessary

I got the vertical positioning wrong in some cases (e.g. the Web Inspector's popup; the text there also looks heavier -- I'll investigate).

(In reply to comment #8)
> (From update of attachment 13980 [details] [edit])
> I got the vertical positioning wrong in some cases

...and the baseline

> the text there also looks heavier

That's because of text-shadow (text-stroke is also missing).

I think I managed to get the baseline calculation right (down to the accumulated rounding errors in the current implementation), but then I found myself copying in big chunks of InlineTextBox code in attempt to support all text effects and decorations, which didn't seem like the right thing to do. Now I'm not sure about the whole approach.

Created attachment 13988 [details]
Patch showing the fake baseline calculation

(In reply to comment #10)
> Now I'm not sure about the whole approach.

Even without rounding hacks, you're not going to match AppKit due to kerning and ligatures. Might be best to just patch updateOptionsWidth().

[Darin Adler]

<rdar://problem/5126392>

Created attachment 16151 [details]
Measure and render option text with run rounding

I think this could be the right thing to do for now.

Created attachment 16152

[Dave Hyatt]

Comment on attachment 16151 [details]
Measure and render option text with run rounding

r=me

[Mark Rowe (bdash)]

Landed in r25322.