Closed Bug 363732 Opened 14 years ago Closed 14 years ago

Table containing bidi, unbreakable text inside inline shrinks horizontally when window is resized

Categories

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

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: uriber, Assigned: uriber)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 3 obsolete files)

In the testcase I'll attach soon, resizing the window causes the tables to shrink to the width of the first run of text only, leaving the second (reverse-direction) run outside the table.

The setting is: <table><tr><td><span>BIDItext</span></td></tr></table>

This regressed with the reflow branch landing.
Attached file testcase
Resize the browser window to see the tables shrink.
Attached patch patch (obsolete) — Splinter Review
The answer to the question in the removed comment appears to be "no" :-)

Actually, I'm not sure this is the correct solution, as all other calls GetNextWord() do set wordLen in the bidi case, as far as I can see. Simon - do you remember why this is done?
Attachment #248550 - Flags: superreview?(dbaron)
Attachment #248550 - Flags: review?(smontagu)
Attached patch patch (obsolete) — Splinter Review
Sorry - I was back to my old habit of attaching the testcase instead of the patch.
Attachment #248550 - Attachment is obsolete: true
Attachment #248551 - Flags: superreview?(dbaron)
Attachment #248551 - Flags: review?(smontagu)
Attachment #248550 - Flags: superreview?(dbaron)
Attachment #248550 - Flags: review?(smontagu)
The answer to "why doesn't this happen upon the initial load" is that the min width in that case is calculated before the bidi frame splitting.

The answer to "why does this only happen when there is an enclosing inline" is that in the process of splitting the bidi inline container, nsInlineFrame::InsertFrames() is called, which ultimately results in a call to nsTableFrame::MarkIntrinsicWidthsDirty(). So when the window is resized, the min width is re-calculated (with the bidi splitting already there). For the plain text case, the value calculated before the split is used when resizing.
Attached patch patch v1.1 (obsolete) — Splinter Review
Leaving wordLen uninitialized was not a good idea.

I'm now more convinced that this is in fact the right solution, as it seems that the "passing the frame text length as input" trick was introduced (in bug 36163) for cases where the caller is not interested in looking into continuation frames (see bug 36163 comment 26, §2). This does not seem to be the case here.
Assignee: nobody → uriber
Attachment #248551 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #248554 - Flags: superreview?(dbaron)
Attachment #248554 - Flags: review?(smontagu)
Attachment #248551 - Flags: superreview?(dbaron)
Attachment #248551 - Flags: review?(smontagu)
Comment on attachment 248554 [details] [diff] [review]
patch v1.1

If there is no longer any special-casing for Bidi, you can remove the #ifdef IBMBIDI. Do you want to make the same change at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.604&mark=5805-5808#5798  ?
So the bidi changes here changed aWordLenResult from an out-parameter to GetNextWord to an in-out parameter where the in and the out have different semantics.  That was a horribly confusing change.

Could you explain what this change actually means?  And why you don't need to make the same change to AddInlinePrefWidth?  (Or do you?  I suspect so.)  And maybe even what the parameter actually means these days?

Will this change work correctly both before and after splitting have happened?  It needs to.
(In reply to comment #6)
> (From update of attachment 248554 [details] [diff] [review] [edit])
> If there is no longer any special-casing for Bidi, you can remove the #ifdef
> IBMBIDI.

Technically I shouldn't, because wordLen doesn't need initializing at all in the non-IBMBIDI world. See e.g. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/base/src/nsTextTransformer.cpp&mark=1743-1745#1742
But I'll get rid of the #IFDEFs anyway, if you think it looks better.

> Do you want to make the same change at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.604&mark=5805-5808#5798
>  ?

Yes, I probably do. I'll attach a revised patch early next week.

(In reply to comment #7)
> So the bidi changes here changed aWordLenResult from an out-parameter to
> GetNextWord to an in-out parameter where the in and the out have different
> semantics.  That was a horribly confusing change.
> 

If by "here" you mean "in bug 36163", then yes, and I agree.

> Could you explain what this change actually means?

It means that GetNextWord() will consider the entire text content associated with the frame when looking for the end of the next word, and won't stop looking at the offset corresponding to the end of the current frame. 

> And why you don't need to
> make the same change to AddInlinePrefWidth?  (Or do you?  I suspect so.)

Yes, I do. (see above).

> And maybe even what the parameter actually means these days?

I'm not sure I understand this question, but I'll try to answer anyway:
If the parameter is negative on input, then the entire content element is considered when looking for the next word. Otherwise, only the prefix of length  *aWordLen is considered.

> Will this change work correctly both before and after splitting have happened? 
> It needs to.

Yes, it will. Before the splitting it won't matter, because the entire content is contained in one frame (so passing -1 or the length of text contained in the frame would yield the same results). After the splitting, this change would fix the problem, as described above.
Attachment #248554 - Attachment is obsolete: true
Attachment #248554 - Flags: superreview?(dbaron)
Attachment #248554 - Flags: review?(smontagu)
Attached patch patch v1.2Splinter Review
This gets rid of the #ifdefs, and applies the change to nsTextFrame::AddInlinePrefWidth() as well.

David, should I file a separate bug on fixing the nsTextTransformer API so that wordLen will go back to being an output-only argument?
Attachment #248833 - Flags: superreview?(dbaron)
Attachment #248833 - Flags: review?(smontagu)
(In reply to comment #8)
> > Could you explain what this change actually means?
> 
> It means that GetNextWord() will consider the entire text content associated
> with the frame when looking for the end of the next word, and won't stop
> looking at the offset corresponding to the end of the current frame. 
> 

To elaborate: this is necessary because nsContinuingTextFrame::AddInline[Min|Pref]Width() "do nothing, since the first-in-flow accounts for everything."
A possible alternative solution would have been to have each continuation frame deal with its own content - but it seems like you chose not to do it that way.
Comment on attachment 248833 [details] [diff] [review]
patch v1.2

sr=dbaron.

It probably is worth having a bug on that API.
Attachment #248833 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #11)

> It probably is worth having a bug on that API.
> 

Submitted bug 364071.
Attachment #248833 - Flags: review?(smontagu) → review+
Checked in:

Checking in mozilla/layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.605; previous revision: 1.604
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.