Bug 13282 - REGRESSION (NativePopUp): Rightmost character cut off in pop-up menu
Summary: REGRESSION (NativePopUp): Rightmost character cut off in pop-up menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-04-04 14:58 PDT by David Kilzer (:ddkilzer)
Modified: 2007-08-30 15:44 PDT (History)
2 users (show)

See Also:


Attachments
Test case (63 bytes, text/html)
2007-04-04 14:59 PDT, David Kilzer (:ddkilzer)
no flags Details
Test case v2 (52 bytes, text/html)
2007-04-04 18:50 PDT, David Kilzer (:ddkilzer)
no flags Details
Make RenderMenuList draw the text directly, disabling rounding if necessary (76.69 KB, patch)
2007-04-06 09:08 PDT, mitz
mitz: review-
Details | Formatted Diff | Diff
Patch showing the fake baseline calculation (8.73 KB, patch)
2007-04-07 15:14 PDT, mitz
no flags Details | Formatted Diff | Diff
Measure and render option text with run rounding (97.84 KB, patch)
2007-08-29 02:30 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2007-04-04 14:58:23 PDT
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).
Comment 1 David Kilzer (:ddkilzer) 2007-04-04 14:59:06 PDT
Created attachment 13952 [details]
Test case
Comment 2 David Kilzer (:ddkilzer) 2007-04-04 18:50:41 PDT
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
Comment 3 mitz 2007-04-05 00:38:48 PDT
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.
Comment 4 Dave Hyatt 2007-04-05 01:02:40 PDT
Should force it to be left-aligned all the time too. :)

Comment 5 mitz 2007-04-06 09:08:27 PDT
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 6 mitz 2007-04-06 09:22:01 PDT
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+.
Comment 7 Darin Adler 2007-04-06 09:34:15 PDT
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 8 mitz 2007-04-07 10:01:55 PDT
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).
Comment 9 mitz 2007-04-07 10:40:50 PDT
(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).
Comment 10 mitz 2007-04-07 15:11:28 PDT
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.
Comment 11 mitz 2007-04-07 15:14:10 PDT
Created attachment 13988 [details]
Patch showing the fake baseline calculation
Comment 12 mitz 2007-04-07 16:56:17 PDT
(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().
Comment 13 Darin Adler 2007-04-11 02:10:34 PDT
<rdar://problem/5126392>
Comment 14 mitz 2007-08-29 02:30:11 PDT
Created attachment 16151 [details]
Measure and render option text with run rounding

I think this could be the right thing to do for now.
Comment 15 mitz 2007-08-29 02:32:18 PDT
Created attachment 16152
Comment 16 Dave Hyatt 2007-08-29 15:18:41 PDT
Comment on attachment 16151 [details]
Measure and render option text with run rounding

r=me
Comment 17 Mark Rowe (bdash) 2007-08-30 15:44:53 PDT
Landed in r25322.