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 ;).
Created attachment 2617 [details] Screenshot of safari behavior.
Created attachment 2618 [details] Screenshot of mozilla behavior Correct behavior in my opinion.
Created attachment 2619 [details] Testcase for word-spacing
Created attachment 2621 [details] tesctase in xhtml strict
Created attachment 2622 [details] fix for xhtml strict test.
Created attachment 2623 [details] better fix for xhtml test
in xhtml, it doesn't seem to work at all in this test. it seems as tho the inline style information is completely ignored.
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...
Created attachment 2734 [details] über-test-case
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 :) )
Created attachment 2812 [details] Corrected formatting
Comment on attachment 2812 [details] Corrected formatting Removing review request as patch no longer applies to ToT
Created attachment 2862 [details] Uber patch to fix all wordspacing issues This will probably need to be tested for performance related badness
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
Created attachment 2863 [details] Uber patch to fix all wordspace issues
Created attachment 2864 [details] layout test input Testcase to match soon to be attached dumprendertree output
Created attachment 2865 [details] expected output from wordspacing test
Comment on attachment 2864 [details] layout test input Looks like a good layout test.
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.
Created attachment 3171 [details] Fixed and remerged patch Okay this fixes the word-spacing patch to work with current ToT
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.
Created attachment 3185 [details] reformatted patch Reformatted to (hopefully) match coding guidelines
Created attachment 3186 [details] Yet more reformatting
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.
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
Created attachment 3205 [details] layout test input Fixed misleading text
Created attachment 3206 [details] layout test expected
Created attachment 3207 [details] More reformatting Hopefully this should be the last of the reformatting
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.
Created attachment 3221 [details] Wordspace patch with hopefully correct formatting
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.
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.
*** Bug 3793 has been marked as a duplicate of this bug. ***