Bug 86179 - [Qt] tab-size CSS property doesn't work at all
Summary: [Qt] tab-size CSS property doesn't work at all
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Bruno Abinader (history only)
URL:
Keywords: Qt, QtTriaged
Depends on: 85203 92119
Blocks: 52994 87008
  Show dependency treegraph
 
Reported: 2012-05-11 00:04 PDT by Csaba Osztrogonác
Modified: 2012-07-24 08:34 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 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
Comment 1 Csaba Osztrogonác 2012-05-11 00:14:38 PDT
I skipped it on Qt until proper fix - https://trac.webkit.org/changeset/116734
Comment 2 Allan Sandfeld Jensen 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.
Comment 3 Bruno Abinader (history only) 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?
Comment 4 Bruno Abinader (history only) 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.
Comment 5 Allan Sandfeld Jensen 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.
Comment 6 Caio Marcelo de Oliveira Filho 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.
Comment 7 Bruno Abinader (history only) 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.
Comment 8 Caio Marcelo de Oliveira Filho 2012-07-24 04:59:17 PDT
Bruno, could you make a patch to unskip it? :-)
Comment 9 Bruno Abinader (history only) 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.
Comment 10 Bruno Abinader (history only) 2012-07-24 06:59:14 PDT
Marking as "reopened" to trigger commit-queue.
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-07-24 08:25:45 PDT
All reviewed patches have been landed.  Closing bug.