Soft hyphen not working in tables.

RESOLVED FIXED in mozilla5

Status

()

Core
Layout: Text
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Ho KS, Assigned: jfkthame)

Tracking

({testcase})

Trunk
mozilla5
testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
wanted1.9 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 304933 [details]
Testcase

Updated

10 years ago
Component: General → Layout: Fonts and Text
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Version: unspecified → Trunk

Comment 2

10 years ago
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.

Comment 4

10 years ago
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+

Comment 7

10 years ago
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.

Comment 8

10 years ago
Created attachment 323037 [details]
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

10 years ago
Created attachment 353467 [details]
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.
OS: Windows XP → All
Hardware: PC → All

Comment 10

9 years ago
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).

Comment 11

7 years ago
Created attachment 516604 [details]
test case

Comment 12

7 years ago
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
(Assignee)

Comment 14

7 years ago
Created attachment 517369 [details] [diff] [review]
patch, handle &shy; 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. :)
Attachment #517369 - Flags: review?(roc)
(Assignee)

Comment 15

7 years ago
Created attachment 517370 [details] [diff] [review]
reftests for soft hyphen in table cells
Attachment #517370 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 20

7 years ago
(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.
(Assignee)

Comment 22

7 years ago
Created attachment 517710 [details] [diff] [review]
patch, handle &shy; 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.
Attachment #517369 - Attachment is obsolete: true
Attachment #517710 - Flags: review?(roc)
Attachment #517369 - Flags: review?(roc)
(Assignee)

Comment 23

7 years ago
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]

Comment 24

7 years ago
http://hg.mozilla.org/mozilla-central/rev/25beb9ced8d2
http://hg.mozilla.org/mozilla-central/rev/03070beac9e7
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
Depends on: 645072
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 .
(Assignee)

Comment 27

7 years ago
Created attachment 522184 [details] [diff] [review]
patch, handle &shy; 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.
Attachment #522184 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
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.
(Assignee)

Comment 29

7 years ago
(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.
(Assignee)

Comment 30

7 years ago
Created attachment 522462 [details] [diff] [review]
patch, handle &shy; 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 &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?
(Assignee)

Comment 32

7 years ago
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.
(Assignee)

Comment 34

7 years ago
Created attachment 522636 [details] [diff] [review]
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.
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+
(Assignee)

Comment 36

7 years ago
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
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 646561
(Assignee)

Updated

7 years ago
Depends on: 650499
You need to log in before you can comment on or make changes to this bug.