21701 - [BLOCK] misuse of style attr mVisible

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[rods (gone)]

In the following places (see below) in layout we are misusing style
Diplay's attr mVisible, it should always be checked against the flags:

// See nsStyleDisplay
#define NS_STYLE_VISIBILITY_HIDDEN              0
#define NS_STYLE_VISIBILITY_VISIBLE             1
#define NS_STYLE_VISIBILITY_COLLAPSE            2

Instead of:
if (display->mVisible) {
.....

I have made all the changes to the form controls and to nsScrollFrame.cpp where
it was being misused. Please fix any that are yours and pass this bug along.

--------------------------
S:\mozilla\layout\html\base\src\nsBlockFrame.cpp(5423):  if (disp->mVisible &&
(NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) &&
S:\mozilla\layout\html\base\src\nsBulletFrame.cpp(123):  if (disp->mVisible) {
S:\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp(569):    if
(display->mVisible) {
S:\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp(636):    if
(display->mVisible) {
S:\mozilla\layout\html\base\src\nsHRFrame.cpp(102):  if (!disp->mVisible) {
S:\mozilla\layout\html\base\src\nsHTMLContainerFrame.cpp(67):    if
(disp->mVisible && mRect.width && mRect.height) {
S:\mozilla\layout\html\base\src\nsImageFrame.cpp(486):  if (disp->mVisible &&
mRect.width && mRect.height) {
S:\mozilla\layout\html\base\src\nsLeafFrame.cpp(45):    if (disp->mVisible) {
S:\mozilla\layout\html\base\src\nsObjectFrame.cpp(1185):  if ((disp != nsnull)
&& !disp->mVisible)
S:\mozilla\layout\html\base\src\nsScrollPortFrame.cpp(257):
view->Init(viewManager, mRect, parentView, nsnull, display->mVisible ?
S:\mozilla\layout\html\base\src\nsScrollPortFrame.cpp(477):    if
(display->mVisible) {
S:\mozilla\layout\html\base\src\nsTextFrame.cpp(803):  if (disp->mVisible) {

S:\mozilla\layout\html\table\src\nsTableCellFrame.cpp(223):    if
(disp->mVisible) {
S:\mozilla\layout\html\table\src\nsTableColFrame.cpp(115):      if
(disp->mVisible) {
S:\mozilla\layout\html\table\src\nsTableColGroupFrame.cpp(444):      if
(disp->mVisible) {
S:\mozilla\layout\html\table\src\nsTableFrame.cpp(1196):    if (disp->mVisible)
{
S:\mozilla\layout\html\table\src\nsTableRowFrame.cpp(351):      if
(disp->mVisible) {
S:\mozilla\layout\html\table\src\nsTableRowGroupFrame.cpp(204):      if
(disp->mVisible) {
S:\mozilla\layout\xul\base\src\nsBoxFrame.cpp(1747):  if (!disp->mVisible)
S:\mozilla\layout\xul\base\src\nsColorPickerFrame.cpp(157):  if
(!disp->mVisible)
S:\mozilla\layout\xul\base\src\nsDeckFrame.cpp(176):	if (!disp->mVisible)
S:\mozilla\layout\xul\base\src\nsDeckFrame.cpp(180):    if (disp->mVisible &&
mRect.width && mRect.height) {
S:\mozilla\layout\xul\base\src\nsProgressMeterFrame.cpp(373):  if
(!disp->mVisible)
S:\mozilla\layout\xul\base\src\nsSliderFrame.cpp(260):    if (disp->mVisible) {
S:\mozilla\layout\xul\base\src\nsTitledButtonFrame.cpp(460):	if
(!disp->mVisible)
S:\mozilla\layout\xul\base\src\nsToolboxFrame.cpp(279):  if (!disp->mVisible)

Comment 1

[rods (gone)]

added everyone to the CC list

Comment 2

[rods (gone)]

fixed charlie's e-mail

Comment 3

[David Hyatt]

Boxes rightfully support collapse, and it means (more or less) the same thing as
it does in tables, with the exception that the other elements can fill in the
space left over when another element in the box is collapsed.  Any XUL widget
derived from box (menus, tabs, decks, toolbars, toolboxes, etc.) also supports
collapse.

Comment 4

[rods (gone)]

It's not a question of supporting it, it's that the lines of code I have shown
in the bug are incorrectly processing the mVisible attr.

Comment 5

[leger]

Updating to default Layout Assignee...kipp no longer with us :-(

Comment 6

[troy]

Why are you re-reassing layout bugs? Do NOT touch layout bugs.

The bugs are assigned to Kipp so they can stay neatly organized until we have a
new owner for the block/inline code.

Comment 7

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

I think this was basically the same as bug 9691.  This bug has lots more info
now, though.

Comment 8

[troy]

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

Comment 9

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 10

[buster]

mine! mine mine mine!  all mine!  whoo-hoo!

Comment 11

[Pierre Saslawsky]

In order to fix most of bug 32199, I did some cleanup that should make the 
present bug fairly easy to fix. Two functions have been added to nsStyleDisplay: 
IsVisible() and IsVisibleOrCollapsed(). I used them everywhere it made sense in 
lieu of 'mVisible'.

In the following files, I replaced "if (disp->mVisible)" with "if (disp->
IsVisible())" as a BUG FIX for bug 32199 (see the testcase there for more info):
    nsBlockFrame
    nsBulletFrame
    nsHRFrame
    nsHTMLContainerFrame
    nsImageFrame
    nsTextFrame

In the following files, I replaced "if (disp->mVisible)" with "if (disp->
IsVisibleOrCollapsed())". It's merely a SYNTACTICAL CHANGE but it will make it 
easier for the respective owners to check each instance of IsVisibleOrCollapsed() 
and replace it IsVisible() if necessary. That's only 10 files to check and there 
is no more than 1 or 2 instances per file:
    nsBoxFrame
    nsColorPickerFrame
    nsDeckFrame
    nsLeafFrame
    nsObjectFrame
    nsScrollPortFrame
    nsSliderFrame
    nsTitledButtonFrame
    nsToolboxFrame
    nsXULTextFrame

Chris Karnaze will do the same thing in the following files. This is covered by 
bug 32199:
    nsTableCellFrame
    nsTableColFrame
    nsTableColGroupFrame
    nsTableFrame
    nsTableRowFrame
    nsTableRowGroupFrame

I touched some other files to use displ->IsVisible() but these were syntactical 
changes only with no further action required.

Comment 12

[rickg]

moving all buster m15 bugs to m16.

Comment 13

[buster]

this is a quick kill.  I'll look at this soon.

Comment 14

[buster]

I believe Pierre fixed all remaining problems for block and inline, 
based on his comment on 2000-03-17 02:45.  I've reviewed the code change and 
looked at all the relevant test cases on bug 32199.

Comment 15

[Chris Petersen]

Marking  verified per last comments.