Bug 24434 - WebKit/win fails LayoutTests/fast/forms/textarea-scrollbar-height.html
Summary: WebKit/win fails LayoutTests/fast/forms/textarea-scrollbar-height.html
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-06 14:18 PST by Peter Kasting
Modified: 2010-06-10 20:47 PDT (History)
5 users (show)

See Also:


Attachments
Screenshots (222.40 KB, image/png)
2009-09-28 13:54 PDT, Victor Wang
no flags Details
Proposed patch - round line height (335.46 KB, patch)
2009-10-01 15:25 PDT, Victor Wang
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-03-06 14:18:56 PST
LayoutTests/fast/forms/textarea-scrollbar-height.html does not pass on WebKit for Windows.  It passed in Safari 3, presumably due to Mac-style font metrics/rendering.

This test also fails in Chromium.

Real-world page that's affected: http://de.news.yahoo.com/wirtschaft.html , see text in right navigation strip saying "Wirtschaft: Alle Videos »" that has an unnecessary scrollbar.
Comment 1 Victor Wang 2009-09-28 13:54:11 PDT
Created attachment 40255 [details]
Screenshots
Comment 2 Victor Wang 2009-09-28 13:55:32 PDT
In this case, the CSS line height is 15 (13*1.22) and the text font height is 16 on windows (13 + 3 internal leading for accent marks etc), so it is actually overflowed by 1 pixel. This is why Webkit for windows displays a scrollbar. The same thing could happen to Webkit on MAC if the line height number <= 1.15 (The text height on mac for this case is 15). Repro on safari if line height changes to <= 1.15.

Here are two places where Webkit is different from IE and Firefox:

1. The CSS line height calculation:
IE and firefox round the line height number while webkit truncates the fractional digits, so in this case (13*1.22 = 15.86), IE and Firefox return 16 and webkit returns 15.

2. Scroll bar display (See attached image)
-. IE does not display vertical scroll bar if the text item overflows;
-. Firfox does not display scroll bar for most cases unless the total number of lines > certain threshold. The scroll bar does not seem help much when it is displayed (see the last screenshot in attached image).
-. Webkit displays a scroll bar with > 0 pixel overflow.

In this case, the scroll bar does not seem very helpful and the IE behavior looks better to me, should we remove the scroll bar for list item?

Any comments?
Comment 3 Dave Hyatt 2009-09-28 14:28:58 PDT
Rounding non-integer line height values sounds reasonable to me.
Comment 4 Victor Wang 2009-10-01 15:25:26 PDT
Created attachment 40480 [details]
Proposed patch - round line height
Comment 5 Dave Hyatt 2009-10-08 10:44:59 PDT
Comment on attachment 40480 [details]
Proposed patch - round line height

r=me
Comment 6 Victor Wang 2009-10-14 11:03:30 PDT
Patch that rounds non-integer line height was landed. It fixes this layout test.
another issue is whether or not to display vertical bar when list item overflow a few pixels. Another bug was filed for it: https://bugs.webkit.org/show_bug.cgi?id=30361
Comment 7 David Kilzer (:ddkilzer) 2009-10-19 14:34:53 PDT
Fixed in r49567.  <http://trac.webkit.org/changeset/49567>
Comment 8 Darin Adler 2010-01-17 09:57:34 PST
This change broke the layout of the Dashboard Widget named “Business” included with Mac OS X.
Comment 9 Darin Adler 2010-01-17 10:00:26 PST
(In reply to comment #8)
> This change broke the layout of the Dashboard Widget named “Business” included
> with Mac OS X.

It has a line-height of 1.5 that applies to some blocks with a font size of 9px, and the layout of the widget was relying on the resulting line height being 13px rather than of 14px.
Comment 10 mitz 2010-01-17 10:21:02 PST
(In reply to comment #9)
> (In reply to comment #8)
> > This change broke the layout of the Dashboard Widget named “Business” included
> > with Mac OS X.
> 
> It has a line-height of 1.5 that applies to some blocks with a font size of
> 9px, and the layout of the widget was relying on the resulting line height
> being 13px rather than of 14px.

I just tried this test case
    <div style="line-height: 1.5; font-size: 9px; background-color:
yellow">a</div>
in Firefox (3.5.6) and got a 13px tall block.
Comment 11 mitz 2010-01-17 10:23:59 PST
In Firefox, line-height: 1.54351 still gives 13px, but 1.54352 gives 14px.
Comment 12 mitz 2010-01-18 13:30:58 PST
(In reply to comment #11)
> In Firefox, line-height: 1.54351 still gives 13px, but 1.54352 gives 14px.

Actually, Firefox stores and reports the height (in the case of line-height: 1.5) as 13.5px, and the pixel height on screen depends on where the block is positioned (presumably the render tree uses fractional coordinates and they are rounded to integers closer to render time).
Comment 13 mitz 2010-01-18 19:00:58 PST
Reverted r49567 in <http://trac.webkit.org/projects/webkit/changeset/53450> due it breaking the Business widget.
Comment 14 Victor Wang 2010-01-19 10:03:09 PST
Hi Dan and Darin,

Thanks for taking time looking into this and sorry for breaking the Businees widget (was off yesterday so did not reply earlier).

The test failed is textarea-scrollbar-height.html, and I was checking the clientHeight on different platform to see why this test fails. A couple of examples show me that IE and Firefox are rounding this number, so I thought the line-height is also rounded. Per Dan, looks like FireFox stores floating number as line-height and rounds the result when calculating the clientHeight. In webkit, line-height is truncated and returned as integer, this causes the clientHeight to be truncated and the test fails on Windows. Do you have any suggestions on how to fix the test?

Thanks,
Victor
Comment 15 Peter Kasting 2010-01-19 10:15:50 PST
Note that per comment 0 this behavior affects real-world sites too, so it's not just a matter of changing a test.

I think we should match IE and Firefox if possible.  I would be unhappy to end up in a place where all WebKit-based user agents had to prioritize a particular OS X dashboard widget over the rest of the web, although it's not clear to me that this has come to that.  (If it did come to that, I'd suggest putting this behavior under a flag, turning that flag on only for dashboard widgets, expiring it at a particular WebKit revision number, and shipping a new version of the dashboard widget that checks the revision number.)
Comment 16 Darin Adler 2010-01-19 11:53:07 PST
Sure, we would like to match the other browsers.

Dan’s testing showed the patch did not make us match Firefox, although it made us closer.

We may have to create quirks that keep content that is depending on Safari’s old behavior working. The Dashboard widget is one example; there may be others. Some approach to this does have to be part of the patch now that we’re aware of the issue.
Comment 17 Darin Adler 2010-01-19 11:53:38 PST
The original comment describes this as a Safari 3 to Safari 4 regression. Could someone comment more on why that page worked in Safari 3?
Comment 18 Peter Kasting 2010-01-19 11:57:24 PST
Clarity: The testcase passed in Safari 3/Win.  I don't know if the real-world page looked right or not (I don't recall having tested it).  My suspicion at the time was that the testcase passed because Safari 3/Win defaulted to Mac-style font rendering/metrics (which didn't trip this bug, see comment 2) and Safari 4/Win defaulted to Windows-style font rendering/metrics.
Comment 19 Eric Seidel (no email) 2010-02-02 14:03:07 PST
Comment on attachment 40480 [details]
Proposed patch - round line height

Marking this patch as r- since it was reverted.