418975 - Soft hyphen not working in tables.

Status: RESOLVED FIXED

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

Description

[Ho KS]

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

Comment 1

[José Jeria]

Attachment: Testcase

Comment 2

[José Jeria]

Is there a certain reason why this doesn't work with display table-cell?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[cliffspradlin]

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 ­ 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).

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

okay, I guess consistency is good.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[richard]

I agree with Cliff #4: Inside tables, a ­ is often used to prevent that the table grows too wide. It's frustrating to see that this doesn't work with FF.

Comment 8

[Mathias Nater]

Attachment: How soft hyphenation does and does not work in tables.

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!

Comment 9

[Dominykas Blyžė]

Attachment: A test for fieldset element with soft hyphens

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.

Comment 10

[Arman]

Note that the ­ 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 ­ character is only deleted and a break is generated.

(Tested with Firefox 3.0.5).

Comment 11

[Alexey]

Attachment: test case

Comment 12

[Alexey]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This would be a fun introduction to intrinsic-min-width computation for Jonathan :-)

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: patch, handle ­ in nsTextFrame::AddInlineMinWidthForFlow so that it works in table cells

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. :)

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: reftests for soft hyphen in table cells

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 20

[Jonathan Kew [:jfkthame]]

(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.)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we provide that length only in the hyphens case? There's a bit of overhead in converting the offsets.

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: patch, handle ­ in nsTextFrame::AddInlineMinWidthForFlow, updated for review comments

This is modified so that it only computes the length to pass to the PropertyProvider if hyphenation is actually present.

Comment 23

[Jonathan Kew [:jfkthame]]

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)

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/25beb9ced8d2
http://hg.mozilla.org/mozilla-central/rev/03070beac9e7

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So when relanding, you should (a) land the revised patch and (b) back out https://hg.mozilla.org/mozilla-central/rev/f1d26af4c57b .

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: patch, handle ­ in nsTextFrame::AddInlineMinWidthForFlow - updated to fix crasher

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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[Jonathan Kew [:jfkthame]]

(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.

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: patch, handle ­ in nsTextFrame::AddInlineMinWidthForFlow - updated to fix potential crashes/assertions

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 ­ 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.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why is mStart.GetOriginalOffset() + mLength > mFrag.Length() in this case? That seems wrong?

Comment 32

[Jonathan Kew [:jfkthame]]

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!

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK that makes sense. Then I think the correct fix here is to pass the correct length to the PropertyProvider.

Comment 34

[Jonathan Kew [:jfkthame]]

Attachment: patch, now with corrected length for 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.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 36

[Jonathan Kew [:jfkthame]]

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.)