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

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

({regression, testcase})

Trunk
PowerPC
Mac OS X
regression, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

12 years ago
Created attachment 248537 [details]
testcase

Resize the browser window to see the tables shrink.
(Assignee)

Comment 2

12 years ago
Created attachment 248550 [details] [diff] [review]
patch

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

Comment 3

12 years ago
Created attachment 248551 [details] [diff] [review]
patch

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

Comment 4

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

Comment 5

12 years ago
Created attachment 248554 [details] [diff] [review]
patch v1.1

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

Comment 8

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

Updated

12 years ago
Attachment #248554 - Attachment is obsolete: true
Attachment #248554 - Flags: superreview?(dbaron)
Attachment #248554 - Flags: review?(smontagu)
(Assignee)

Comment 9

12 years ago
Created attachment 248833 [details] [diff] [review]
patch v1.2

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

Comment 10

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

Comment 12

12 years ago
(In reply to comment #11)

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

Submitted bug 364071.

Updated

12 years ago
Attachment #248833 - Flags: review?(smontagu) → review+
(Assignee)

Comment 13

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

Updated

12 years ago
Flags: in-testsuite?

Updated

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