92868 - Hiding/Removing tables/elements from the flow using CSS doesn't work

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Sandip Chokshi]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    20010718

Trying to programitically hide and then remove a table from the flow causing 
all adjacent elements to reflow.  On IE, this can be accomplished using css y 
manipulating the visibility and position attributes.  This doesn't work on NS 
6.  I also tried to use the display attribute, but that didn't work as well.

Reproducible: Always
Steps to Reproduce:
1. Click on the above link
2. Follow the instructions on the page

Actual Results:  Hide doesn't cause a reflow, also breaks show
Expected Results:  Hiding the table should cause the table below to re-flow,
                   that is slide up.  Show should work after hidden, shouldn't
                   require two clicks.

Comment 1

[Greg Breland]

Reporter-> URL is invalid, can you fix the url or give us a short example of
what is happening?

Comment 2

[Sandip Chokshi]

Attachment: Simple html file with the test case

Comment 3

[Sandip Chokshi]

The URL is only accessable inside the AOL/Netscape firewall.  I've attached the 
html test case.

Comment 4

[Johnny Stenback (:jst)]

Setting visibility to 'hide' shouldn't cause a reflow, so that's not a bug, and
the behavior of setting element.style.position = ''; isn't defined by the DOM
spec, so I'm not sure what to do about that (what does IE do?).

Using element.style.display = 'none'; (and 'table') does seem to work tho, at
least to some extent. If I change the 'enable' method to:

function enable(spanID, displayState) {
    var element = document.getElementById(spanID);

    if (displayState == 'hidden') {
        element.style.display = 'none';
    } else {
        element.style.display = 'table';
    }
}

The table does get removed and things are reflown when disabling a table, and
enabling a table (i.e. setting style.display = 'table') does show the table
again but after this the table is never removed again. The DOM code does the
right thing, reassigning to table layout.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Testcase that uses style.display = none/table.

Comment 6

[Sandip Chokshi]

I was using the position attribute to remove it from the flow.  On IE, setting 
the display attribute from element.style.display = 'none' to 
element.style.display = '' resets the display to the default, effectively an 
undo.  Does netscape an equivalent way of "undoing" the setting back to the 
default?
This is useful because it allows the developer to write generic code that 
removes an element from the flow / adds it back into the flow without writing a 
switch to determine the appropiate display value for the element 
("table", "inline", ...)

Comment 7

[Hixie (not reading bugmail)]

|element.style.property = ''| should indeed 'reset' the property, as if the
property was not specified on the style attribute.

Comment 8

[Christopher Hoess (gone)]

Removing dead URL, adding keyword.

Comment 9

[anthonyd]

ok, from viewing the testcase and reading the report, this appears to be 
INVALID?

anyone ?

marking as INVALID.

Comment 10

[Amarendra Hanumanula]

 Hiding and removing from the tables and elements using CSS properties and 
the attached testcase works fine for me on Build ID: 2002010703
Platform: WIN2K
 

Comment 11

[Sandip Chokshi]

Attachment: Latest attachment that demonstrates the bug

This bug has not been fixed.  See the updated attachment.

Comment 12

[Sandip Chokshi]

Re-opening the bug.  This bug doesn't appear to be *completely* fixed.  I've 
attached a new test cases that demonstrates the bug.

Summary: Setting display:'none' and back to '' on a table removes and adds the 
table back into the flow.  This works now.  But if you reset the same table's 
display back to 'none', the table *does not* get removed from the flow.

The ability to dynamically removing / add elements into the flow of the 
document is a core feature of DIG applications.

Comment 13

[Sandip Chokshi]

This test case fails on Windows XP against NS 6.2 and Mozilla 0.9.8.

Comment 14

[Jaime Rodriguez, Jr.]

nominating for nsbeta1 as a request from the DIG.

Comment 15

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 16

[Kevin McCluskey (gone)]

Marking topembed+

Comment 17

[karnaze (gone)]

->attinasi (but hoping that glazman or dbaron will take this)

When things are working properly in the 3rd attachment, 
PresShell::AttributeChanged gets a hint of NS_STYLE_HINT_FRAMECHANGE, but when 
trying to hide the table a 2nd time, it gets a hint of NS_STYLE_HINT_NONE. Since 
jst says that this isn't a dom problem, it could be a problem in the css parser. 

PresShell::AttributeChanged(PresShell * const 0x032603a8, nsIDocument * 
0x032ad7a0, nsIContent * 0x032946b0, int 0, nsIAtom * 0x00ba1240 {"style"}, int 
1, int 6) line 5122
nsDocument::AttributeChanged(nsDocument * const 0x032ad7a0, nsIContent * 
0x032946b0, int 0, nsIAtom * 0x00ba1240 {"style"}, int 1, int 6) line 1991 + 36 
bytes
nsHTMLDocument::AttributeChanged(nsHTMLDocument * const 0x032ad7a0, nsIContent * 
0x032946b0, int 0, nsIAtom * 0x00ba1240 {"style"}, int 1, int 6) line 1460
nsDOMCSSAttributeDeclaration::ParsePropertyValue(const nsAString & {...}, const 
nsAString & {...}) line 282

Comment 18

[karnaze (gone)]

Taking the bug. Now I'm seeing that the hint is always 
NS_STYLE_HINT_FRAMECHANGE.

Comment 19

[karnaze (gone)]

dittto

Comment 20

[karnaze (gone)]

Attachment: test case reduced further

Comment 21

[karnaze (gone)]

Attachment: patch to get correct style context of table

Comment 22

[Alexandru Savulov]

Comment on attachment 71545 [details] [diff] [review]
patch to get correct style context of table

r=

Comment 23

[Bernd]

I remember that Boris did something similar, does your patch influence the
getcomputedstyle problem. Does your patch make Boris patch obsolete?

Comment 24

[Boris Zbarsky [:bzbarsky]]

This doesn't affect computed style or anything outside the CSSFrameConstructor....

Comment 25

[karnaze (gone)]

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

Comment 26

[karnaze (gone)]

Attachment: patch to fix this and bugs 51037 90960 91443

Comment 27

[karnaze (gone)]

Attachment: html diff of last patch

Comment 28

[Bernd]

Comment on attachment 72413 [details] [diff] [review]
patch to fix this and bugs 51037 90960 91443

r=bernd

Comment 29

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

Comment on attachment 72413 [details] [diff] [review]
patch to fix this and bugs 51037 90960 91443

sr=roc+moz

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 72413 [details] [diff] [review]
patch to fix this and bugs 51037 90960 91443

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 31

[Bernd]

once this is checked in the following bugs could also dissapear:  bug 56746 and
bug 102145. But this is only a hope.

Comment 32

[karnaze (gone)]

The patch is in.

Comment 33

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

This caused bug 129827.  Could you please either fix quickly or back out?

Comment 34

[Matthias Versen [:Matti]]

has this bug caused bug 129866 ?

Comment 35

[Christopher Whitt]

...and bug 129850 (according to dbaron).

Comment 36

[Mike Shaver (:shaver emeritus)]

Reopening: patch was backed out to resolve serious regression (bug 129827),
a=drivers.

Comment 37

[Mats Palmgren (inactive)]

All testcases works as expected.
WORKSFORME, build 2002-03-09-08 (trunk) on Windows 98 SE.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Mats Palmgren, this patch was backed out _after_ the build you are testing with.

Comment 39

[Bernd]

Attachment: default style sheet

Comment 40

[Bernd]

Attachment: alternate stylesheet

Comment 41

[Bernd]

Attachment: testcase

Comment 42

[Bernd]

I dont get bugzilla to serve the style sheets right. So I put the testcase as
bug92868_1.html under table/bugs.

Comment 43

[Boris Zbarsky [:bzbarsky]]

sheets fixed but the link to the first sheet in the testcase has the wrong
attachment number

Comment 44

[Bernd]

Attachment: revised testcase

Comment 45

[karnaze (gone)]

Attachment: revised patch

This fixes the regressions caused by the previous patch (attachment #6 [details]). That
patch made the erroneous assumption that for non tables, the parent style
context of a frame's style context equals the style context of a frame's parent
frame.

Comment 46

[karnaze (gone)]

Attachment: html diff of patch

Comment 47

[Marc Attinasi]

Comment on attachment 73540 [details] [diff] [review]
revised patch

sr=attinasi

Comment 48

[Alexandru Savulov]

Comment on attachment 73540 [details] [diff] [review]
revised patch

r=

Comment 49

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

Comment on attachment 73540 [details] [diff] [review]
revised patch

>Index: base/public/nsIFrame.h
>+   * Get the frame that is the primary frame associated with the content. 
>+   * aProviderFrame == aFrame except in the case of an outer table, in
>+   * which case it is the inner table.

This should say |aProviderFrame == this|, or something similar.  There's no
|aFrame|.  It might be good to state that the general requirement is that
|aProviderFrame| is either |this| or a child of |this|, although it would
probably be better to say that no nsIFrame implementations other than
the table outer frame have any business to override this.  I'm not really
a big fan of the new name for this method, either, but I can't think of
anything better.

>Index: html/base/src/nsFrame.cpp
>+nsFrame::GetStyleContextProvider(nsIPresContext* aPresContext,
>+                                 nsIFrame**      aProviderFrame)
> {
>   NS_ASSERTION(aPresContext && aProviderFrame, "null arguments: aPresContext and-or aProviderFrame");
>   if (aProviderFrame) {
>     // parent context provider is the parent frame by default
>+    *aProviderFrame = this;
>   }
>   return ((aProviderFrame != nsnull) && (*aProviderFrame != nsnull)) ? NS_OK : NS_ERROR_FAILURE;

Anybody who's silly enough to pass null for an out parameter deserves to
crash.	Also, |this| will never be null for a virtual function.  This function
could thus be rewritten as two lines:

  *aProviderFrame = this;
  return NS_OK;

Also, the comment is incorrect considering the current semantics of the
method.

>Index: html/base/src/nsFrame.h
>+// similar to NS_ENSURE_TRUE but with no return value
>+#define ENSURE_TRUE(x)                                        \
>+  PR_BEGIN_MACRO                                              \
>+    if (!(x)) {                                               \
>+       NS_WARNING("ENSURE_TRUE(" #x ") failed");              \
>+       return;                                                \
>+    }                                                         \
>+  PR_END_MACRO

Do we really want a macro with such a general name in nsFrame.h?
Maybe this should just be in nsFrameManager.cpp?  (Does
|NS_ENSURE_TRUE(condition, );| not work?)

>Index: html/base/src/nsFrameManager.cpp

Could you put all the ENSURE_TRUE() on their own line, and also
try to keep the line length under 80 characters in other places?
(Having them on a single line is probably also a pain with a
debugger.)

>+    nsIStyleContext* parentContext = aParentContextIn;

How about making this |nsCOMPtr<nsIStyleContext> parentContext;|,
and changing this:

>+      if (parentContext) {
>+        // addref the parent since it gets released later
>+        NS_ADDREF(parentContext);
>+        NOISY_TRACE_FRAME("non-null parent context provided: using it and assuming already resolved",aFrame);

to:

if (aParentContextIn) {
  parentContext = aParentContextIn;
  NOISY_TRACE_FRAME(...

and then removing the NS_IF_RELEASE at the end and using
|getter_AddRefs| in the right places?

It looks like these 4 lines should be |#ifdef DEBUG|, since
you don't need the GetParent call in optimized builds.	Since
there's a new variable, it would probably be good to put the
variable in a nested scope, i.e. (but with another 2 char
indent):

#ifdef DEBUG
     {
>+      // check to make sure that the provider is the child of aFrame
>+      nsIFrame* providerFrameParent;
>+      providerFrame->GetParent(&providerFrameParent); ENSURE_TRUE(providerFrameParent);
>+      NS_ASSERTION(providerFrameParent == aFrame, "invalid style context provider");
     }
#endif

These two lines don't seem to do anything (other than get a style
context that's never used).  Can you just remove them?

>+      nsCOMPtr<nsIStyleContext> providerContext;
>+      providerFrame->GetStyleContext(getter_AddRefs(providerContext));ENSURE_TRUE(providerContext);


>+      nsIFrame* grandParentFrame;
>+      aFrame->GetParent(&grandParentFrame); ENSURE_TRUE(grandParentFrame);
>+      nsCOMPtr<nsIStyleContext> providerContextParent;
>+      grandParentFrame->GetStyleContext(getter_AddRefs(providerContextParent)); ENSURE_TRUE(providerContextParent);

Instead of the ENSURE_TRUE, I think it makes more sense (in case
the table outer frame is the root, although that's not going to
happen in the current model -- but if so, why bother with the
ENSURE_TRUE?) is:

nsCOMPtr<nsIStyleContext> providerStyleContext;  /* or should this be called
grandParentStyleContext ? */
nsIFrame *grandParentFrame;
aFrame->GetParent(&grandParentFrame);
if (grandParentFrame) {
  grandParentFrame->GetStyleContext(getter_AddRefs(providerStyleContext));
}

>     // do primary context
>     nsIStyleContext* newContext = nsnull;
>     if (pseudoTag) {
>        nsIContent* pseudoContent = (aParentContent ? aParentContent : localContent);
>-       aPresContext->ResolvePseudoStyleContextFor(pseudoContent, pseudoTag, aParentContext, PR_FALSE,
>-                                                &newContext);
>-      NS_RELEASE(pseudoTag);
>+       aPresContext->ResolvePseudoStyleContextFor(pseudoContent, pseudoTag, parentContext, PR_FALSE,
>+                                                  &newContext);
>+       NS_RELEASE(pseudoTag);

Could you fix the indent here?	This code was 3-space indented
instead of 2-space, and it bugs me every time I see it.
(Also, if I'm going to pick style nits,
the ResolvePseudoStyleContextFor line is really long and the ()
around the ?: aren't needed.)

>-                ReResolveStyleContext(aPresContext, child, nsnull, content,
>+                nsIStyleContext* styleContext = (aFrame == providerFrame) ? nsnull : newContext;
>+                ReResolveStyleContext(aPresContext, child, styleContext, content,

I don't completely follow this.  A comment here might be nice.

Comment 50

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

> #ifdef DEBUG
>      {
> >+      // check to make sure that the provider is the child of aFrame
> >+      nsIFrame* providerFrameParent;
> >+      providerFrame->GetParent(&providerFrameParent);
> ENSURE_TRUE(providerFrameParent);
> >+      NS_ASSERTION(providerFrameParent == aFrame, "invalid style context 
> provider");
>      }
> #endif

Also, there's no need for the ENSURE_TRUE here.

Comment 51

[karnaze (gone)]

Attachment: revised patch with reviewer's comments

dbaron, thanks for the thorough review. I made the changes except - (a) I want
to keep ENSURE_TRUE in nsFrame.h, because I am tired of redefining it in tables
and elsewhere. I guess it really doesn't belong in xpcom, since com methods
always return a value. Not specifying the 2nd arg in NS_ENSURE_TRUE results in
compiler warnings. (b) I don't see much to be gained from an extra block and
the loss of 2 vertical lines in the #ifdef DEBUG change. Also, I relunctantly
took your suggestion of putting ENSURE_TRUE on separate lines, because
apparantly others don't aggree with my style of pushing this type of error
checking clutter out of sight.

Comment 52

[karnaze (gone)]

Attachment: htmldiff of patch

Comment 53

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

Comment on attachment 73630 [details] [diff] [review]
revised patch with reviewer's comments

r=dbaron

Comment 54

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

Comment on attachment 73630 [details] [diff] [review]
revised patch with reviewer's comments

a=roc+moz

carrying forward attinasi's sr

Comment 55

[karnaze (gone)]

The patch is in.

Comment 56

[Boris Zbarsky [:bzbarsky]]

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

Comment 57

[Amarendra Hanumanula]

 Hiding and removing tables from the flow using CSS works fine. All the
testcases works fine. Marking Verified.

Comment 58

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

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

Comment 59

[Christopher Hoess (gone)]

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

Comment 60

[Boris Zbarsky [:bzbarsky]]

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

Comment 61

[Boris Zbarsky [:bzbarsky]]

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