Closed
Bug 92868
Opened 23 years ago
Closed 22 years ago
Hiding/Removing tables/elements from the flow using CSS doesn't work
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: sandipc, Assigned: karnaze)
References
Details
(Keywords: testcase, topembed+, Whiteboard: DIGBug PATCH)
Attachments
(9 files, 6 obsolete files)
2.33 KB,
text/html
|
Details | |
2.28 KB,
text/html
|
Details | |
1.82 KB,
text/html
|
Details | |
643 bytes,
text/html
|
Details | |
42 bytes,
text/css
|
Details | |
48 bytes,
text/css
|
Details | |
771 bytes,
text/html
|
Details | |
19.84 KB,
patch
|
dbaron
:
review+
roc
:
superreview+
roc
:
approval+
|
Details | Diff | Splinter Review |
68.99 KB,
text/html
|
Details |
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.
Reporter | ||
Updated•23 years ago
|
Whiteboard: DIGBug
Comment 1•23 years ago
|
||
Reporter-> URL is invalid, can you fix the url or give us a short example of what is happening?
Reporter | ||
Comment 2•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
The URL is only accessable inside the AOL/Netscape firewall. I've attached the html test case.
Comment 4•23 years ago
|
||
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.
Assignee: jst → karnaze
Status: UNCONFIRMED → NEW
Component: DOM Style → HTMLTables
Ever confirmed: true
QA Contact: ian → amar
Comment 5•23 years ago
|
||
Reporter | ||
Comment 6•23 years ago
|
||
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•23 years ago
|
||
|element.style.property = ''| should indeed 'reset' the property, as if the property was not specified on the style attribute.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla1.1
ok, from viewing the testcase and reading the report, this appears to be INVALID? anyone ? marking as INVALID.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
Comment 10•23 years ago
|
||
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
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 11•23 years ago
|
||
This bug has not been fixed. See the updated attachment.
Reporter | ||
Comment 12•23 years ago
|
||
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.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Comment 13•23 years ago
|
||
This test case fails on Windows XP against NS 6.2 and Mozilla 0.9.8.
Comment 15•23 years ago
|
||
Marking nsbeta1+
Assignee | ||
Comment 17•23 years ago
|
||
->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
Assignee: karnaze → attinasi
Status: REOPENED → NEW
Component: HTMLTables → Style System
Assignee | ||
Comment 18•23 years ago
|
||
Taking the bug. Now I'm seeing that the hint is always NS_STYLE_HINT_FRAMECHANGE.
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Whiteboard: DIGBug → DIGBug PATCH
Comment 22•23 years ago
|
||
Comment on attachment 71545 [details] [diff] [review] patch to get correct style context of table r=
Attachment #71545 -
Flags: review+
Comment 23•23 years ago
|
||
I remember that Boris did something similar, does your patch influence the getcomputedstyle problem. Does your patch make Boris patch obsolete?
Comment 24•23 years ago
|
||
This doesn't affect computed style or anything outside the CSSFrameConstructor....
Assignee | ||
Comment 25•23 years ago
|
||
*** Bug 93973 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•23 years ago
|
Attachment #71545 -
Attachment is obsolete: true
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
Comment 28•23 years ago
|
||
Comment on attachment 72413 [details] [diff] [review] patch to fix this and bugs 51037 90960 91443 r=bernd
Attachment #72413 -
Flags: review+
Comment on attachment 72413 [details] [diff] [review] patch to fix this and bugs 51037 90960 91443 sr=roc+moz
Attachment #72413 -
Flags: superreview+
Comment 30•22 years ago
|
||
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
Attachment #72413 -
Flags: approval+
Comment 31•22 years ago
|
||
once this is checked in the following bugs could also dissapear: bug 56746 and bug 102145. But this is only a hope.
Assignee | ||
Comment 32•22 years ago
|
||
The patch is in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
This caused bug 129827. Could you please either fix quickly or back out?
Comment 34•22 years ago
|
||
has this bug caused bug 129866 ?
Comment 35•22 years ago
|
||
...and bug 129850 (according to dbaron).
Reopening: patch was backed out to resolve serious regression (bug 129827), a=drivers.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 37•22 years ago
|
||
All testcases works as expected. WORKSFORME, build 2002-03-09-08 (trunk) on Windows 98 SE.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → WORKSFORME
Comment 38•22 years ago
|
||
Mats Palmgren, this patch was backed out _after_ the build you are testing with.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 39•22 years ago
|
||
Comment 40•22 years ago
|
||
Comment 41•22 years ago
|
||
Comment 42•22 years ago
|
||
I dont get bugzilla to serve the style sheets right. So I put the testcase as bug92868_1.html under table/bugs.
Updated•22 years ago
|
Attachment #73457 -
Attachment mime type: text/plain → text/css
Updated•22 years ago
|
Attachment #73458 -
Attachment mime type: text/plain → text/css
Comment 43•22 years ago
|
||
sheets fixed but the link to the first sheet in the testcase has the wrong attachment number
Comment 44•22 years ago
|
||
Attachment #73459 -
Attachment is obsolete: true
Assignee | ||
Comment 45•22 years ago
|
||
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.
Attachment #72413 -
Attachment is obsolete: true
Assignee | ||
Comment 46•22 years ago
|
||
Attachment #72414 -
Attachment is obsolete: true
Comment 47•22 years ago
|
||
Comment on attachment 73540 [details] [diff] [review] revised patch sr=attinasi
Attachment #73540 -
Flags: superreview+
Comment 48•22 years ago
|
||
Comment on attachment 73540 [details] [diff] [review] revised patch r=
Attachment #73540 -
Flags: review+
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.
> #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.
Assignee | ||
Comment 51•22 years ago
|
||
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.
Assignee | ||
Comment 52•22 years ago
|
||
Attachment #73540 -
Attachment is obsolete: true
Attachment #73542 -
Attachment is obsolete: true
Comment on attachment 73630 [details] [diff] [review] revised patch with reviewer's comments r=dbaron
Attachment #73630 -
Flags: review+
Comment on attachment 73630 [details] [diff] [review] revised patch with reviewer's comments a=roc+moz carrying forward attinasi's sr
Attachment #73630 -
Flags: superreview+
Attachment #73630 -
Flags: approval+
Assignee | ||
Comment 55•22 years ago
|
||
The patch is in.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 56•22 years ago
|
||
*** Bug 130887 has been marked as a duplicate of this bug. ***
Comment 57•22 years ago
|
||
Hiding and removing tables from the flow using CSS works fine. All the testcases works fine. Marking Verified.
Status: RESOLVED → VERIFIED
*** Bug 112836 has been marked as a duplicate of this bug. ***
Comment 59•22 years ago
|
||
*** Bug 104386 has been marked as a duplicate of this bug. ***
Comment 60•22 years ago
|
||
*** Bug 110309 has been marked as a duplicate of this bug. ***
Comment 61•22 years ago
|
||
*** Bug 137272 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•