172896 - remove NS_BLOCK_WRAP_SIZE

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P1)

Description

[Bernd]

see attached testcase and the debug log. 

especially the last lines 
    text 026D3A84 d=3030,285 me=3030 
   caption 026D3BB0 d=3480,1200 me=1788 
where the strange MES reduction takes place.

VP 026CF040 d=9180,4470 
VP 026CF040 r=1 a=9180,4470 c=9180,4470 cnt=851 
 block 026D3490 r=1 a=9180,UC c=8940,UC cnt=856 
  text 026D3678 r=0 a=8940,UC c=UC,UC cnt=857 
  text 026D3678 d=0,0 
  tblO 026D37C8 r=0 a=8940,UC c=0,0 cnt=858 
   caption 026D3BB0 r=0 a=UC,UC c=UC,UC cnt=859 
    text 026D3A84 r=0 a=UC,UC c=UC,UC cnt=860 
    text 026D3A84 d=3030,285 me=3030 
   caption 026D3BB0 d=3930,1200 me=3930 
   tbl 026D3994 r=0 a=8940,UC c=UC,UC cnt=861 
    rowG 026D3D54 r=0 a=UC,UC c=UC,UC cnt=862 
     row 026D3EE0 r=0 a=UC,UC c=UC,UC cnt=863 
      cell 026D40B4 r=0 a=UC,UC c=UC,UC cnt=864 
       block 026D4114 r=0 a=UC,UC c=UC,UC cnt=865 
        text 026D4254 r=0 a=UC,UC c=UC,UC cnt=866 
        text 026D4254 d=795,285 me=435 
       block 026D4114 d=795,300 me=435 
      cell 026D40B4 d=825,330 me=465 
     row 026D3EE0 d=UC,330 
    rowG 026D3D54 d=UC,330 
    colG 026D4380 r=0 a=UC,UC c=UC,UC cnt=867 
     col 026D44A8 r=0 a=0,0 c=UC,UC cnt=868 
     col 026D44A8 d=0,0 
    colG 026D4380 d=0,0 
    rowG 026D3D54 r=2 a=885,UC c=885,UC cnt=869 
     row 026D3EE0 r=2 a=885,UC c=885,UC cnt=870 
      cell 026D40B4 r=2 a=825,UC c=795,UC cnt=871 
       block 026D4114 r=2 a=795,UC c=795,UC cnt=872 
       block 026D4114 d=795,300 
      cell 026D40B4 d=825,330 
     row 026D3EE0 d=885,330 
    rowG 026D3D54 d=885,330 
    colG 026D4380 r=2 a=885,UC c=885,UC cnt=873 
     col 026D44A8 r=0 a=0,0 c=UC,UC cnt=874 
     col 026D44A8 d=0,0 
    colG 026D4380 d=0,0 
   tbl 026D3994 d=975,480 
   caption 026D3BB0 r=0 a=3930,UC c=UC,UC cnt=875 
    text 026D3A84 r=2 a=3030,UC c=UC,UC cnt=876 
    text 026D3A84 d=3030,285 
   caption 026D3BB0 d=3930,1200 
  tblO 026D37C8 d=3930,1680 
  text 026D4500 r=0 a=8940,UC c=UC,UC cnt=877 
  text 026D4500 d=0,0 
 block 026D3490 d=8940,1680 
VP 026CF040 d=9180,4470 
VP 026CF040 r=1 a=9180,4470 c=9180,4470 cnt=878 
 block 026D3490 r=1 a=9180,UC c=8940,UC cnt=883 
  tblO 026D37C8 r=1 a=8940,UC c=0,0 cnt=884 
   caption 026D3BB0 r=1 a=3930,UC c=888,UC cnt=885 
VALUE 888 is not a whole pixel 
    text 026D3A84 r=3 a=888,UC c=UC,UC cnt=886 
VALUE 888 is not a whole pixel 
    text 026D3A84 d=3030,285 me=3030 
   caption 026D3BB0 d=3480,1200 me=1788 
VALUE 1788 is not a whole pixel 
  tblO 026D37C8 d=3480,1680 
 block 026D3490 d=8940,1680 
VP 026CF040 d=9180,4470

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks perfectly fine to me.  It's returning the right MES the first time, and
then when the DOM sets style="width: 20%", it's returning that width.  The text
is supposed to overflow.

Comment 3

[Bernd]

Attachment: patch

Comment 4

[Bernd]

>The text is supposed to overflow.
NO, it is not, because the NS_BLOCK_WRAP_SIZE flag is set. And this means the
block can't shrink below the MES of the text inside.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I disagree.  Strongly.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why in the world do we have an incorrect answer this close to being the final
result?

Comment 7

[Bernd]

David, due to my limited knowledge of english I don't get the complete
understanding what you mean.
>I disagree.  Strongly.
I understand that you disagree, I dont understand, why. 
1) If we have a NS_BLOCK_WRAP_SIZE set should the text overflow the box? 
2) If the box is not allowed to be shrinked below the text-MES why should we not
report a MES back that includes the text-MES and borderpadding?
>Why in the world do we have an incorrect answer this close to being the final
result?
3) Do you mean the patch fixes at the wrong place, this can easily be true I
would not argue against, it was more intended as a demonstration what looks
wrong to me. 
4)But this is block and line layout and you are the expert. The only thing I see
as a consumer of the block code is that it reports back wrong sizes.
Or my understanding of the meanings of the flags is wrong (remember my
missunderstanding of unit_null). 

I gathered my knowledge from
http://www.mozilla.org/newlayout/doc/block-and-line.html

NS_BLOCK_WRAP_SIZE

    When this flag is set, the frame is supposed to ``wrap around'' all its
in-flow children. This means that we may need to enlarge it to fit around any
frames that stick outside its box after ... ?

    This is set by default for select frames, area frames, and table cell inner
frames when the frame is created.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A width is a width.  It can't be expanded.  In other words, I think
NS_BLOCK_WRAP_SIZE shouldn't exist as something separate from
NS_BLOCK_SHRINK_WRAP.  (Can you find the reason they were added as separate
things?  Are there testcases on the bug?)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(My suspicion was that it was added because, in a case in the block code where
we are correct and MSIE is buggy, someone decided that blocks inside tables
should be buggy like MSIE (for lack of reading/understanding the standards,
perhaps?) so we had to have a separate way of handling blocks inside of tables.)

Comment 10

[Bernd]

Attachment: block wrap history

Comment 11

[Bernd]

The code at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#1500
is indeed evil, removing the stuff fixes bug 46983 .

Comment 12

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

Agreed, it is evil. We must change it to not rely on the overflow area. It may
be as simple as ditching the X case and fixing the Y case to just include the
floater  bottom edge.

Comment 13

[Bernd]

I think I would prefer to see the cases that actually rely on this behaviour by
placing a assertion and running the regression tests. My plan would be to remove
the NS_BLOCK_WRAP_SIZE from the inner table cell frame and caption frame. Once
this is done successfully I would leave it for David to kill this stuff entirely.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Retitling bug to reflect what I think it now means.  Retitle again if you
disagree.  (Changing from "nsBlockFrame::ComputeFinalSize returns wrong MES" to
"remove NS_BLOCK_WRAP_SIZE".)

Comment 15

[Bernd]

Attachment: list of testcases that invoke NS_BLOCK_WRAP_SIZE

Comment 16

[Bernd]

Attachment: testcase that uses NS_BLOCK_WRAP_SIZE

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to remove NS_BLOCK_WRAP_SIZE implementation

This removes the implementation of NS_BLOCK_WRAP_SIZE and replaces it with what
I think should be there.  I haven't tested it, and I can't find where Bernd
pointed out a testcase that broke with the more simple removal...

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase this causes problems with

The patch causes problems with this testcase (simplified from
layout/html/tests/table/bugs/bug4520.html).  It also seems to cause a very
slight reduction in table height in certain cases, although the only visible
changes involve HRs at the end of table cells.	I'm not quite sure I understand
that and there's a possibility it could be some form of common noise on the
regression tests.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problems may be related to the case where |okToAddRectRgn| is false in
nsBlockReflowState::FlowAndPlaceFloater.  I think it would be nice if that did
the x-offset calculation more the way nsBlockReflowContext::ReflowBlock does it
for what seems at first glance to be the same thing.  (Why do we need both
pieces of code?  What's the difference?)  That is, attachment 99893 [details] [diff] [review] might help
(see bug 159363).

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(This would be so much easier if we didn't attempt to do min-width, pref-width,
and final layout in the same functions.)

Comment 21

[karnaze (gone)]

*** Bug 145141 has been marked as a duplicate of this bug. ***

Comment 22

[Brendan Eich [:brendan]]

Re: comment #20, why don't we separate those computations?  Is there a bug for
that design flaw fix?

/be

Comment 23

[Jaime Rodriguez, Jr.]

carrying over keyword foo and url from bug 145141.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Re comment 22:  When I refer to something such as "cleaning up reflow", that's
what I mean.  It's probably a multiple people on a branch for a few weeks/months
type of job unless we can find a way to do it incrementally, and either way we'd
need to have a good bit of design discussion beforehand.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect what I need to fix is the right floater code in
nsBlockFrame::PlaceLine (perhaps in addition to what I mentioned in comment 19).

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: regression test results for attachment 109297

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, there seem to be two problems:

 1) in ComputeFinalSize, the code that adds in the bottom padding to the
autoHeight is before the code that accounts for floats.  Switching them helps
many of the problems above.

 2) This patch makes it so right floats aren't accounted for in the height
calculation for one of the many shrinkwrapping concepts.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This patch is only failing one test that I don't think it should be failing. 
(I modified it a bit since I last ran the tests to avoid some asserts on the
failing test, but I don't think those modifications should have changed the
behavior on the other tests.)  The one test is:

http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug7113.html


and the problem only occurs on the first layout (not reflows caused by text
zoom).

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, alternative #1

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, alternative #2

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regression test changes are the following:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/28811.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug131020.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug131020-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug137388-1.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug137388-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug137388-3.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug4576.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug6674.html
  Inner table cell frame (block) narrower, but the table and the content
  of the cell are the same size.  (Case of horizontal overflow from
  cell.)  In other words, no visible changes.
 
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/81776.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug16252.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug17548.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug33137.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug60992.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/printing/bug138349-1.html
  Table cell containing HR is slightly shorter (less space below HR, due
  to weird vertical alignment on generated content "\n").

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/87543.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/169620.htmlhttp://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug1055-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug12012.xul
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug32205-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug83786.html
  Table supposedly slightly shorter, but nothing visible.

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug7113.html
  Slight increase in table width on initial load, fixing an incremental
  reflow bug.

*http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/printing/bug92215.html
  Alternative patch #1 breaks this one, by causing the lower float to
  have its left edge adjacent with the right edge of the upper float,
  instead of its right edge.
  (I forgot to check if the problem goes away on text zoom.)

*http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/printing/163614.html
  Alternative patch #2 breaks this one (and causes an incremental reflow
  bug), by making the table wider (since at large widths the image near
  the bottom could overlap the floating images).


I prefer alternative patch #2 since it should make right floats affect things
the same way left floats do.  Each alternative causes one regression in the
regression tests.  I'm not too concerned about taking either since I'm hoping we
can rewrite all the min/max width stuff soon anyway (so that it doesn't get
computed within |Reflow|).

Comment 33

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

Comment on attachment 109592 [details] [diff] [review]
patch, alternative #2

r+sr=roc+moz

I really like this approach to handling right floaters. The old approach always
made me queasy. "infinity - x" ... ugh.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2002-12-18 16:11/12 PDT.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 111720 has been marked as a duplicate of this bug. ***