173051 - Wrong space between block level elements that have no margin

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

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 [:bzbarsky]]

worksforme, linux build 2002-10-04-08....  This is probably an OS-dependent font
metrics issue...

Comment 2

[Shanjian Li]

The problem seems caused by accumulated calculation round-off difference. 
I could reproduce the bug. 

Comment 3

[Boris Zbarsky [:bzbarsky]]

.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Has this been seen on any non-Windows platforms?  I'm not sure why it's PC/All.

Comment 5

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

Sorry, my mistake.
I change to All/All.

We could reproduce with...
Win98SE, WinMe, WinXP, MacOS9.2, FreeBSD.

Comment 6

[Simon Paquet [:sipaq]]

*** Bug 218037 has been marked as a duplicate of this bug. ***

Comment 7

[Hideo Saito]

Attachment: patch

The value of yTop and yBottom are adjusted to alignment of onePixel by trimming
the reminder divided with onePixel.

Comment 8

[Hideo Saito]

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]

Attachment: patch

Comment 10

[Ben Fowler]

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-4, no longer working on Mozilla)]

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)]

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)]

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)]

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)]

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)]

looks like the bug is really in nsTransform2D.

Comment 17

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

Attachment: 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-4, no longer working on Mozilla)]

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)]

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-4, no longer working on Mozilla)]

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

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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)]

checked in