Bug 3692 – Word-spacing doesn't work as expected

• Component: CSS
• Status: CLOSED FIXED

[Joost de Valk (AlthA)]

Word-spacing works perfectly on plain text, but as soon as an other tag is placed around text, like span or 
an anchor, the word-spacing doesn't take place anymore. This behavior is different than behavior in IE 
5:mac and Firefox. Will attach a testcase and screenshots of safari and firefox behavior.

This actually breaks my mother's site in safari, please fit it ;).

[Joost de Valk (AlthA)]

Created attachment 2617 [details]
Screenshot of safari behavior.

[Joost de Valk (AlthA)]

Created attachment 2618 [details]
Screenshot of mozilla behavior

Correct behavior in my opinion.

[Joost de Valk (AlthA)]

Created attachment 2619 [details]
Testcase for word-spacing

[Joost de Valk (AlthA)]

Created attachment 2621 [details]
tesctase in xhtml strict

[Joost de Valk (AlthA)]

Created attachment 2622 [details]
fix for xhtml strict test.

[Joost de Valk (AlthA)]

Created attachment 2623 [details]
better fix for xhtml test

[Joost de Valk (AlthA)]

in xhtml, it doesn't seem to work at all in this test. it seems as tho the inline style information is 
completely ignored.

[Oliver Hunt]

Created attachment 2728 [details]
More blatantly obvious breakge

This test case uses the same word, and monospace for the font family, in order
to make or easier to work out what is going on...

[Joost de Valk (AlthA)]

Created attachment 2734 [details]
über-test-case

[Oliver Hunt]

Created attachment 2810 [details]
Prelim Patch

Okay, this patch is far from tidy, and currently conflicts with the current
patch for bug 3793 (which it will either replace or depend on :) )

[Oliver Hunt]

Created attachment 2812 [details]
Corrected formatting

[Oliver Hunt]

Comment on attachment 2812 [details]
Corrected formatting

Removing review request as patch no longer applies to ToT

[Oliver Hunt]

Created attachment 2862 [details]
Uber patch to fix all wordspacing issues

This will probably need to be tested for performance related badness

[Oliver Hunt]

Comment on attachment 2862 [details]
Uber patch to fix all wordspacing issues

Among other things this will definitely need a performance review of some kind

[Oliver Hunt]

Created attachment 2863 [details]
Uber patch to fix all wordspace issues

[Oliver Hunt]

Created attachment 2864 [details]
layout test input

Testcase to match soon to be attached dumprendertree output

[Oliver Hunt]

Created attachment 2865 [details]
expected output from wordspacing test

[Darin Adler]

Comment on attachment 2864 [details]
layout test input

Looks like a good layout test.

[Darin Adler]

Comment on attachment 2865 [details]
expected output from wordspacing test

We don't really need to do the patch review process on the expected output, so
I'm removing the flag.

[Oliver Hunt]

Created attachment 3171 [details]
Fixed and remerged patch

Okay this fixes the word-spacing patch to work with current ToT

[Darin Adler]

Comment on attachment 3171 [details]
Fixed and remerged patch

There are some minor formatting issues with the patch, for example:

+	     if(r->box->isInlineTextBox()){
+	       InlineTextBox * text=static_cast<InlineTextBox*>(r->box);

should instead be:

	    if (r->box->isInlineTextBox()) {
	      InlineTextBox *text = static_cast<InlineTextBox *>(r->box);

I think Dave Hyatt should be the one to do a real review of this.

[Oliver Hunt]

Created attachment 3185 [details]
reformatted patch

Reformatted to (hopefully) match coding guidelines

[Oliver Hunt]

Created attachment 3186 [details]
Yet more reformatting

[Darin Adler]

Comment on attachment 3186 [details]
Yet more reformatting

Formatting looks pretty good now.

Could still use a little tweaking to make spaces match how we usually do them.
For example:

+    void setStart(uint start) {m_start=start;}

would normally be:

    void setStart(uint start) { m_start=start; }

But the more important issue here than formatting is getting a conceptual
review from Dave Hyatt and some performance testing. I'd love to see this patch
land soon, but both of those have to happen first.

[Eric Seidel (no email)]

Ran autovicki per olliej's request.  AV showed no regression:

Before:
TestSuite: PageLoadTest
TestType: cvs-base
OSVersion: 8C46
SafariVersion: (v412+)
Machine Model: PowerBook G4 12"
Number Of CPUs: 1
CPU Speed: 1.33 GHz
Memory: 1.25 GB
Date: Mon Aug  1 21:32:36 PDT 2005
==================================

cvs-base Cold:      	0.418
cvs-base Uncached:      0.225
cvs-base AvgCached:     0.158
cvs-base Cached 1:  0.159
cvs-base Cached 2:  0.158
cvs-base Cached 3:  0.158
cvs-base Cached 4:  0.159
cvs-base Cached 5:  0.157



After:
TestSuite: PageLoadTest
TestType: cvs-base
OSVersion: 8C46
SafariVersion: (v412+)
Machine Model: PowerBook G4 12"
Number Of CPUs: 1
CPU Speed: 1.33 GHz
Memory: 1.25 GB
Date: Mon Aug  1 21:39:23 PDT 2005
==================================

cvs-base Cold:      	0.430
cvs-base Uncached:      0.226
cvs-base AvgCached:     0.158
cvs-base Cached 1:  0.160
cvs-base Cached 2:  0.159
cvs-base Cached 3:  0.158
cvs-base Cached 4:  0.159
cvs-base Cached 5:  0.158

[Oliver Hunt]

Created attachment 3205 [details]
layout test input

Fixed misleading text

[Oliver Hunt]

Created attachment 3206 [details]
layout test expected

[Oliver Hunt]

Created attachment 3207 [details]
More reformatting

Hopefully this should be the last of the reformatting

[Dave Hyatt]

Comment on attachment 3207 [details]
More reformatting

Patch seems ok conceptually.  Just needs some clean-up in terms of formatting. 
Some comments:

(1) There's a lot of 2-space indentation being used.  Our standard is 4-space
indentation, so go over the patch and make sure the indentation is correct.

(2) You patched some dead code (slated for removal) in font.cpp.  I don't see
any reason to update that code path.  You can just leave those changes out of
the patch.

(3) Make sure to match our coding conventions.	I know a lot of the surrounding
code does not, but make sure new code does.  For example, this line:

+	     if ( (r->start == 0) && needsWordSpacing &&
(rt->text()[r->start].isSpace()) ) {
+	       effectiveWidth += rt->htmlFont(m_firstLine)->getWordSpacing();
+	     }

Get rid of the extra spaces after the opening of the if ( and before the ). 
Also get rid of the unnecessary braces.

(5) For this line:

+    if (needsWordSpacing && len>1) 


len > 1 rather than len>1.  Yes, I know a lot of the surrounding code doesn't
obey current conventions, but it's all due for a big cleanup soon. :)

Logically everything looks great.  Let's just get all the formatting nitpicks
out of the way and then we can get this landed.

I do think (since compacts have been touched a bit) that the word-spacing uber
test should test display: compact to make sure word-spacing is properly
factoring in to the calculations for whether or not they can fit in the block's
margin.

[Oliver Hunt]

Created attachment 3221 [details]
Wordspace patch with hopefully correct formatting

[Darin Adler]

Comment on attachment 2864 [details]
layout test input

Just clearing the review flag from this layout test because otherwise this
shows up in "reviewed patches that need to be committed" queries.

[Maciej Stachowiak]

Comment on attachment 3221 [details]
Wordspace patch with hopefully correct formatting

This does still have one minor coding style issue:

+		 if ((r->start == 0) && needsWordSpacing &&
(rt->text()[r->start].isSpace())) {
+		     effectiveWidth +=
rt->htmlFont(m_firstLine)->getWordSpacing();
+		 }


shouldn't be braces here. But let's say review+ anyway.

[Oliver Hunt]

*** Bug 3793 has been marked as a duplicate of this bug. ***