Bug 86179

Summary: [Qt] tab-size CSS property doesn't work at all
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: 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: P2 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-
DetailsFormatted DiffDiff
Unskip layout test patch (2.01 KB, patch)
2012-07-24 05:56 PDT, Bruno Abinader (history only) 		no flags
DetailsFormatted DiffDiff
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.