Closed Bug 173051 Opened 22 years ago Closed 20 years ago

Wrong space between block level elements that have no margin

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: roc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
worksforme, linux build 2002-10-04-08.... This is probably an OS-dependent font metrics issue...
The problem seems caused by accumulated calculation round-off difference. I could reproduce the bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Future
.
Assignee: attinasi → block-and-inline
Component: Layout → Layout: Block & Inline
Priority: P3 → --
QA Contact: cpetersen0953 → ian
Target Milestone: Future → ---
Has this been seen on any non-Windows platforms? I'm not sure why it's PC/All.
Depends on: 63336
Sorry, my mistake. I change to All/All. We could reproduce with... Win98SE, WinMe, WinXP, MacOS9.2, FreeBSD.
Hardware: PC → All
*** Bug 218037 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) (deleted) — Splinter Review
The value of yTop and yBottom are adjusted to alignment of onePixel by trimming the reminder divided with onePixel.
Component: Layout: Block and Inline → Accessibility APIs
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;
Attached patch patch (deleted) — Splinter Review
Attachment #163919 - Attachment is obsolete: true
Component: Accessibility APIs → Layout: Fonts and Text
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.
Attachment #164026 - Flags: review?(dbaron)
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.
Attachment #164026 - Flags: review?(dbaron) → review-
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.
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.
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.
I can reproduce the problem in attachement 1238, and the layout is fine (blocks are adjoining). So the problem is in Gfx.
looks like the bug is really in nsTransform2D.
Attached patch fix (deleted) — Splinter Review
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).)
Assignee: layout.block-and-inline → roc
Status: NEW → ASSIGNED
Attachment #178418 - Flags: superreview?(dbaron)
Attachment #178418 - Flags: review?(dbaron)
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.
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.
...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?
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 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.
Attachment #178418 - Flags: superreview?(dbaron)
Attachment #178418 - Flags: superreview+
Attachment #178418 - Flags: review?(dbaron)
Attachment #178418 - Flags: review+
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.
Blocks: 287774
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Blocks: 310761
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: