Bug 91990 – [Qt] Default sizes for input-text and text-area are different when running DRT/WTR

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Caio Marcelo de Oliveira Filho]

For testing we use the same fonts and the same RenderTheme for WK1 and WK2. I think the default sizes for forms should be the same as well, so we can keep the same expected results for both WK1 and WK2.

I identified this problem while rebaselining editing/inserting for the new fonts. Two tests failing in WK2 because of this are:

editing/inserting/4960120-1.html
editing/inserting/before-after-input-element.html

[Caio Marcelo de Oliveira Filho]

tables/mozilla_expected_failures/bugs/bug92647-1.html

[Caio Marcelo de Oliveira Filho]

css3/selectors3/html/css3-modsel-23.html
css3/selectors3/html/css3-modsel-24.html
css3/selectors3/html/css3-modsel-68.html
css3/selectors3/html/css3-modsel-69.html
css3/selectors3/xhtml/css3-modsel-23.xml
css3/selectors3/xhtml/css3-modsel-24.xml
css3/selectors3/xhtml/css3-modsel-68.xml
css3/selectors3/xhtml/css3-modsel-69.xml
css3/selectors3/xml/css3-modsel-23.xml
css3/selectors3/xml/css3-modsel-24.xml
css3/selectors3/xml/css3-modsel-68.xml
css3/selectors3/xml/css3-modsel-69.xml

[Bruno Abinader (history only)]

editing/selection/3690703-2.html
editing/selection/3690703.html
editing/selection/3690719.html

(and more to come from previous rebaselines)

[Bruno Abinader (history only)]

editing/selection/4895428-3.html
editing/selection/4975120.html
editing/selection/drag-select-1.html

[Bruno Abinader (history only)]

fast/forms/basic-inputs.html
fast/forms/basic-textareas-quirks.html
fast/forms/control-restrict-line-height.html
fast/forms/encoding-test.html
fast/forms/fieldset-align.html
fast/forms/input-appearance-bkcolor.html
fast/forms/input-appearance-default-bkcolor.html
fast/forms/input-appearance-disabled.html
fast/forms/input-appearance-focus.html
fast/forms/input-appearance-preventDefault.html
fast/forms/input-appearance-readonly.html
fast/forms/input-appearance-selection.html
fast/forms/input-appearance-visibility.html
fast/forms/input-baseline.html
fast/forms/input-double-click-selection-gap-bug.html
fast/forms/input-placeholder-visibility-1.html
fast/forms/input-placeholder-visibility-3.html
fast/forms/input-readonly-dimmed.html
fast/forms/input-readonly-empty.html
fast/forms/input-spaces.html
fast/forms/input-text-click-inside.html
fast/forms/input-text-double-click.html
fast/forms/input-text-option-delete.html
fast/forms/input-text-self-emptying-click.html
fast/forms/input-text-word-wrap.html
fast/forms/input-width.html
fast/forms/placeholder-position.html
fast/forms/search-cancel-button-style-sharing.html
fast/forms/search-display-none-cancel-button.html
fast/forms/search-rtl.html
fast/forms/search-vertical-alignment.html
fast/forms/text-style-color.html
fast/forms/textarea-placeholder-pseudo-style.html
fast/forms/textarea-placeholder-visibility-1.html
fast/forms/textarea-placeholder-visibility-2.html
fast/forms/textfield-focus-ring.html
fast/forms/textfield-outline.html
fast/forms/textfield-overflow.html

[Bruno Abinader (history only)]

fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/text/textIteratorNilRenderer.html

[Bruno Abinader (history only)]

fast/table/colspanMinWidth.html
fast/table/spanOverlapRepaint.html
fast/table/text-field-baseline.html

[Csaba Osztrogonác]

tables/mozilla/bugs/bug1188.html
tables/mozilla/bugs/bug18359.html
tables/mozilla/bugs/bug24200.html
tables/mozilla/bugs/bug2479-3.html
tables/mozilla/bugs/bug28928.html
tables/mozilla/bugs/bug30559.html
tables/mozilla/bugs/bug4382.html
tables/mozilla/bugs/bug4527.html
tables/mozilla/bugs/bug46368-1.html
tables/mozilla/bugs/bug46368-2.html
tables/mozilla/bugs/bug51037.html
tables/mozilla/bugs/bug55545.html
tables/mozilla/bugs/bug59354.html
tables/mozilla/bugs/bug96334.html
tables/mozilla/dom/tableDom.html
tables/mozilla/other/move_row.html

[Csaba Osztrogonác]

There are only 83 tests related to this bug. I think we can simple check in qt-5.0-wk2 specific results to have better test coverage until find a proper fix this bug. ( or feauture? :) )

[Caio Marcelo de Oliveira Filho]

(In reply to comment #9)
> There are only 83 tests related to this bug. I think we can simple check in qt-5.0-wk2 specific results to have better test coverage until find a proper fix this bug. ( or feauture? :) )

I don't think it's a good idea, given that the Luciano is already working on a fix for this and there's no essential reason to WK2 behave differently than WK1 in those cases.

In general I think having WK2 specific results should be seen as the last option, only have them if we have a very good reason.

[Csaba Osztrogonác]

I didn't notice if Luciano is working on it. Of course having same results 
for WK1 and WK2 is the best solution. :) But if we don't find the proper
solution for a long time, we should commit WK2 specific expected results 
too not to lose test coverage for these tests. The results for WK2 aren't
wrong, only a little bit different.

[Luciano Wolf]

I'm investigating and one of my findings is that DRT and WTR use different fonts (Liberation Serif / Times). Now I'm trying to implement the functions inside webkit/Source/WebKit2/UIProcess/qt/WebPreferencesQt.cpp to make it use the same font as DRT. The current implementation uses default values from webkit/Source/WebKit2/Shared/WebPreferencesStore.h. I hope to close this bug till this weekend.

[Luciano Wolf]

Created attachment 156686 [details]
Use correct fonts from testfonts directory

[Balazs Kelemen]

Comment on attachment 156686 [details]
Use correct fonts from testfonts directory

View in context: https://bugs.webkit.org/attachment.cgi?id=156686&action=review

Here are my informal complaining comments :)

> Source/WebKit2/UIProcess/qt/WebPreferencesQt.cpp:56
> +#define INITIALIZE_PREFERENCE_FROM_NSUSERDEFAULTS(KeyUpper, KeyLower, TypeName, Type, DefaultValue) \
> +    set##TypeName##ValueIfInUserDefaults(WebPreferencesKey::KeyLower##Key(), m_store, qFontHint);

This is a crasy name for this macro :)

> Tools/WebKitTestRunner/qt/main.cpp:124
> +    WebKit::initializeTestFonts();
> +    QCoreApplication::setAttribute(Qt::AA_Use96Dpi, true);
> +

Does that mean that we draw input-tet and text-area on the UI side? Otherwise it's not clear to me why this is necessary.

[Luciano Wolf]

(In reply to comment #14)
> (From update of attachment 156686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156686&action=review
> 
> Here are my informal complaining comments :)
> 
> > Source/WebKit2/UIProcess/qt/WebPreferencesQt.cpp:56
> > +#define INITIALIZE_PREFERENCE_FROM_NSUSERDEFAULTS(KeyUpper, KeyLower, TypeName, Type, DefaultValue) \
> > +    set##TypeName##ValueIfInUserDefaults(WebPreferencesKey::KeyLower##Key(), m_store, qFontHint);
> 
> This is a crasy name for this macro :)

I took it from Mac implementation :) I'll rename it to something better.

> 
> > Tools/WebKitTestRunner/qt/main.cpp:124
> > +    WebKit::initializeTestFonts();
> > +    QCoreApplication::setAttribute(Qt::AA_Use96Dpi, true);
> > +
> 
> Does that mean that we draw input-tet and text-area on the UI side? Otherwise it's not clear to me why this is necessary.

If we don't initialize fontconfig stuff before reaching that macro QFont.defaultFamily returns different (Dejavu font type) results. The way I found to fix this behaviour was to initialize here - as DumpRenderTree does.

[Luciano Wolf]

Created attachment 156717 [details]
Macro has been renamed

[Luciano Wolf]

Created attachment 156746 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 156746 [details]
Patch for landing

Clearing flags on attachment: 156746

Committed r124808: <http://trac.webkit.org/changeset/124808>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> (From update of attachment 156746 [details])
> Clearing flags on attachment: 156746
> 
> Committed r124808: <http://trac.webkit.org/changeset/124808>

You forgot to unskip tests and remove unnecessary qt-5.0-wk2 expected files.
I did it - https://trac.webkit.org/changeset/124873

[Luciano Wolf]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 156746 [details] [details])
> > Clearing flags on attachment: 156746
> > 
> > Committed r124808: <http://trac.webkit.org/changeset/124808>
> 
> You forgot to unskip tests and remove unnecessary qt-5.0-wk2 expected files.
> I did it - https://trac.webkit.org/changeset/124873

oops! Thanks a lot!