310124 - Double border is incorrectly compressed when rendered; regression.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michael Bolin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

In Firefox 1.0.7, a double border was given its correct spacing. In Firefox
1.5b1, this is not the case. The provided URL demonstrates this problem where a
double border is compressed into a single line for certain screen resizings.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.bolinfest.com/mozilla/missing_border.html
2. Click the link to show the page at the specified size


Actual Results:  
Look at the 4th border and note it is compressed.

Expected Results:  
Should have been a double border with a visible space between the two lines.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Somewhere in March 2005.
Build with double line: 1.8b2_2005032807
Build with single line: 1.8b2_2005032906

Comment 2

[Martijn Wargers (dead)]

I get the same regression range (although it's not clear to me what is causing it)
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-03-28+07%3A00%3A00&maxdate=2005-03-29+07%3A00%3A00&cvsroot=%2Fcvsroot

Comment 3

[Martijn Wargers (dead)]

Attachment: Testcase from reporter

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

taking.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

same problem is occured with "groove" and "ridge".
See this groove case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2982&action=view

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0(original patch created by Takeshi Ichimaru)

testcase for all border styles:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2981&action=view

Comment 7

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

It looks like this allows a 1px double border to be drawn 2px wide, and thus extend outside layout's border-box for the frame. Is that true?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> It looks like this allows a 1px double border to be drawn 2px wide, and thus
> extend outside layout's border-box for the frame. Is that true?
> 

I cannot reproduce it on comment 6's testcase.
But I can see sometimes the case in 0.5px + ridge or groove cases.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This patch has a ploblem when the border-width < 1px.
But I think that we can fix this problem by fixing bug 287624.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

We don't draw 2px wide if the border-width < 1px. See:
http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSRendering.cpp#1807
> 1807     // If a side needs a double border but will be less than two pixels,
> 1808     // force it to be solid (see bug 1781).
> 1809     if (aBorderStyle.GetBorderStyle(side) == NS_STYLE_BORDER_STYLE_DOUBLE) {
> 1810       nscoord widths[] = { border.top, border.right, border.bottom, border.left };
> 1811       forceSolid = (widths[side]/twipsPerPixel < 2);
> 1812     } else 
> 1813       forceSolid = PR_FALSE;

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry.

> We don't draw 2px wide if the border-width < 1px. See:

We don't draw 2px wide if the border-width < 2px. See:
                                            ^^^

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah... Should we expand this code as following?

> 1809     if (aBorderStyle.GetBorderStyle(side) == NS_STYLE_BORDER_STYLE_DOUBLE ||
               aBorderStyle.GetBorderStyle(side) == NS_STYLE_BORDER_STYLE_GROOVE ||
               aBorderStyle.GetBorderStyle(side) == NS_STYLE_BORDER_STYLE_RIDGE) {
> 1810       nscoord widths[] = { border.top, border.right, border.bottom, border.left };
> 1811       forceSolid = (widths[side]/twipsPerPixel < 2);

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

This patch looks better than previous patch.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in. Thank you Ichimaru-san.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, now, the tree is closed, I backed out the patch.
I will retry to check-in after opened.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Masayuki Nakano, do you want to add that testcase to the layout regression tests?  Or should I do it?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> Masayuki Nakano, do you want to add that testcase to the layout regression
> tests?  Or should I do it?

What's? I cannot understand what you say...
Does this patch make regression?

Comment 19

[Boris Zbarsky [:bzbarsky]]

No, this didn't cause a regression.  What I was saying was that if we can create a testcase for this bug that shows the problem when just loaded in a window (independently of window size), we should check the testcase into CVS at http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/ and add it to the list at http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/rtest.lst so that running the layout regression tests would catch this bug being reintroduced if that ever happens.

The problem would be writing such a testcase, I guess.  I hadn't realized till I just looked at it that this testcase depends on a particular window size.  :(

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
Yes. This bug is reproduced *sometimes*. But I can see this bug in this testcase.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2981&action=view

if "border: double 2px;", the border should be rendered as two pixel width solid line. And if "border: double 5px;" or "border: double 7px;", these twin lines should be rendered always same witdh.

And "border-width: 0.5px;", it should be rendered as 1px line. Because it means border. We should render the border, even if we ignore the width.
# but we are having this bug in dotted and dashed cases.
# This, I will file a new bug and fix it.

I think that we can make stable testcase with above cases.

Comment 21

[Martijn Wargers (dead)]

Attachment: testcase3

Something like this? I think this is a testcase that is resolution independant.
Maybe a more elaborate testcase (like from comment 20) is needed that is also resolution independant.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

looks good!

Comment 23

[Boris Zbarsky [:bzbarsky]]

Added that to the regression tests: http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20051201+1422&maxdate=20051201+1423&cvsroot=%2Fcvsroot

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.8 branch.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for checked-in to 1.8 branch