Bug 33093 - [Qt] LayoutTests/fast/html/text-field-input-types.html
Summary: [Qt] LayoutTests/fast/html/text-field-input-types.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-01 06:55 PST by Robert Hogan
Modified: 2010-02-14 23:47 PST (History)
4 users (show)

See Also:


Attachments
fix patch (1.78 KB, patch)
2010-02-08 10:58 PST, Chang Shu
abarth: review+
Details | Formatted Diff | Diff
fix patch 2 (2.02 KB, patch)
2010-02-13 00:43 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-01-01 06:55:30 PST
This test is skipped on Qt because it's output is:

A B C DE F

rather than

A B C D E F.

In the dumped text the spaces represent an <input> text field.

The reason for the difference is that on Mac all the input fields fit on a single line, whereas on Qt 'D' starts on a new line. This happens because the width() of each of the <input> controls on Qt is 175, which must be larger than it is on Mac. The 175 is arrived at by taking the avgCharWidth of the render style's font (8) and multiplying it by InputElement::s_defaultSize (20), then adding the padding/margins. So the difference must stem from a different avgCharWidth between Mac and Qt.

What's the right thing to do here? Is it a bug that the text dump does not begin on a new line with E and F?

The test itself can be fixed for all platforms by specifying a small size attribute in the <input> markup, maybe that's the right way to go.
Comment 1 Chang Shu 2010-02-08 10:58:31 PST
Created attachment 48344 [details]
fix patch
Comment 2 Adam Barth 2010-02-09 12:38:41 PST
Comment on attachment 48344 [details]
fix patch

Hum...  I think this patch is correct.  There's a separate question of whether you want the Qt metrics to match Mac, but that's not what this test is trying to test, so I think that's better dealt with separately.

I agree that it's possible we should improve the text dumping to be immune to these issues.  If this issue recurs, we can revisit that question.

BTW, it would be helpful if you'd copy more of your analysis from Comment #0 into the ChangeLog.  I found Comment #0 quite informative but you ChangeLog somewhat mysterious.
Comment 3 Chang Shu 2010-02-13 00:08:23 PST
(In reply to comment #2)
> (From update of attachment 48344 [details])
> Hum...  I think this patch is correct.  There's a separate question of whether
> you want the Qt metrics to match Mac, but that's not what this test is trying
> to test, so I think that's better dealt with separately.
> I agree that it's possible we should improve the text dumping to be immune to
> these issues.  If this issue recurs, we can revisit that question.
> BTW, it would be helpful if you'd copy more of your analysis from Comment #0
> into the ChangeLog.  I found Comment #0 quite informative but you ChangeLog
> somewhat mysterious.

Thanks for the review, Adam. And thanks to Robert, too. It is actually Robert who did all the analysis. I made this patch simply because this bug blocked my further work on making some of the input types working on Qt. And I also think the intention of the test case is somewhere else. Yes, I will add Robert's comment into the ChangLog.
Comment 4 Chang Shu 2010-02-13 00:43:32 PST
Created attachment 48701 [details]
fix patch 2
Comment 5 Adam Barth 2010-02-13 00:48:55 PST
Comment on attachment 48701 [details]
fix patch 2

Thanks.
Comment 6 WebKit Commit Bot 2010-02-14 21:14:57 PST
Comment on attachment 48701 [details]
fix patch 2

Clearing flags on attachment: 48701

Committed r54761: <http://trac.webkit.org/changeset/54761>
Comment 7 WebKit Commit Bot 2010-02-14 21:15:07 PST
All reviewed patches have been landed.  Closing bug.