Bug 29647 – [Qt] <select>s on http://www.ryanair.com render wrong

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Yamin]

Hi,

<select>s are truncated on http://www.ryanair.com. The last character on the two 'Depart Date' fields is truncated and not visible. It's displayed fine on Firefox (Linux) and Safari (Windows).

Problem can be observed with Arora and Qt-WebKit 4.5.2 on Linux. Screen shots and a patch is attached.

Cheers,

Tim

[Tim Yamin]

Created attachment 39933 [details]
Patch to fix issue

[Tim Yamin]

Created attachment 39934 [details]
Screenshot (before)

[Tim Yamin]

Created attachment 39935 [details]
Screenshot (after)

[Tor Arne Vestbø]

Thanks for your bug report.

In the future, please follow the QtWebKit bug reporting guidlines:

http://trac.webkit.org/wiki/QtWebKitContrib#ReportingBugs

In particular:

	- All bugs related to the Qt port of WebKit should have the keyword 'Qt'
	- The 'WebKit Qt' component should only be used for the QtWebKit API layer

[Jocelyn Turcotte]

Reproduced in trunk.

Tim, your patch attachment misses a ChangeLog and the review=? flag.
Please see http://trac.webkit.org/wiki/QtWebKitContrib for more information.

[Andreas Kling]

Created attachment 51901 [details]
Same patch in proper format

[Eric Seidel (no email)]

Comment on attachment 51901 [details]
Same patch in proper format

I take it that Qt does not run pixel tests yet?

[Luiz Agostini]

Created attachment 52544 [details]
MacOS screenshots.

I tryed this patch in macos and the result was not so good.

Attached is an image that has three screenshots of QtLauncher showing a simple page with three combos. The difference between them is the padding used in the combo. The first screenshot uses the proposed patch.

Why don't  just to increase the right padding and keep left and bottom padding as they are now?

[Simon Hausmann]

Comment on attachment 51901 [details]
Same patch in proper format

Luiz' screenshot is very convincing ;(

[Robert Hogan]

(In reply to comment #8)
> Created an attachment (id=52544) [details]
> MacOS screenshots.
> 
> I tryed this patch in macos and the result was not so good.
> 
> Attached is an image that has three screenshots of QtLauncher showing a simple page with three combos. The difference between them is the padding used in the combo. The first screenshot uses the proposed patch.
> 
> Why don't  just to increase the right padding and keep left and bottom padding as they are now?

Luiz, do you still have your test case for this? Were you constraining the size of the combo box?

[Robert Hogan]

Worth noting that the site fixes the width of the menulist to 90px and specifies 9px for the font. 

There are a few things wrong with the way QtWebKit is rendering menulists:

- There's no bottom padding so the text is positioned too low in the drop-down box.
- The menulist widgets are much taller than they need to be. QtWebKit needs to follow Firefox and others by by hugging the text a little more. The net effect is a lot neater.
- Glyphs like g and j have their tails obscured in most styles I've tried in QtTestBrowser with this page.
- The bug highlighted here, which seems (i) at least partly a consequence of QtWebKit's hefty left and right padding to cater for styles like oxigen, far exceeding the padding used by other ports (4 is the largest I can find anywhere else, in RenderThemeSafari). And (ii) you can't help feeling there's a font-size bug in here somewhere.

The first three are probably separate bugs. For this one I'd really like to try the test case behind Luiz's screenshots as I've found motif, windows, oxygen and gtk all behave quite well when the right padding on menulists is reduced to zero.

[Luiz Agostini]

(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=52544) [details] [details]
> > MacOS screenshots.
> > 
> > I tryed this patch in macos and the result was not so good.
> > 
> > Attached is an image that has three screenshots of QtLauncher showing a simple page with three combos. The difference between them is the padding used in the combo. The first screenshot uses the proposed patch.
> > 
> > Why don't  just to increase the right padding and keep left and bottom padding as they are now?
> 
> Luiz, do you still have your test case for this? Were you constraining the size of the combo box?

No, I do not have it. The one I found here is a little bit different but I would say that the one used at that time was just

<select><option>option 10</option></select>
<select><option>option 100</option></select>
<select><option>option 1000</option></select>

with no size constraint. This simple html in QtTestBrowser reproduced exactly the second screenshot of those three, as expected.

[Robert Hogan]

(In reply to comment #12)
> 
> No, I do not have it. The one I found here is a little bit different but I would say that the one used at that time was just
> 
> <select><option>option 10</option></select>
> <select><option>option 100</option></select>
> <select><option>option 1000</option></select>
> 
> with no size constraint. This simple html in QtTestBrowser reproduced exactly the second screenshot of those three, as expected.

There seems to be something amiss with the MacOS style: pixelMetric() is not useful to tell us the width of the combo-box drop down button. The first example in your screenshot shows this - if the only padding is the width of the button icon, the text should not impinge on it. So we need to find another way of gauging the width of the button icon - or fix the value reported by the MacOS style.

[Robert Hogan]

(In reply to comment #13)
> There seems to be something amiss with the MacOS style: pixelMetric() is not useful to tell us the width of the combo-box drop down button. The first example in your screenshot shows this - if the only padding is the width of the button icon, the text should not impinge on it. So we need to find another way of gauging the width of the button icon - or fix the value reported by the MacOS style.

Right enough, QMacStyle::pixelMetric() does not support QStyle::PM_ButtonIconSize so we get the default windows style value instead. Maybe I'm looking in the wrong place but the padding for Mac does not seem to be available through style metrics, instead they have to do stuff like:

const int* RenderThemeSafari::popupButtonPadding(NSControlSize size) const
{
    static const int padding[3][4] =
    {
        { 2, 26, 3, 8 },
        { 2, 23, 3, 8 },
        { 2, 22, 3, 10 }
    };
    return padding[size];
}

So it looks like we need a compile-guard for MAC builds to retain the current (large) padding on OSX and allow other styles to use QStyle::PM_ButtonIconSize + a padding of 2.

Sound sensible?

[Luiz Agostini]

Created attachment 83326 [details]
patch

This patch reduces padding as previous one and introduces a QMacStyle specific adjustment.
The reason for this mac specific adjustment is explained in a comment in the code.

[WebKit Commit Bot]

Comment on attachment 83326 [details]
patch

Clearing flags on attachment: 83326

Committed r79407: <http://trac.webkit.org/changeset/79407>

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed r79407: <http://trac.webkit.org/changeset/79407>

I updated Qt specific expected files: http://trac.webkit.org/changeset/79414
Guys, don't you run layout tests before committing?

Can we close this bug now?

[Luiz Agostini]

(In reply to comment #17)
> (In reply to comment #16)
> > Committed r79407: <http://trac.webkit.org/changeset/79407>
> 
> I updated Qt specific expected files: http://trac.webkit.org/changeset/79414
> Guys, don't you run layout tests before committing?

sure

> 
> Can we close this bug now?

I think we can.