32199 - (XUL only) visibility:collapse is not being interpreted as hidden on block and inline level elements

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P3)

Description

[Hixie (not reading bugmail)]

The 'visibility' property's value 'collapse' is not being interpreted 
as 'hidden' when applied to elements that are not rows, row groups, columns,
or column groups.

Test cases:
  Most 'display' types: http://www.bath.ac.uk/~py8ieh/m/visible-collapse.html
  'block':  http://www.bath.ac.uk/~py8ieh/m/visible-collapse-blocks.html
  'inline':  http://www.bath.ac.uk/~py8ieh/m/visible-collapse-inlines.html
  'cell':  http://www.bath.ac.uk/~py8ieh/m/visible-collapse-cells.html
  'table':  http://www.bath.ac.uk/~py8ieh/m/visible-collapse-tables.html

Comment 1

[Pierre Saslawsky]

It's mostly fixed. The page at the URL above now works correctly except for cell 
& table. The remaining problems are dups of bug 21701: we should review the use 
of display->IsVisibleOrCollapsed() and in some cases change it to display->
IsVisible() in the following files:
    nsTableCellFrame
    nsTableColFrame
    nsTableColGroupFrame
    nsTableFrame
    nsTableRowFrame
    nsTableRowGroupFrame

See bug 21701 for more info. I'm not closing the present bug yet. I prefer to 
reassign it to karnaze: it will allow him to work on the files above while buster 
takes care of the other ones.

Comment 2

[Bernd Mielke]

Adding testcase keyword so this doesn't show up on the bugathon search page.

Comment 3

[ekrock's old account (dead)]

Changing testcase to donttest. Bernd: FYI, this is the "correct" way to keep a 
bug off the BugAThon radar if no test case is needed. (See the Keywords 
definition list for more info.) But you had the right idea--thanks for your 
help!

Comment 4

[Marc Attinasi]

Denying beta3 approval.

Comment 5

[Hixie (not reading bugmail)]

Taking QA per managerial policy.

Comment 6

[Bernd]

Ian, your testcases are not visible could you attach them to the bug

Comment 7

[Hixie (not reading bugmail)]

(test cases are back, it was a transient server problem)

Comment 8

[Greg K.]

So is the fact that I see red-bordered blocks in the cell and table testcases
evidence of this bug? The blocks are otherwise empty.

Comment 9

[Hixie (not reading bugmail)]

Yes, red = bad in my tests.

Comment 10

[Greg K.]

Okay, thanks; just checking. Reconfirmed using FizzillaCFM/2002071208. Setting
All/All.

Comment 11

[Eric Murphy]

I just want to add that I am having this problem in XUL too. Also, on some XUL
code I have, CPU goes to 100% when I try to set a block of code to hidden. I
can't make a small testcase at the moment :-(

Comment 12

[Bernd]

taking the bug

Comment 13

[Bernd]

Attachment: patch

Comment 14

[Bernd]

the patch has also some CallQueryInterface cleanup, from a quick search
http://lxr.mozilla.org/seamonkey/search?string=IsVisibleOrCollapsed I believe a
lot if not all callers of this function are wrong.

Comment 15

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

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

Other than these two changes, sr=dbaron.  (I don't really object to the
commented code, either, although it would be good to say why it's there.  But
if you want me to sr the other bit, you'd better explain what it's fixing.  I'm
assuming these were just included by accident.)

>@@ -2212,6 +2212,8 @@
>       SetResizeReflow(PR_TRUE);
>     }
>   }
>+  //aDoCollapse = PR_TRUE;
>+  
>   return rv;
> }
> 
>@@ -3310,7 +3312,6 @@
>         if (NS_FRAME_IS_COMPLETE(aStatus) && isPaginated && 
>             (NS_UNCONSTRAINEDSIZE != kidReflowState.availableHeight)) {
>           nsIFrame* nextKid = (childX + 1 < numRowGroups) ? (nsIFrame*)rowGroups.ElementAt(childX + 1) : nsnull;
>-          pageBreak = PageBreakAfter(*kidFrame, nextKid);
>         }
>         // Place the child
>         PlaceChild(aPresContext, aReflowState, kidFrame, desiredSize);

Comment 16

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

I don't even see why we should have an |IsVisibleOrCollapsed| method.  Feel free
to reassign this to me after you land the patch if you want me to finish
cleaning it up.

Comment 17

[Bernd]

David, you are so right, this is cruft. I feel save enough to fix the layout
callers too, but xul is beyond my scope   

Comment 18

[karnaze (gone)]

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

Please remove the following from your patch, otherwise it will break
page-break-after

-	   pageBreak = PageBreakAfter(*kidFrame, nextKid);

Comment 19

[Bernd]

Attachment: xul testcase

Comment 20

[Bernd]

Attachment: html testcase

Comment 21

[Bernd]

I checked the patch in. I could not create a testcase  proving that the
remaining occurences are really causing trouble. Before I damage something that
I dont understand I give this bug back.

Comment 22

[Pascal Chevrel:pascalc]

The bug doesn't show for me with the HTML testcase, only the XUL testcase.

Comment 23

[Pascal Chevrel:pascalc]

The bug doesn't show for me with the HTML testcase, only the XUL testcase (build
2003060308 WinXP)

Comment 24

[Martijn Wargers (dead)]

Attachment: testcase3

Also the flash plugin still shows up in this case.

Comment 25

[Gregory Pappas [:gregp]]

Is this still relevant with flex emulation?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

not really