Closed Bug 418975 Opened 12 years ago Closed 9 years ago

Soft hyphen not working in tables.

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: hoks, Assigned: jfkthame)

References

Details

(Keywords: testcase)

Attachments

(6 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

The entities ­ or ­ is ignored when used within tables.


Reproducible: Always

Steps to Reproduce:
<table border="1">
<tr>
<td width="250">WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW&shy;WWWWWWWW</td>
<td width="50">AAA VVVV</td>
</tr>
</table>
Actual Results:  
The line is not wrapped.

Expected Results:  
The line should be wrapped
Attached file Testcase
Component: General → Layout: Fonts and Text
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Version: unspecified → Trunk
Is there a certain reason why this doesn't work with display table-cell?
We calculate the cell minimum width assuming soft hyphens are not used. It would be easy to change that decision, but I'm not sure that we should.
I've run into a bug where my website depends on this being fixed to render properly.  

The behavior of webkit, IE, and Opera calculate cell minimum width with soft hyphens (if available).  Obviously that doesn't mean Gecko has to, but consistency would be nice.

The goal of the soft hyphens in my case was to prevent the table cell from growing beyond a fixed width under any circumstances.

I should mention that in the original &shy; bug, Bug 9101, there is a testcase "Soft Hyphen Test in a Simple Table" the firefox nightly currently fails (which I believe this bug covers).
okay, I guess consistency is good.
Assignee: nobody → roc
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9+
Actually I think we shouldn't fix this for 1.9.

The fix would be risky since touching the text min-width code is an easy way to break the world. Also, while it would be nice to be consistent with other browsers, since FF2 ignored soft hyphens for min-width calculations, we're not going to create any new problems by keeping that behaviour for one more release.
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
I agree with Cliff #4: Inside tables, a &shy; is often used to prevent that the table grows too wide. It's frustrating to see that this doesn't work with FF.
I just wanted to submit this bug but found this one.

Some refinement: Soft hyphenation DOES work in tables, but not for the first word in a cell (see my attachment).

I'm working on an automatic hyphenation script (http://code.google.com/p/hyphenator/) and it would be great, if Firefox would - finally - behave like other browsers!
Apparently, the soft hyphen behaviour also breaks when it's inside a <fieldset> element. Not sure if that's supposed to be raised as a separate bug.
OS: Windows XP → All
Hardware: PC → All
Note that the &shy; character must be replaced by the hyphen ('-') character at the point where the word is hyphened!

The test file 'How soft hyphenation does and does not work in tables' (in attachments) clearly demonstrates that this does not work correctly. In the test, if the hyphenation works, the &shy; character is only deleted and a break is generated.

(Tested with Firefox 3.0.5).
Attached file test case
Check this test case. Not to mention expanded fixed width of a table, Firefox acts kind of strange depending on if there's a space in the line we want to break.
This would be a fun introduction to intrinsic-min-width computation for Jonathan :-)
Assignee: nobody → jfkthame
This makes the testcases here all work as intended, and still passes all our unit tests so I don't think it breaks too much of the world. :)
Attachment #517369 - Flags: review?(roc)
Duplicate of this bug: 617239
+  nsAutoTArray<PRPackedBool,BIG_TEXT_NODE_SIZE> hyphBuffer;

Just use an nsTArray, since we won't usually need this buffer.

+  PRUint32 len =
+    tmp.ConvertSkippedToOriginal(flowEndInTextRun) - iter.GetOriginalOffset();
   PropertyProvider provider(mTextRun, textStyle, frag, this,
-                            iter, PR_INT32_MAX, nsnull, 0);
+                            iter, len, nsnull, 0);

Why compute len here?

+    // In the case of a break at &shy;, we need to add the width of the hyphen
+    // to the current line. (This is not done by nsTextFrameThebes before
+    // calling OptionallyBreak in case the break is not actually taken.)

The comment could be a little more clear, something like
 // If the break is taken, aHyphenWidth must be added to the width of the current
 // line.
(In reply to comment #17)
> +  nsAutoTArray<PRPackedBool,BIG_TEXT_NODE_SIZE> hyphBuffer;
> 
> Just use an nsTArray, since we won't usually need this buffer.

Oops, I changed my mind on this, ignore :-)
Comment on attachment 517370 [details] [diff] [review]
reftests for soft hyphen in table cells

Please add <!DOCTYPE HTML> to the start of each test to make sure all browsers are in standards mode...
Attachment #517370 - Flags: review?(roc) → review+
(In reply to comment #17)
> +  PRUint32 len =
> +    tmp.ConvertSkippedToOriginal(flowEndInTextRun) - iter.GetOriginalOffset();
>    PropertyProvider provider(mTextRun, textStyle, frag, this,
> -                            iter, PR_INT32_MAX, nsnull, 0);
> +                            iter, len, nsnull, 0);
> 
> Why compute len here?

Because PropertyProvider::GetHyphenationBreaks wants a bounded length - see its NS_PRECONDITIONs. (Or did you mean "why use a local variable"? Just for clarity and ease of debugging; the compiler should optimize it away in the end.)
Can we provide that length only in the hyphens case? There's a bit of overhead in converting the offsets.
This is modified so that it only computes the length to pass to the PropertyProvider if hyphenation is actually present.
Attachment #517369 - Attachment is obsolete: true
Attachment #517710 - Flags: review?(roc)
Attachment #517369 - Flags: review?(roc)
Pushed to cedar, in preparation for landing on m-c:
http://hg.mozilla.org/projects/cedar/rev/25beb9ced8d2 (patch)
http://hg.mozilla.org/projects/cedar/rev/03070beac9e7 (tests)
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/25beb9ced8d2
http://hg.mozilla.org/mozilla-central/rev/03070beac9e7
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
I backed this out because it caused a new topcrash, bug 645072.

I didn't back out the tests patch -- instead I marked the tests as failing.

I also added the simplified testcase for the crash as a crashtest (although I didn't test that it actually crashed when in the crashtest harness).

https://hg.mozilla.org/mozilla-central/rev/5c844a79e5c1
https://hg.mozilla.org/mozilla-central/rev/f1d26af4c57b
https://hg.mozilla.org/mozilla-central/rev/1d3457c061ff


This should reland once the crash is fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So when relanding, you should (a) land the revised patch and (b) back out https://hg.mozilla.org/mozilla-central/rev/f1d26af4c57b .
Added a check in PropertyProvider::GetHyphenationBreaks to return immediately if aLength is zero, so that we don't attempt to look at the (non-existent) first character in the fragment in the case where it is zero-length. This resolves the crashes in local testing - will push to tryserver as well, to check the crashtests there.
Attachment #522184 - Flags: review?(roc)
Attachment #517710 - Attachment is obsolete: true
Is run.NextRun() returning a zero-length run in this case? If that's the problem, I'd prefer we fixed this by ensuring NextRun() never returns zero-length run.
(In reply to comment #28)
> Is run.NextRun() returning a zero-length run in this case? If that's the
> problem, I'd prefer we fixed this by ensuring NextRun() never returns
> zero-length run.

That's not the issue here: the crash occurs _before_ the while(run.NextRun()) loop, not within it, during the initialization of allowHyphenBreakBeforeNextChar.

It looks like we can avoid the issue, though, by checking that the fragment actually contains some text and not setting the 'hyphenating' flag if it's empty; then GetHyphenationBreaks won't be called at all in the case of an empty fragment, which is what's triggering the crash here.
It turned out comment #29 wasn't quite enough; Jesse's additional testcase in bug 645072 uncovered another case where the "lookback" in the initialization of allowHyphenBreakBeforeNextChar could trigger an assertion, even without an "empty" text fragment. The conditions that trigger it involve adding a new textNode that begins with &shy; after a whitespace-only textNode in a fieldset; because the iterator has been advanced past the initial whitespace, it ends up at the position beyond the end of the text of the whitespace-only node, and so it's not safe to look at the character there to see if it's SHY.

The simple fix is to check that the prevTrailingCharOffset position is valid for the text fragment, before reading the character at that position. With this, the additional crashtest no longer triggers an assertion.
Attachment #522184 - Attachment is obsolete: true
Attachment #522462 - Flags: review?(roc)
Attachment #522184 - Flags: review?(roc)
Why is mStart.GetOriginalOffset() + mLength > mFrag.Length() in this case? That seems wrong?
Yes, that puzzled me too, but I think I understand now...

The _following_ inline frame begins with a SHY character, which is skipped by the gfxSkipCharsIterator in AddInlineMinWidthForFlow. So when we do iter.ConvertSkippedToOriginal(flowEndInTextRun) to find the ending offset in the original text, the iterator returns an offset past the SHY, which means it's referring to a position _beyond_ the end of the current frame's text and into the following frame. Passing this to the PropertyProvider as the text length leads to the out-of-range assertion (or potential crash).

So instead of using the gfxSkipCharsIterator to figure out the original-text length that the PropertyProvider covers by converting back from flowEndInTextRun, it would be better to use frag->GetLength() - iter.GetOriginalOffset(). I think that should be the appropriate length, but will run some checks.... there are enough different kinds of offsets and indexing going on here that it's easy to get lost!
OK that makes sense. Then I think the correct fix here is to pass the correct length to the PropertyProvider.
Yes, now that I understand the lengths better, I think this is correct - it works (and passes the tests cleanly) locally, and I've pushed a tryserver job to double-check.
Attachment #522462 - Attachment is obsolete: true
Attachment #522636 - Flags: review?(roc)
Attachment #522462 - Flags: review?(roc)
Comment on attachment 522636 [details] [diff] [review]
patch, now with corrected length for PropertyProvider

+  NS_PRECONDITION(mFrag->GetLength() > 0,
+                  "Don't call GetHyphenationBreaks for empty text fragments")

I don't think this assertion is needed now.
Attachment #522636 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/058e4330278e - re-enabled tests
http://hg.mozilla.org/mozilla-central/rev/2f45c30741c5 - updated patch

(Also landed an additional crashtest for this in bug 645072.)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 646561
Depends on: 650499
You need to log in before you can comment on or make changes to this bug.