222846 - dynamic reflow targetting at table and caption is broken

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

see the attached testcase. Btw Opera 7.11 does render both table identical with
large fonts.

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[Bernd]

the style resolution is correctly done, only the caption is not a reflown, and
as a consequence not painted. Moving another window over the caption shows that
the font size info did change for the caption. But it did not get a reflow.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This happens because we target an incremental style-changed reflow at the table
outer frame.  This goes through nsTableOuterFrame::Reflow and
nsTableOuterFrame::IncrementalReflow, into nsTableOuterFrame::IR_TargetIsMe. 
This function has a switch on the reflow type and the important part for this
bug is:

  case eReflowType_StyleChanged :    
    rv = IR_InnerTableReflow(aPresContext, aDesiredSize, aReflowState, aStatus);
    break;

Oops.  IR_InnerTableReflow never reflows the caption unless the inner table
frame's width changes.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133640 [details] [diff] [review]
Something like this?

Well, it does fix the bug... ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Taking, I guess.

Comment 7

[Bernd]

Boris, if you remove the following from the patch then r=bernd.
+                    priorInnerSize.height != innerMet.height;

I dont think this is nessary. The comment above the if (// if there is a caption
and the width or height of the inner table changed 
   // from a reflow, then reflow or move the caption as needed) can also be read
that  if these size changes take the nessary action: width- > reflow, height ->
repositioning. 

The side effect of this line is that every adding of rows which happens during
incr. loading will cause a reflow of the caption frame. 

You can of course convince me with a testcase that we need that statement :-).

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, after thinking about it I have failed to produce a testcase what would
require reflow of caption on height change.

Patch checked in with that line removed, and testcase checked in as well

Comment 9

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031021 BuildID 2003102104

Some observations:

the top box resizes, the top caption not.
When I switch to another tab, and back, the top caption has same size as the
other one, but still starts at the same point, so its length grows at the right
side.

For testing redraw, I opened BookmarksManager and reduced its size to the bare
minimum, about an inch wide and half an inch high.

If I shift BM over the top caption upward, downward, from the right or from the
left, only the part which was hidden gets redrawn, and on redraw resized, but
not repositioned, i.e. if I cover the small 'caption' only, only a big 'cap'
gets drawn, the whitespace thereafter stays untouched. 
If I cover this whitespace only, nothing gets redrawn, if I cover the small
ending -ion and the whitespace thereafter, -ption gets drawn big sized.

I can erase the pixels in the newly gained length, that means, 'cap' is always
restored, pixels of 'tion' can be erased.

I´ve used this method also in the testcase of win98-specific? Bug 220576,
http://bugzilla.mozilla.org/attachment.cgi?id=132519&action=view
URL: http://www.mozilla.org/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Sounds like yur build did not get this patch -- those are exactly the symptoms
of this bug....

Comment 11

[Hermann Schwab]

I was assuming that a patch checked in 10/20/2003 21:30	would be in BuildID
2003102104 but I´ll recheck tomorrow. I was reading tinderbox yesterday, and
looked at the testcase, but didn´t understand enough of the code to decide if
the position would also be adjusted by the reflow. As it is now, the most
left/down point isn´t changed, the height grows upward, and the width grows to
the right.

Comment 12

[Hermann Schwab]

Attachment: 7 kb screenshot showing bug

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031023

A 6250 bytes screenshot telling more than a lenghty description

Comment 13

[Hermann Schwab]

I want to reopen this bug as to me it seems not to be fixed.
Patch checked in 10/20/2003 21:30, todays nightly BuildID 2003102304

The cell in the top box is resized and repositioned by JS, but not the caption
of box1. Do a reload, and you´ll see. Then do another reload and wait for the
box to get resized. Switch to another tab, and back, and the caption is resized
too, but not moved. I´ve resized Bookmarks Manager to the minimum you see in the
screenshot, and then hovered around over the caption.
If you hover over the small caption, the parts of it touched get big.
If you hover over the big caption, you can erase the part that was newly added
horizontally, i.e. CAP can´t be erased, but TION can be erased. 

I just tested with Firebird 20031002, and it is showing the same behaviour.
Box gets resized by JS, caption not.
Could it be that this fix does nothing on WIN98 and Win98SE ?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Reopening... it's not fixed in a trunk build....

For some reason, the reflow target is not the outer frame but the inner one. 
Any ideas why?

Comment 15

[Bernd]

Attachment: issue a style reflow for the caption as well.

Comment 16

[Bernd]

Attachment: patch

The patch issues a style change reflow when the inner table frame is the target
of on incr. style change reflow. While Boris patch has been the right thing to
do, this is a hack around the bizzare style context relationship between the
inner and the outer table frame.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134108 [details] [diff] [review]
patch

Yeah, this is more like what I was thinking of....

>+  // see if we need to reflow the caption in addition
>+  if (aNeedToReflowCaption) {
>+    if (!*aNeedToReflowCaption && mInnerTableFrame == aChildFrame) {

if (aNeedToReflowCaption && !*aNeedToReflowCaption && 
    mInnerTableFrame == aChildFrame)

>+        if (eReflowType_StyleChanged == type) {
>+          *aNeedToReflowCaption = PR_TRUE;
>+        }

*aNeedToReflowCaption = eReflowType_StyleChanged == type;

(since you know *aNeedToReflowCaption is false if you got down to that code.

>+  if((eReflowReason_StyleChange != reflowReason) && reflowCaption) {

Add space after "if" and before "(".  Remove extra parens around first
comparison.

r+sr=bzbarsky with those nits picked.

Comment 18

[Boris Zbarsky [:bzbarsky]]

To bernd, since he has the patch.

Comment 19

[Bernd]

fix checked in