265165 - XUL elements not displayed at all with 3px splitter

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Mime Čuvalo]

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

Comment 1

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

Could you possible create a reduced testcase for the problem?

Comment 2

[Mime Čuvalo]

Attachment: Test case

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.

Comment 3

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

If you could find a Linx volunteer to help you minimize the testcase, that would
really help.

Comment 4

[Mime Čuvalo]

Attachment: Test case exhibiting bug

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.

Comment 5

[Mime Čuvalo]

Attachment: Test case without display problems

Setting it to 4px makes everything ok.

Comment 6

[Andrew Schultz]

confirmed with linux trunk 2004102005

==> XUL

Comment 7

[[e-mail]]

Could this be because a splitter has 2×2px wide borders?

Comment 8

[[e-mail]]

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.

Comment 9

[Mime Čuvalo]

Attachment: Another test case showing similar problem

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.

Comment 10

[Mime Čuvalo]

Attachment: Testcase without problem

This is with the vertical splitters again.  (again, this I tested on Windows)

Comment 11

[Mime Čuvalo]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: A start

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...

Comment 14

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

Attachment: fix

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.

Comment 15

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

Attachment: fix

Sorry, this is the actual patch.

Comment 16

[Boris Zbarsky [: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...

Comment 17

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

> 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...

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[Boris Zbarsky [:bzbarsky]]

er, GetMinSize(), not GetMinWidth().

Comment 20

[[e-mail]]

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

Comment 21

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

(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 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163778 [details] [diff] [review]
fix

r+sr=bzbarsky

Comment 23

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

I've still got to fix the tree-splitters...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, ok.

Comment 25

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

Attachment: fix v2

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 26

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

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.

Comment 27

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

Although I would really like to see how Hyatt's flexbox CSS draft handles this
issue before implementing a really comprehensive fix.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

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

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.

Comment 30

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

checked in.

Comment 31

[Stephen Walker]

Attachment: Port the min-size .tree-splitter changes to toolkit's version of xul.css

Comment 32

[Philip K. Warren]

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

Comment 33

[Steffen Wilberg]

Toolkit patch landed again on trunk.

Comment 34

[Boris Zbarsky [:bzbarsky]]

This checkin seems to have regressed the testcase for bug 266284 even more -- now the image isn't even resized.