Closed
Bug 3505
Opened 26 years ago
Closed 25 years ago
[BLOCK] display:none doesn't (always) delete applicable frames
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect, P2)
Tracking
()
VERIFIED
FIXED
M5
People
(Reporter: mikepinkerton, Assigned: buster)
References
Details
When display:none is set on a toolbar (by clicking on the grippy and setting the
attribute via CSS), the reflow is triggered but the frames in question never get
destroyed.
Reporter | ||
Comment 1•26 years ago
|
||
On debug builds, this is easy to test. Click on the grippy in a toolbar, and you
will see the following message if this still happens:
BUG 3505:: Found a collapsed toolbar (display:none) but the frame still
exists!!!!
I'm testing in the reflow code if the attribute has been set on the content node.
The printf occurs if there is a frame that corresponds to the content node and
the attribute is set. It shouldn't trigger the printf because there shouldn't be
a frame for that content node, but it does, ergo the bug.
Reporter | ||
Updated•26 years ago
|
Summary: display:none doesn't delete applicable frames → [BLOCK] display:none doesn't delete applicable frames
Target Milestone: M4
Reporter | ||
Comment 2•26 years ago
|
||
Setting m4 milestone and marking it as a blocker
Reporter | ||
Comment 3•26 years ago
|
||
adding trudelle to cc list
Updated•26 years ago
|
Status: NEW → ASSIGNED
Comment 6•26 years ago
|
||
Accepting bug. I will look into this today.
Comment 7•26 years ago
|
||
Yeah I'm seeing this on Linux now.
Comment 8•26 years ago
|
||
I'm seeing this on Linux, too, and don't necessarily have to click on a grippy
to get it (pnunn was playing around with loading images in apprunner and we saw
a bunch of these messages).
Comment 9•26 years ago
|
||
Any progress on this bug Nisheeth? It is still blocking Pink from work we need
on the toolbar.
Comment 10•26 years ago
|
||
OK, I am on this as I write this. Last week I got diverted into M3 bugs. Sorry
about that, pink. I'll update this bug as I know more...
Updated•26 years ago
|
Assignee: nisheeth → waterson
Status: ASSIGNED → NEW
Comment 11•26 years ago
|
||
nsToolboxFrame::CollapseToolbar() handles the click event on the grippy on the
toolbar. It calls RDFElementImpl::SetAttribute() to set the "collapsed"
attribute on the toolbar to true. RDFElementImpl::SetAttribute() sends an
attribute change notification that ends up at
nsCSSFrameConstructor::AttributeChanged() which calls
RDFElementImpl::GetStyleHintForAttributeChange() to get a style hint from the
RDF content object.
In order for collapsing toolbars to work the following needs to happen:
1) RDFElementImpl::GetStyleHintForAttributeChange() should return a style hint
of NS_STYLE_HINT_FRAMECHANGE if the "collapsed" attribute is changing. I think
its going to be bad to centralize the style hint decision inside the RDF
element. Maybe it should get a style hint from the XUL element contained within
it. That way each XUL element gets to decide the style hint for attribute
changes on itself.
I'm re-assigning the bug to Chris Waterson, owner of RDFElementImpl and putting
myself on the cc list.
Reporter | ||
Comment 12•26 years ago
|
||
The node shouldn't have to have any special knowledge about this to trigger a
reframe. I can make up any attribute i want and have style attribute selectors
that reframe, or reflow, or make it dance like a chicken if the style I write
says so.
Or am i totaly misunderstanding the general nature of attributes and css?
Comment 13•26 years ago
|
||
Accepted bug to get of terry's spambot.
Comment 14•26 years ago
|
||
Eli, I'm re-assigning you these Mac bugs to take ownership. These are
Greg's old bugs...
Reporter | ||
Comment 15•26 years ago
|
||
Any status on this? I'm blocked on this and its target milestone is M4.
Comment 16•26 years ago
|
||
Added hyatt to cc list. He understands all this layout stuff. I don't.
Comment 17•26 years ago
|
||
Nisheeth, you should talk with Peter Linss about how the attribute system
works. FOR ANY ATTRIBUTE CHANGES THAT CAUSE ATTRIBUTE SELECTOR-BASED CSS RULES
TO BE MATCHED/UNMATCHED, THE ATTRIBUTE HINT SYSTEM IS IRRELEVANT. Since Pink's
toolbars rely only on a CSS attribute-selector rule that happens to specify a
reframe (it could just as easily have only needed a reflow), it is up to the
style system code that fires on an attribute changed event to determine the
impact of the change caused by the newly matched/unmatched rules and to do the
right thing.
We should NOT be returning the hint that you're saying we should be returning.
That's wrong.
Comment 18•26 years ago
|
||
This sounds like it's Peter Linss's bug.
Comment 19•26 years ago
|
||
Yes, I agree with Pink and Hyatt's analysis. I was confused.
Comment 20•26 years ago
|
||
Slipping to M5.
Comment 21•26 years ago
|
||
I believe this is fixed with my checkin for bug 4622. Anyone care to check?
Updated•26 years ago
|
Assignee: waterson → evaughan
Status: ASSIGNED → NEW
Comment 22•26 years ago
|
||
Re-assigning this to Eric Vaughan, as per discussion between Vidur, him, and me.
nsBoxFrame needs to over-ride the InsertFrame() and RemoveFrame() methods of
nsIFrame.
Updated•26 years ago
|
Assignee: evaughan → kipp
Comment 23•26 years ago
|
||
Ok I looked at this. nsBoxFrame does not respond when frames are removed or
added dynamically because its base class nsContainerFrame does not provide any
default behavior for RemoveFrames, AppendFrames, InsertFrames. This class should
provide basic implementation so subclasses can inherit them. If I just put this
code in my nsBoxFrame I would have to duplicate the code over and over for every
frame I create that is a container. Only frames that need to do something
special like BlockFrames should be redefining these methods.
Kipp can you take a look at this and maybe to get to the right layout person?
Thanks
Comment 24•26 years ago
|
||
After a bit of debate, we ended up deciding that because the desired behavior
isn't (yet) generic, eric would put the necessary code in his container
class(s).
Updated•26 years ago
|
Status: NEW → RESOLVED
Closed: 26 years ago
Resolution: --- → FIXED
Comment 25•26 years ago
|
||
Implemented InsertFrames, AppendFrames, and DeleteFrame. All fixed.
Comment 26•25 years ago
|
||
Using the current Win32 and Linux M4 builds (4.13.99), I can't reproduce this
problem.
However, I'd prefer that Mike put in his two cents prior to verifying it, since
I'm not familiar with the functional area, and am relying on the StdOut warning
text.
Reporter | ||
Comment 27•25 years ago
|
||
It works with the top toolbox, but not the blue toolbar at the bottom. I think
that there is still something wrong with the css reflow analysis. I'll put in a
breakpoint and see if DeleteFrame is being called on the box. If not, it's a CSS
bug.
Reporter | ||
Updated•25 years ago
|
Status: RESOLVED → REOPENED
Reporter | ||
Comment 28•25 years ago
|
||
Ok, today it doesn't work at all. RemoveFrame isn't being called on any toolbox.
reopening bug.
Reporter | ||
Updated•25 years ago
|
Resolution: FIXED → ---
Reporter | ||
Comment 29•25 years ago
|
||
clearing the resolution.
Updated•25 years ago
|
Assignee: evaughan → vidur
Status: REOPENED → NEW
Comment 30•25 years ago
|
||
This worked for a while. But now in the current build removeFrame doesn't ever
called anymore.
Updated•25 years ago
|
Assignee: vidur → kipp
Comment 31•25 years ago
|
||
Don't know why this isn't working, but layout ain't my bag, baby!
Reporter | ||
Updated•25 years ago
|
Hardware: Macintosh → All
Summary: [BLOCK] display:none doesn't delete applicable frames → [BLOCK] display:none doesn't (always) delete applicable frames
Reporter | ||
Comment 32•25 years ago
|
||
changing platform to all
Ok, we're back to the state we were in a week or two ago. Collapsing in the top
toolbox in apprunner works (as much as I've coded it, the toolbars disappear,
which is the key). However, in the toolbox at the bottom of the window (the blue
one, representing a task bar) the reframe still does not happen. The
nsBoxFrame::RemoveFrames() method is not getting called where is most certainly
is getting called in the top toolbox.
As far as the bottom toolbox knows, it is setting the attribute which triggers
the style change to display:none. In debug builds it continues to print out
diagnostic error messages that the frame is still there when the attribute is set
to be collapsed.
Why this works in one toolbox and not the other is the new bug. Let's keep this
one open because the refame-determination code isn't perfect.
Status: NEW → RESOLVED
Closed: 26 years ago → 25 years ago
Resolution: --- → FIXED
Comment 33•25 years ago
|
||
It turned out to be css cascading priority bug with xul.css and
navigator.css...fixed!
Reporter | ||
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 34•25 years ago
|
||
verifying.
Comment 35•25 years ago
|
||
Thanks, Mike!
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•