244117 - a table cell containing <script> tag spreads to the width of a table

Status: RESOLVED DUPLICATE of bug 234593

(Core :: Layout: Tables, defect)

Description

[Hideo Saito]

There are the following two problems which have a common cause.

1. a table cell containing <script> tag spreads to the width of a table.
2. a table cell containing <script> tag shrinks and causes some funny text
wrapping on the bonsai query page.

In the first case, when measuring the contents of the first cell, the contents
of the second cell were empty.

This problem was reported by Bugzilla-jp
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3700

actual testcase:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2157&action=view
simple testcase:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2214&action=view

In the second case, when measuring the contents of the first cell, the contents
of the first cell were short like the following texts.

Modify Query
Mail everyone on this page (12 people)

This problem was repored at Bug 232838.
http://bugzilla.mozilla.org/show_bug.cgi?id=232838#c20
Refer to following screen shot.
http://bugzilla.mozilla.org/attachment.cgi?id=143915&action=view

I think that this problem is regression of attachment 131205 [details] [diff] [review] of Bug 210269.

Comment 1

[Hideo Saito]

Attachment: test patch for a first cell spreads

Comment 2

[Hideo Saito]

Attachment: test patch for a first cell shrinks

Comment 3

[Boris Zbarsky [:bzbarsky]]

As long as we don't check that second patch in no matter what....  ;)

Comment 4

[Hideo Saito]

What I meant with the second patch is to prevent from flushing the notification
in the script processing by the next program in nsPresShell.cpp.

    // force flushing of any pending notifications
    mDocument->BeginUpdate(UPDATE_ALL);
    mDocument->EndUpdate(UPDATE_ALL);

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, well... that code should arguably be removed -- there's no reason for
presshell to flush content before reflow.

Comment 6

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

Currently, there is a reason: in some places we set attributes during reflow,
and if that causes a content flush which causes a reframe, deleting frames we're
in the middle of reflowing, we crash nastily. If we get rid of all attribute
setting during reflow, then we can not flush, but we're some ways away from that.

These are just table incremental reflow bugs, aren't they? We should be able to
cope with content flushing and reflow at any point during load.

Comment 7

[Bernd]

A reflow log and discussion what is going on might be a good idea. I don't think
that the current patch is correct.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't think Hideo Saito is claiming the patches are correct (hence "test
patch")...

Comment 9

[Hideo Saito]

Attachment: testcase for a first cell spreads

Comment 10

[Hideo Saito]

Attachment: testcase for a first cell shrinks

Comment 11

[Hideo Saito]

Attachment: Screen Shot

Comment 12

[Hideo Saito]

For the testcase of for a first cell shrinks, its structure is simplified as
follows. It seems that a <br> and an incremental reflow are related.

<b>text<br>text<script src=""></script>text</b>

Comment 13

[Hideo Saito]

Attachment: patch for a first cell spreads

The first cell width is set by an incremental reflow without |NeedStrategyInit|
is set to true by |nsTableRowFrame::AppendFrames| that is
|nsTableRowFrame::AppendFrames| is called after an incremental reflow. If the
NS_FRAME_IS_DIRTY bit of |mState| does not set, |resetComputedWidth| should be
set to true, however 
its pass2 is not desired.

This patch is added to the patch of attachment 150077 [details] [diff] [review] on Bug 237366.

Comment 14

[Bernd]

Won't that patch create a situation where a table cell will be reflown with an
unconstrained computed size but there is no guarantee that a second reflow will
be issued?

Comment 15

[Bernd]

When I remove the overflow hidden from attachment 150623 [details] the rendering changes
dramatically. This makes me wonder whether the patch is targetting the correct
place. Hideo can you provide a testcase without the overflow style? And as I
stated in comment 7 a reflow log discussion for this case would be good.

Comment 16

[Hideo Saito]

In the case of attachment 150623 [details], a scrollable frame is necessary, but its
overflow does not need hidden.

In the case of attachment 150623 [details], |nsFrameManager::AppendFrames| calls following
functions and an incremental reflow is proceeded. I think it is cause that
|resetComputedWidth| is false in first incremental reflow.

nsBlockFrame::AppendFrames
nsBlockFrame::AppendFrames
nsInlineFrame::AppendFrames
nsBlockFrame::AppendFrames
  nsTableFrame::Reflow, eReflowReason_Incremental, resetComputedWidth is false
nsBlockFrame::AppendFrames
nsTableRowFrame::AppendFrames
nsBlockFrame::AppendFrames
  nsTableFrame::Reflow, eReflowReason_Incremental, resetComputedWidth is true

If second <script> tag is suppressed, the first cell width does not spread.

nsBlockFrame::AppendFrames
nsBlockFrame::AppendFrames
nsInlineFrame::AppendFrames
nsBlockFrame::AppendFrames
nsTableRowFrame::AppendFrames
nsBlockFrame::AppendFrames
  nsTableFrame::Reflow, eReflowReason_Incremental, resetComputedWidth is true

Comment 17

[Bernd]

So one needs that overflow that creates a scrollable frame to reproduce the problem.
 resetComputedWidth is true either 
aTableFrame.NeedStrategyInit()
aTableFrame.NeedStrategyBalance
are true.
Are they true if no scrollable frame is involved? They usually become true if
the MEW of the cell content changes. Are you sure that the code does not suffer 
from the code path:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#894
which seems very wrong to me.
(With reflow log I mean
http://lxr.mozilla.org/seamonkey/source/layout/doc/frame_reflow_debug.html)

Comment 18

[Hideo Saito]

If its frame's overflow is visible, the first cell width does not spread,
because |nsBoxFrame::Reflow| is not called from |nsGfxScrollFrame::Reflow|
although resetComputedWidth is false.

Comment 19

[Hideo Saito]

Comment on attachment 150624 [details]
testcase  for a first cell shrinks

This problem had been already fixed on trunk by patch of Bug 238472, although I
checked it using a build from the source of released mozilla-1.7rc2.

Comment 20

[Bernd]

Hideo, does that mean the bug is wfm for you? If yes could you mark so.

Comment 21

[Hideo Saito]

I reported following two problems. The later problem(2.) is wfm, however the
former problem(1.) still remains.

> 1. a table cell containing <script> tag spreads to the width of a table.
> 2. a table cell containing <script> tag shrinks and causes some funny text
>    wrapping on the bonsai query page.

Comment 22

[Hideo Saito]

*** This bug has been marked as a duplicate of 234593 ***