Last Comment Bug 173051 - Wrong space between block level elements that have no margin
: Wrong space between block level elements that have no margin
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
: Hixie (not reading bugmail)
:
Mentors:
: 218037 (view as bug list)
Depends on: 63336
Blocks: 287774 310761
  Show dependency treegraph
 
Reported: 2002-10-07 08:19 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
Modified: 2006-03-12 17:02 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.65 KB, patch)
2004-10-29 17:02 PDT, Hideo Saito
no flags Details | Diff | Splinter Review
patch (2.62 KB, patch)
2004-10-30 17:30 PDT, Hideo Saito
dbaron: review-
Details | Diff | Splinter Review
fix (4.89 KB, patch)
2005-03-23 17:11 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2002-10-07 08:19:33 PDT
If some block level elements have no margin,the elements should be connected.
But the following testcase are not always visible so.

* testcase of standards mode.
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1236
* testcase of quirks mode.
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1237
* the paragraph has "height:1em;" with testcase of standards mode(1236).
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1238

I can see space of 1px between the paragraphs.
If you can't see the space, you may use "TextZoom".
This problem seems to be influenced by the font-size.

#screenshot
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1239
2002100204/Win-ME/attachment-1236/100%

This problem is reproduced....
2002100608-trunk/WinXP.................all cases
2002091808-trunk/Win98SE...............all cases
2002100308-trunk/MacOS 9.2.............1238 only
2002100208-trunk/FreeBSD...............1238 only

This problem comes Bugzilla-jp.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2718
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2002-10-07 11:25:40 PDT
worksforme, linux build 2002-10-04-08....  This is probably an OS-dependent font
metrics issue...
Comment 2 Shanjian Li 2002-10-07 15:16:53 PDT
The problem seems caused by accumulated calculation round-off difference. 
I could reproduce the bug. 
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2003-05-01 22:36:13 PDT
.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2003-05-05 17:13:35 PDT
Has this been seen on any non-Windows platforms?  I'm not sure why it's PC/All.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2003-08-07 17:53:20 PDT
Sorry, my mistake.
I change to All/All.

We could reproduce with...
Win98SE, WinMe, WinXP, MacOS9.2, FreeBSD.
Comment 6 Simon Paquet [:sipaq] 2003-09-02 03:35:38 PDT
*** Bug 218037 has been marked as a duplicate of this bug. ***
Comment 7 Hideo Saito 2004-10-29 17:02:55 PDT
Created attachment 163919 [details] [diff] [review]
patch

The value of yTop and yBottom are adjusted to alignment of onePixel by trimming
the reminder divided with onePixel.
Comment 8 Hideo Saito 2004-10-30 08:53:23 PDT
The following testcase using textzoom(for example 90%) indicates another problem
that is appearance of black lines.

> * the paragraph has "height:1em;" with testcase of standards mode(1236).
> http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1238

The font size of nsStyleFont of nsTextFrame::Reflow is wrong, since the font
size of 90% is not aligned by onePixel. For exmanple, if default font size of
mDefaultSansSerifFont on PresContext structure is 192 twips, the size of 90% is
172 twips, in result (if one pixel is 12 twips) the remainder is 4 twips. 

please refer to following code of content/base/src/nsRuleNode.cpp.
The value of fontData->mFont.size is wrong, since the font size has remainder.

nsRuleNode::SetDefaultOnRoot(const nsStyleStructID aSID, nsStyleContext* aContext)
{
  switch (aSID) {
    case eStyleStruct_Font: 
    {
      nsStyleFont* fontData = new (mPresContext) nsStyleFont(mPresContext);

      nscoord minimumFontSize =
        mPresContext->GetCachedIntPref(kPresContext_MinimumFontSize);

      if (minimumFontSize > 0 && !IsChrome(mPresContext)) {
        fontData->mFont.size = PR_MAX(fontData->mSize, minimumFontSize);
      }
      else {
        fontData->mFont.size = fontData->mSize;
      }
      aContext->SetStyle(eStyleStruct_Font, fontData);
      return fontData;
Comment 9 Hideo Saito 2004-10-30 17:30:24 PDT
Created attachment 164026 [details] [diff] [review]
patch
Comment 10 Ben Fowler 2005-02-05 07:35:45 PST
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050205
Firefox/1.0+

Probably still in.

The third testcase using text zoom has the defect.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-03-07 21:52:18 PST
roc could correct me on this, but I think we've had discussions about whether we
want to do this like the pixel rounding that attachment 164026 [details] [diff] [review] does, and I think
we've decided that we don't want to do this.  And if we do, I think this is the
wrong approach -- we shouldn't be changing things in just one or two spots.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-20 21:48:57 PST
Well, it's a bit complicated.

On non-antialiasing devices it's fine to just ensure that the blocks are
contiguous in layout coordinate space. Then we can make sure that the gfx layer
rounds correctly so that it sees every pixel is covered by some block. That was
what I used to think was the solution.

But on antialiasing devices --- like Cairo will be --- it's harder because the
natural thing to do with coverage-based antialiasing would make some pixel lines
end up 75% white or something (50% white on 50% white on black).

I guess we need to talk to the Cairo guys to see how they think apps should
solve such issues in general.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-22 13:41:29 PST
There's a really good thread on the Cairo list about this *exact* issue.

http://lists.freedesktop.org/archives/cairo/2005-March/003342.html

My understanding from the thread is that coverage-based antialiasing will simply
not work for us unless we work really hard to identify all rendered rectangle
edges that might possible adjoin other rendered rectangle edges. That seems
unrealistic. Instead we should simply turn off Cairo's antialiasing. When the
resources are available, we can use supersampling --- i.e., render into a canvas
that's 4X x 4X the actual size, and then shrink it down, averaging pixel values.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-22 13:43:25 PST
In the meantime, we should fix this bug by ensuring that a) layout makes the
actual block boxes adjoining and b) Gfx rounds correctly so that a pixel always
gets the color of the box its top-left corner is in.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-22 21:52:18 PST
I can reproduce the problem in attachement 1238, and the layout is fine (blocks
are adjoining). So the problem is in Gfx.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-22 21:59:35 PST
looks like the bug is really in nsTransform2D.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-23 17:11:12 PST
Created attachment 178418 [details] [diff] [review]
fix

This is the logical fix ... just always transform points, and transform them
consistently. It fixes this bug and all the testcases referenced in
nsTransform2D bugs still seem to work. I don't know what dcone's logic was
supposed to achieve.

By "transforming points consistently" I mean that a transformed coordinate is
always rounded to the nearest integer. This has the effect that whenever we
paint, a pixel should be filled with the color that would be painted at the
pixel's center. E.g. when filling the rectangle x=0.3, y=0.3, w=0.4, h=0.4, it
contains the point (0.5, 0.5) so we fill the pixel at (0, 0). (The logic works
out like this: the rectangle is defined by (0.3, 0.3), (0.7, 0.7), which rounds
to (0, 0), (1, 1).)
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-03-23 20:20:39 PST
Perhaps dcone's logic was to achieve consistency of border widths or something
like that?  (I haven't looked at it closely yet.  I'll try to do so later
tonight or tomorrow.)  That said, maybe we should be rounding border widths to
the nearest pixel in the style system for exactly that reason.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-23 20:41:00 PST
Yeah, with this approach, fractional coordinates can make rectangles with
nominally the same width be rendered with different widths. I don't think
there's any way to fix that that avoids gaps. I'd rather avoid gaps.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-03-23 20:52:26 PST
...and would you object to fixing what's probably the most obvious of those
problems by rounding border widths to the nearest pixel in the style system?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-23 21:05:37 PST
Rounding things in the style system is OK by me providing
-- people don't yell at us for being incorrect
-- it doesn't break real pages
-- we actually round to *device* pixels so that you can still get nice layouts
on high-res devices
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-03-24 15:01:28 PST
Comment on attachment 178418 [details] [diff] [review]
fix

Actually, I suspect this was an attempt to solve the fact that there were
problems when we scrolled by something that wasn't an exact number of pixels
(and then repainted some things over what was blitted).  The solution to that
problem (bug 16200) was to always scroll by exactly an integer number of pixels
so the bits blitted would correspond to what was repainted.
Comment 23 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-03-24 15:11:54 PST
FWIW, I filed bug 287624 on the borders thing -- although it's worth noting that
I was misunderstanding things, and it really has no relation to what's being
fixed here.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-28 13:03:37 PST
checked in

Note You need to log in before you can comment on or make changes to this bug.