Bug 86179

Summary:

[Qt] tab-size CSS property doesn't work at all

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

Tools / Tests

Assignee:

Bruno Abinader (history only) <bruno.abinader>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, bruno.abinader, cmarcelo, morrita, ossy, simon.fraser, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

85203, 92119

Bug Blocks:

52994, 87008

Attachments:

Description	Flags
"Force tab size layout test to use monospace" patch	cmarcelo: review-
Unskip layout test patch	none

Csaba Osztrogonác

Reported 2012-05-11 00:04:23 PDT

New test introduced in https://trac.webkit.org/changeset/116723 fails on Qt. It seems tab-size doesn't work at all on Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r116723%20%2822802%29/fast/css/tab-size-diffs.html

Attachments
"Force tab size layout test to use monospace" patch (2.75 KB, patch) 2012-06-14 11:12 PDT, Bruno Abinader (history only)	cmarcelo: review-	Details Formatted Diff Diff
Unskip layout test patch (2.01 KB, patch) 2012-07-24 05:56 PDT, Bruno Abinader (history only)	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Csaba Osztrogonác

Comment 1 2012-05-11 00:14:38 PDT

I skipped it on Qt until proper fix - https://trac.webkit.org/changeset/116734

Allan Sandfeld Jensen

Comment 2 2012-05-14 03:38:21 PDT

It is not limited to tab-size. Qt platform doesn't support tabs at all. We always normalize the tabs to spaces before drawing them.

Bruno Abinader (history only)

Comment 3 2012-06-14 10:34:19 PDT

Adding bug 85203 as dependency, using Caio's patches for modifying the default font the results are now passing. The issue is related to the fact that non-monospaced fonts do have different lengths for each character, so now if the tab size, for example, equals to 8, then it will be exactly the lenght of a whitespace multiplied by 8. For even better control,a font-family:monospace would force even more proper image comparison results, what do you guys think?

Bruno Abinader (history only)

Comment 4 2012-06-14 11:12:14 PDT

Created attachment 147619 [details] "Force tab size layout test to use monospace" patch This patch forces the tab size layout test to use monospace font family. However, please notice this patch *still* does not fix the Qt issue as it depends on bug 85203. This patch makes sure every other platform uses monospace to avoid character/whitespace length variations that could cause test image comparison to fail.

Allan Sandfeld Jensen

Comment 5 2012-06-18 09:16:37 PDT

(In reply to comment #2) > It is not limited to tab-size. Qt platform doesn't support tabs at all. We always normalize the tabs to spaces before drawing them. This comment is obviously wrong, so just ignore it. I was confused by the fact that some of other backends handle tabs in the platform-code while Qt doesn't.

Caio Marcelo de Oliveira Filho

Comment 6 2012-07-23 15:00:39 PDT

Comment on attachment 147619 [details] "Force tab size layout test to use monospace" patch View in context: https://bugs.webkit.org/attachment.cgi?id=147619&action=review > LayoutTests/fast/css/tab-size.html:43 > +<body style="font-family:'monospace'" onload="test();"> > <div id="template" style="display:none;"> > <pre>&Tab;x</pre> Setting the style here is wrong. We are using <pre> tag and it should select a monospace font in the test environment.

Bruno Abinader (history only)

Comment 7 2012-07-23 18:43:50 PDT

Thanks Caio :) My bad actually, I've runned the patch back when patch from bug 85203 wasn't applied yet, so <pre> was not properly using monospace-like fonts, thus the forced usage. After bug 85203, layout tests results are okay, so if anyone mind I'm setting this bug to "fixed". (In reply to comment #6) > (From update of attachment 147619 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147619&action=review > > > LayoutTests/fast/css/tab-size.html:43 > > +<body style="font-family:'monospace'" onload="test();"> > > <div id="template" style="display:none;"> > > <pre>&Tab;x</pre> > > Setting the style here is wrong. We are using <pre> tag and it should select a monospace font in the test environment.

Caio Marcelo de Oliveira Filho

Comment 8 2012-07-24 04:59:17 PDT

Bruno, could you make a patch to unskip it? :-)

Bruno Abinader (history only)

Comment 9 2012-07-24 05:56:59 PDT

Created attachment 154032 [details] Unskip layout test patch This patch unskips the fast/css/tab-size.html layout test, after monospace fix from r121971.

Bruno Abinader (history only)

Comment 10 2012-07-24 06:59:14 PDT

Marking as "reopened" to trigger commit-queue.

WebKit Review Bot

Comment 11 2012-07-24 08:25:40 PDT

Comment on attachment 154032 [details] Unskip layout test patch Clearing flags on attachment: 154032 Committed r123478: <http://trac.webkit.org/changeset/123478>

WebKit Review Bot

Comment 12 2012-07-24 08:25:45 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.