Closed Bug 265165 Opened 15 years ago Closed 15 years ago

XUL elements not displayed at all with 3px splitter

Categories

(Core :: Web Painting, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mimecuvalo, Assigned: roc)

References

()

Details

(Keywords: testcase)

Attachments

(4 files, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040928 Firefox/0.9.3

I have written the fireFTP extension and while it works fine under Windows and
Mac OSX, it's having problems with rendering under Linux.  This was filed as a
bug for my extension but I believe that this is a problem with Mozilla. See
http://mozdev.org/bugs/show_bug.cgi?id=7603 for the details of this bug.

Reproducible: Always
Steps to Reproduce:
1. Install the fireFTP extension ( http://fireftp.mozdev.org ) on a Debian system.
2. Voila
Could you possible create a reduced testcase for the problem?
Attached file Test case (obsolete) —
I cannot test to see if this is exhibiting the problem b/c I don't have linux
currently.  However, there's no reason it shouldn't - it is basically the same
XUL file as the original without any functionality.

If you need me to strip it down further, let me know.
If you could find a Linx volunteer to help you minimize the testcase, that would
really help.
I finally got a copy of Debian so I was able to mess around with it.  The
problem I found was with this element <splitter style="width: 3px; max-width:
3px;"/>
Setting the values to 3px causes the disappearances of elements but setting it
to 4px makes them reappear.  I'll post an attachment next that doesn't show the
bug.
Attachment #162638 - Attachment is obsolete: true
Setting it to 4px makes everything ok.
confirmed with linux trunk 2004102005

==> XUL
Assignee: roc → nobody
Status: UNCONFIRMED → NEW
Component: Layout: View Rendering → XP Toolkit/Widgets: XUL
Ever confirmed: true
Keywords: testcase
QA Contact: ian
Hardware: Other → PC
Summary: XUL elements not displayed at all → XUL elements not displayed at all with 3px splitter
Could this be because a splitter has 2×2px wide borders?
OK, so the first oddity is that when the border widths sum to more than the max
width the border is drawn incorrectly; what appears to happen is that a left
border of 1px is drawn and a right border is drawn 1px outside of the box.

Then when you add -moz-border-XXX-colors all hell breaks loose. Note that you
don't have to specify left or right colors, top or bottom breaks it too.
Assignee: nobody → roc
Component: XP Toolkit/Widgets: XUL → Layout: View Rendering
QA Contact: ian
In a similar vein, this testcase also causes rendering problems.  This is when
you have a vertical splitter and setting the max-width attribute to anything. 
(This testcase is on a Windows XP machine - I'm not sure if this is showing up
in Linux) I'll post the testcase without max-width showing no problems.
Attached file Testcase without problem (obsolete) —
This is with the vertical splitters again.  (again, this I tested on Windows)
Comment on attachment 163292 [details]
Another test case showing similar problem

never mind - i'm an idiot - this is using max-width not max-height
Attachment #163292 - Attachment is obsolete: true
Attachment #163293 - Attachment is obsolete: true
So the real bug is the 5px min-width is not overriding the 3px width/maxwidth,
which it should per CSS spec.  So we end up with a frame that's too narrow for
its own borders and such, which presumably confuses some code somewhere else. 
The point is, that really just shouldn't happen.  I suppose we can try to find
the code that mispaints as well (though it's not clear what "correct" painting
would be!), but we should fix the underlying XUL bug.
Attached patch A start (obsolete) — Splinter Review
This fixes nsBox::BoundsCheck.	This doesn't help, though, for two reasons:

1) There are cases in nsSprocketLayout where we GetMaxSize and truncate to it
if
   the frame got to be bigger than it.
2) Splitters inherit from nsBoxFrame, which has its own GetMaxSize impl.  So
   even fixing nsBox::GetMaxSize to do GetMinSize and then BoundsCheck()
doesn't
   help.

I could hack all this to work "right", but there may be other things that make
the same bogus assumptions...

The right answer is to get computed style data from the reflow state, not from
the style structs in AddCSSMinSize/AddCSSMaxSize.  Unfortunately, the reflow
state on the box layout state is for some random frame (higher up the frame
tree?) not for the box in question.  So getting style data off it is sorta
useless.

None of this will help with a testcase that resets the min-width, of course...
Attached patch fix (obsolete) — Splinter Review
This seems to fix it. Do what bz did, but also, whenever we do GetMaxSize, we
make sure it is subject to bounds checking at least by comparing it to the
minsize. Add nsBox::BoundsCheckMinMax for the case where we don't need to
constrain the prefsize.
Attachment #163311 - Attachment is obsolete: true
Attachment #163777 - Flags: superreview?(bzbarsky)
Attachment #163777 - Flags: review?(bzbarsky)
Attachment #163777 - Attachment is obsolete: true
Attachment #163777 - Flags: superreview?(bzbarsky)
Attachment #163777 - Flags: review?(bzbarsky)
Attached patch fix (obsolete) — Splinter Review
Sorry, this is the actual patch.
Attachment #163778 - Flags: superreview?(bzbarsky)
Attachment #163778 - Flags: review?(bzbarsky)
Is there a reason not to put the bounds-checkes in GetMaxSize?

And again, this won't work if the CSS resets the min-width too...
> Is there a reason not to put the bounds-checkes in GetMaxSize?

Performance ... if you're getting the minsize anyway, it's cheaper to not have
to get it again in GetMaxSize.

> this won't work if the CSS resets the min-width too...

Not sure what you mean...
Splitters have an explicit min-width set in CSS, last I checked.  Does the
testcase render correctly with that patch and the CSS min-width set to 0?  That
is, does GetMinWidth() still end up with the right number here?
er, GetMinSize(), not GetMinWidth().
Will the .tree-splitter class needs an extra min-width: 0px; in xul.css?
(In reply to comment #18)
> Splitters have an explicit min-width set in CSS, last I checked.  Does the
> testcase render correctly with that patch and the CSS min-width set to 0?

It does if I set min-width on the splitter element to 0 like so:
  <splitter style="width: 3px; max-width: 3px; min-width:0px;"/>


> That is, does GetMinWidth() still end up with the right number here?

Yeah. Why wouldn't it? the splitter doesn't have children so
nsBoxFrame::GetMinSize just computes 0,0.

> Will the .tree-splitter class needs an extra min-width: 0px; in xul.css?

Actually they are too fat but setting min-width to 0px doesn't fix it. Hmm.
Comment on attachment 163778 [details] [diff] [review]
fix

r+sr=bzbarsky
Attachment #163778 - Flags: superreview?(bzbarsky)
Attachment #163778 - Flags: superreview+
Attachment #163778 - Flags: review?(bzbarsky)
Attachment #163778 - Flags: review+
I've still got to fix the tree-splitters...
Attached patch fix v2Splinter Review
The problem with tree-splitters is that CSS min-width:0 is interpreted as no
min-width set, so nsBoxFrame::GetMinSize just falls back to the boxlayout
minimum size which includes borders and padding, so we get a minimum size that
isn't zero. Setting the CSS min-width to anything other than zero is honoured,
but it doesn't look right and the tree code seems to go haywire anyway.

So I thought of this awful hack. Currently XUL doesn't honour percentage
min-width. Supporting it properly would probably be hard, but supporting 0% is
not hard at all, so we can set the width and height of .tree-splitter to 0%,
and 0% is not interpret as unset...

I made the rules important because the cascade seems to favour the XBL
stylesheet otherwise.
Comment on attachment 164043 [details] [diff] [review]
fix v2

If you think this is too ugly, an alternative, less hacky approach would be to
extend the min-width and min-height properties to take an additional value,
-moz-auto, style all XUL elements with min-width:-moz-auto and
min-height:-moz-auto by default (is that possible?), so that we can identify an
explicit setting of min-width/min-height to zero length.
Attachment #164043 - Flags: superreview?(bzbarsky)
Attachment #164043 - Flags: review?(bzbarsky)
Although I would really like to see how Hyatt's flexbox CSS draft handles this
issue before implementing a really comprehensive fix.
Comment on attachment 164043 [details] [diff] [review]
fix v2

Well, the real question is why a setting of min-width to 0 should be treated
differently from the default value of 0.  They're really the same thing -- no
minimum width....

r+sr=me, I guess, but please file a followup on sorting this stuff out.
Attachment #164043 - Flags: superreview?(bzbarsky)
Attachment #164043 - Flags: superreview+
Attachment #164043 - Flags: review?(bzbarsky)
Attachment #164043 - Flags: review+
I think the intent of the current code is that setting CSS 'min-width' and
'min-height' overrides the internal flexbox-based minimum size calculation
completely. So if you have box A containing box B and B's minimum size is
20px,20px then A's layout-based minimum size is (say) 20px,20px but setting CSS
minimum size to 1px,1px allows A to shrink to 1px,1px. Now since the initial
value of CSS minimum properties is 0px,0px we have to treat that as "not sent"
otherwise nothing will ever use the flexbox-based minimum size calculation.
checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
xul.css change for toolkit checked in:

Checking in xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.43; previous revision: 1.42
done
Blocks: 268099
Toolkit patch landed again on trunk.
This checkin seems to have regressed the testcase for bug 266284 even more -- now the image isn't even resized.
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.