Closed Bug 426772 Opened 16 years ago Closed 16 years ago

Floated first-letter generates too tall box

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: phiw2, Assigned: masayuki)

References

()

Details

(Keywords: regression, testcase)

Attachments

(5 files, 15 obsolete files)

385 bytes, text/html
Details
10.34 KB, image/png
Details
613 bytes, text/html
Details
597 bytes, text/html
Details
23.59 KB, patch
masayuki
: review+
masayuki
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Attached file testcase
The border around the letter F should fit nicely, there should be no space at the top or the bottom.

Regressed:
20080329_1413 OK
20080329_1427 fails

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1206825180&maxdate=1206826019

--> bug 421353
Flags: blocking1.9?
Attached image screenshot
On the left, current (incorrect) display, on the right, correct display (20080329_1413 hourly build)
Because now, the textframes always contains decoration paintable area. And I'm not sure why gecko was used actual glyph bounds for first-letter. Because Opera and Safari use similar rendering for current gecko. Is this "wrong"?
With the current implementation, it is not possible to achieve a typographically correct drop-cap. Depending on the font used, there is a large space at the top and the bottom.
Look at this page, on a Mac: http://www.l-c-n.com/phiw/
The first letter nearly drops bellow the first-line (not sure how it looks on Windows, I'm not close to a Windows box..

Up till now, Gecko implemented this by applying this from CSS 2.1:
quote:
To allow UAs to render a typographically correct drop cap or initial cap, the UA may choose a line-height, width and height based on the shape of the letter, unlike for normal elements.

In Safari and Opera, the typographically correct drop cap can be accomplished by using a narrow line-height, but line-height has no effect in Gecko (in this case of floated first-letter).
We should probably disable the adding of decoration paintable areas for floating first-letters.
(In reply to comment #4)
> We should probably disable the adding of decoration paintable areas for
> floating first-letters.

Yes.  (Or at least make it conditional on their actually having text-decoration, but that would require making text-decoration changes reflow, which probably isn't worth it.)

We did make text-decoration cause reflow, and that caused pref problems so bug 421353 fixed that. IMHO we should just stop adding decoration area for floating first-letters and live with the bug that text-decoration added to floating first-letters won't repaint properly. I've never seen anyone add text-decoration there anyway...
I think that if we use tight box for first letter, we should append overflow area for the normal decoration drawing area. I'm now creating such approach patch.
Assignee: nobody → masayuki
OS: Mac OS X → All
Hardware: PC → All
Yes, of course, that would work well. We couldn't use that approach for normal text because overflow areas on every frame would kill performance, but for floating first-letters that's fine.
Attached patch Patch v1.0 (obsolete) — Splinter Review
* using tight box
* adding decoration line drawable area to textframe/firstletterframe's overflow area
* adding GetBaseline() to nsTextFrame that returns mAscent (actual ascent)
* at painting and computing overflow rect, using the actual ascent

That fixes this bug. However, there are two bugs.
1. the decoration lines of floating first letter doesn't painted.
2. the most outer 1px of the decoration lines sometimes not redrawn (this may be bug 424249).
But they are should be separated to other bugs, I'm not sure the causes :-(
Attachment #313509 - Flags: superreview?(roc)
Attachment #313509 - Flags: review?(roc)
Comment on attachment 313509 [details] [diff] [review]
Patch v1.0

+++ layout/generic/nsHTMLContainerFrame.cpp	4 Apr 2008 01:11:06 -0000
@@ -65,16 +65,17 @@
 #include "nsIThebesFontMetrics.h"
 #include "gfxFont.h"
 #include "nsCSSFrameConstructor.h"
 #include "nsDisplayList.h"
 #include "nsBlockFrame.h"
 #include "nsLineBox.h"
 #include "nsDisplayList.h"
 #include "nsCSSRendering.h"
+#include "nsTextFrame.h"

Oops, this is not needed now. Please ignore this.
Attached patch Patch v1.0.1 (obsolete) — Splinter Review
Updating for latest trunk. And fixing some nits.
Attachment #313509 - Attachment is obsolete: true
Attachment #313651 - Flags: superreview?(roc)
Attachment #313651 - Flags: review?(roc)
Attachment #313509 - Flags: superreview?(roc)
Attachment #313509 - Flags: review?(roc)
+'ing so we can get this fix into the tree as soon as it's finished.  Good testcase and a regression.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
+  gfxFloat baseline = metrics.maxAscent;
+  // The ascent of first-letter frame's text is not same as the ascent of the
+  // font metrics. Because that uses tight box of the actual glyph.
+  if (mFrame->GetType() == nsGkAtoms::letterFrame) {

Instead of this code, can we just call GetBaseline on the frame to figure out where the baseline is?

+    nscoord descent = PR_MAX(GetSize().height, minDescent);

This doesn't look right. Need to subtract mAscent from GetSize().height

We only want to change the textframe ascent and descent for *floated* first-letters. Right now we're changing it for all first-letters which seems wrong. That could create line-height inconsistencies just because first-letter is being used.

+    // The ascent of first-letter frame is not same as the ascent of the font
+    // metrics. Because that uses tight box of the actual glyph.
+    // So, the first-letter frame must have the decoration lines drawable area
+    // in its overflow rect.
+    if (mPresContext->CompatibilityMode() != eCompatibility_NavQuirks &&
+        psd->mFrame->mFrame->GetType() == nsGkAtoms::letterFrame) {

I don't think this is right. The first-letter frame might not be a direct child. I think somehow we need to track whether we're inside a first-letter float and add text decorations to the overflow area in exactly that case.
(In reply to comment #15)
> +  gfxFloat baseline = metrics.maxAscent;
> +  // The ascent of first-letter frame's text is not same as the ascent of the
> +  // font metrics. Because that uses tight box of the actual glyph.
> +  if (mFrame->GetType() == nsGkAtoms::letterFrame) {
> 
> Instead of this code, can we just call GetBaseline on the frame to figure out
> where the baseline is?

nsIFrame::GetBaseline returns the distance of the border edge and the baseline. So, if we get it from first letter frame directly (i.e., the value is textframe->GetPosition().y + textframe->GetBaseline()), we need to recompute for the border/padding area.
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #313651 - Attachment is obsolete: true
Attachment #313962 - Flags: superreview?(roc)
Attachment #313962 - Flags: review?(roc)
Attachment #313651 - Flags: superreview?(roc)
Attachment #313651 - Flags: review?(roc)
Comment on attachment 313962 [details] [diff] [review]
Patch v2.0

Oops, that is wrong file, sorry.
Attachment #313962 - Flags: superreview?(roc)
Attachment #313962 - Flags: review?(roc)
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #313962 - Attachment is obsolete: true
Attachment #313963 - Flags: superreview?(roc)
Attachment #313963 - Flags: review?(roc)
mmmm, at floating first-letter, should we use actual glyph size only for the ascent? i.e., the descent space should contain the underline decoration area?
Attached patch Alternate Patch (obsolete) — Splinter Review
This only use tight box size for ascent. The first letter always have underline drawable area.
Attachment #313972 - Flags: superreview?(roc)
Attachment #313972 - Flags: review?(roc)
Attached patch Alternate Patch (obsolete) — Splinter Review
Oops, sorry for the spam.
Attachment #313972 - Attachment is obsolete: true
Attachment #313973 - Flags: superreview?(roc)
Attachment #313973 - Flags: review?(roc)
Attachment #313972 - Flags: superreview?(roc)
Attachment #313972 - Flags: review?(roc)
Attached image screenshot of alternavie patch (obsolete) —
The alternative patch doesn't overlap the underline to another line. But the descent looks too large...
Comment on attachment 313963 [details] [diff] [review]
Patch v2.0

Oops, this is not work at standards mode.
Attachment #313963 - Flags: superreview?(roc)
Attachment #313963 - Flags: review?(roc)
Attachment #313963 - Flags: review-
Comment on attachment 313973 [details] [diff] [review]
Alternate Patch

this doesn't work at standards mode too.
Attachment #313973 - Flags: superreview?(roc)
Attachment #313973 - Flags: review?(roc)
Attachment #313973 - Flags: review-
Attached patch Patch v3.0 (obsolete) — Splinter Review
This is based on alternative way. The bottom space is not killed for the underline. I think that this is better for accessibility and this is enough for the web design.
Attachment #313963 - Attachment is obsolete: true
Attachment #313973 - Attachment is obsolete: true
Attachment #313981 - Flags: superreview?(roc)
Attachment #313981 - Flags: review?(roc)
Attached image screenshot of Philippe's content (obsolete) —
Left is current trunk, right is patched build.
o.k.

This supports the line-height in floating first-letter frame. If first letter frame's line-height is not normal, the frame's content height becomes the specified line-height.

The first letter having underline should be rare case, but we cannot ignore the accessibility issue. But if we support the line-height property, the designers can use the "tight" layout.
Attachment #313981 - Attachment is obsolete: true
Attachment #313987 - Flags: superreview?(roc)
Attachment #313987 - Flags: review?(roc)
Attachment #313981 - Flags: superreview?(roc)
Attachment #313981 - Flags: review?(roc)
Comment on attachment 313987 [details]
screenshot (current trunk vs patched build)

Oops, this is screenshot...
Attachment #313987 - Attachment description: Patch v4.0 → screenshot (current trunk vs patched build)
Attachment #313987 - Flags: superreview?(roc)
Attachment #313987 - Flags: review?(roc)
Attachment #313987 - Attachment is patch: false
Attachment #313987 - Attachment mime type: text/plain → image/png
Attached patch Patch v4.0 (obsolete) — Splinter Review
Attachment #313988 - Flags: superreview?(roc)
Attachment #313988 - Flags: review?(roc)
Patch v4.0 works pretty well, so far. Having control over the line-height is probably more  'web-designer-friendly' as it matches what WebKit and Opera do.

In my tests, I get pretty consistent results with what WebKit does, and no really noticeable differences with Fx2/WinXP (::first-letter was br0ken on Gecko 1.8 Mac anyway - bug 294733).
The visual impact here is pretty minor - I really don't think this should block ff3.  Please re-nom/yell if you disagree...
Flags: blocking1.9+ → blocking1.9-
(In reply to comment #32)
> The visual impact here is pretty minor - I really don't think this should block
> ff3.  Please re-nom/yell if you disagree...
> 
Sorry Mike, I disagree. It is a regression both from 1.9b4, _and_ on Windows/Linux, from Gecko 1.8/FX 2.

See also comment 3.

Flags: wanted1.9.0.x?
Flags: blocking1.9?
Flags: blocking1.9-
I think this should block -- it's basically regressing our support for the entire feature (floated first-letter, i.e., drop-cap) to a state that makes Web authors much less likely to use it.
Is it possible we could do a simpler fix for 1.9, like just disabling the patch that caused this for text inside floating first letters (which I would think would be a very simple change)?
Not a very simple change, since that patch added the underline/overline space to the ascent/descent of the font.
+  // nsIFontMetrics::GetMaxDescent contains the normal underline drawable area.
+  nscoord minDescent;
   fm->GetMaxDescent(minDescent);
-  aMetrics.ascent = PR_MAX(NSToCoordCeil(textMetrics.mAscent), minAscent);
   nscoord descent = PR_MAX(NSToCoordCeil(textMetrics.mDescent), minDescent);
-
   aMetrics.height = aMetrics.ascent + descent;

We don't want to include the font maxDescent here for floating first-letters. I don't understand what you said about accessibility.

+  nscoord minDescent;
   fm->GetMaxDescent(minDescent);

Mixing max and min like this is very confusing. I would call the variables *fontDescent* and *fontAscent*.

Since you're implementing nsFirstLetterFrame::GetBaseline(), it should return ASK_FOR_BASELINE for aMetrics.ascent in Reflow().

+  // Ensure that the overflow rect contains the child textframe.
+  aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea,
+                           nsRect(0, 0, aMetrics.width, aMetrics.height));

This should use ConsiderChildOverflow --- the child's overflow area should be included

+    nscoord tmp = mFrame->GetBaseline() - mFrame->GetUsedBorderAndPadding().top;

Why are you substracting the border and padding here?

+      nsRect ovrelineArea(0, mAscent - ascent,

Typo.

Would this be simpler if we just included the font ascent and descent in the overflow areas of floating nsFirstLetterFrames and their textframe children, instead of including them in the frame areas?
(In reply to comment #37)
> +  // nsIFontMetrics::GetMaxDescent contains the normal underline drawable
> area.
> +  nscoord minDescent;
>    fm->GetMaxDescent(minDescent);
> -  aMetrics.ascent = PR_MAX(NSToCoordCeil(textMetrics.mAscent), minAscent);
>    nscoord descent = PR_MAX(NSToCoordCeil(textMetrics.mDescent), minDescent);
> -
>    aMetrics.height = aMetrics.ascent + descent;
> 
> We don't want to include the font maxDescent here for floating first-letters. I
> don't understand what you said about accessibility.

The underline of the first-letter will overlap to other lines.

> +    nscoord tmp = mFrame->GetBaseline() -
> mFrame->GetUsedBorderAndPadding().top;
> 
> Why are you substracting the border and padding here?

The result of GetBaseline() includes the border-top and padding-top area, right?

> Would this be simpler if we just included the font ascent and descent in the
> overflow areas of floating nsFirstLetterFrames and their textframe children,
> instead of including them in the frame areas?

Did you mean I should use nsIFontMetrics::GetMaxAscent/Descent/Height instead of nsLineLayout::CombineTextDecorations?
(In reply to comment #38)
> (In reply to comment #37)
> > +  // nsIFontMetrics::GetMaxDescent contains the normal underline drawable
> > area.
> > +  nscoord minDescent;
> >    fm->GetMaxDescent(minDescent);
> > -  aMetrics.ascent = PR_MAX(NSToCoordCeil(textMetrics.mAscent), minAscent);
> >    nscoord descent = PR_MAX(NSToCoordCeil(textMetrics.mDescent), minDescent);
> > -
> >    aMetrics.height = aMetrics.ascent + descent;
> > 
> > We don't want to include the font maxDescent here for floating first-letters. I
> > don't understand what you said about accessibility.
> 
> The underline of the first-letter will overlap to other lines.

That's fine with me if it simplifies things. Underlines on floating first-letters are an incredibly rare case, we don't need to worry about it.

> > +    nscoord tmp = mFrame->GetBaseline() -
> > mFrame->GetUsedBorderAndPadding().top;
> > 
> > Why are you substracting the border and padding here?
> 
> The result of GetBaseline() includes the border-top and padding-top area,
> right?

Yes, but you need to include them to get the correct distance from the top of the parent frame to the baseline.

> > Would this be simpler if we just included the font ascent and descent in the
> > overflow areas of floating nsFirstLetterFrames and their textframe children,
> > instead of including them in the frame areas?
> 
> Did you mean I should use nsIFontMetrics::GetMaxAscent/Descent/Height instead
> of nsLineLayout::CombineTextDecorations?

Mmm, something like that.

Seems to me you can fix this by having floating-first-letter text frames set their ascent and descent from the glyph extents, but add the font maxAscent/maxDescent to the frame overflow area if we're in quirks mode. That part's easy. Then you need to handle possible text-decoration on the nsFirstLetterFrame, so if we're in standards mode you add the font maxAscent/maxDescent to the first-letter's overflow area, using the ascent returned by the textframe's Reflow() to determine the baseline. Does that work?
(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > +  // nsIFontMetrics::GetMaxDescent contains the normal underline drawable
> > > area.
> > > +  nscoord minDescent;
> > >    fm->GetMaxDescent(minDescent);
> > > -  aMetrics.ascent = PR_MAX(NSToCoordCeil(textMetrics.mAscent), minAscent);
> > >    nscoord descent = PR_MAX(NSToCoordCeil(textMetrics.mDescent), minDescent);
> > > -
> > >    aMetrics.height = aMetrics.ascent + descent;
> > > 
> > > We don't want to include the font maxDescent here for floating first-letters. I
> > > don't understand what you said about accessibility.
> > 
> > The underline of the first-letter will overlap to other lines.
> 
> That's fine with me if it simplifies things. Underlines on floating
> first-letters are an incredibly rare case, we don't need to worry about it.

ok. I'll remove the line-height support also.

> > > +    nscoord tmp = mFrame->GetBaseline() -
> > > mFrame->GetUsedBorderAndPadding().top;
> > > 
> > > Why are you substracting the border and padding here?
> > 
> > The result of GetBaseline() includes the border-top and padding-top area,
> > right?
> 
> Yes, but you need to include them to get the correct distance from the top of
> the parent frame to the baseline.

If I remove the substracting, the decoration lines are below from the correct position...

> > > Would this be simpler if we just included the font ascent and descent in the
> > > overflow areas of floating nsFirstLetterFrames and their textframe children,
> > > instead of including them in the frame areas?
> > 
> > Did you mean I should use nsIFontMetrics::GetMaxAscent/Descent/Height instead
> > of nsLineLayout::CombineTextDecorations?
> 
> Mmm, something like that.
> 
> Seems to me you can fix this by having floating-first-letter text frames set
> their ascent and descent from the glyph extents,

yes.

> but add the font
> maxAscent/maxDescent to the frame overflow area if we're in quirks mode. That
> part's easy. Then you need to handle possible text-decoration on the
> nsFirstLetterFrame, so if we're in standards mode you add the font
> maxAscent/maxDescent to the first-letter's overflow area, using the ascent
> returned by the textframe's Reflow() to determine the baseline. Does that work?

Yeah, my patch did so, and it works fine. And there is another approach, we are always adding expand the overflow area of the textframe if that is child of floating-first-letter. Then, we can remove the nsFirstLetterFrame's computing. nsFirstLetterFrame just needs to call ConsiderChildOverflow, I think. (Is this hacky?)
Yeah, I guess the text frame must always have the same font style as the nsFirstLetterFrame, so the results will be the same. Do it!
Attached patch Patch v5.0 (obsolete) — Splinter Review
ok, let's land this.
Attachment #313974 - Attachment is obsolete: true
Attachment #313982 - Attachment is obsolete: true
Attachment #313987 - Attachment is obsolete: true
Attachment #313988 - Attachment is obsolete: true
Attachment #314353 - Flags: superreview?(roc)
Attachment #314353 - Flags: review?(roc)
Attachment #313988 - Flags: superreview?(roc)
Attachment #313988 - Flags: review?(roc)
Attached patch Patch v5.1 (obsolete) — Splinter Review
oops, this textframe also returns ASK_FOR_BASELINE.
Attachment #314353 - Attachment is obsolete: true
Attachment #314374 - Flags: superreview?(roc)
Attachment #314374 - Flags: review?(roc)
Attachment #314353 - Flags: superreview?(roc)
Attachment #314353 - Flags: review?(roc)
OK.  Adding this to the blocking list per dbaron in comment #34.  

However, Roc, if you feel there is significant regression risk here, I think we should wait.  Need to know, based on your pending review.
Flags: blocking1.9? → blocking1.9+
nsTextFrame:
+  aMetrics.ascent = nsHTMLReflowMetrics::ASK_FOR_BASELINE;

Why do we need to do this, why can't we just return mAscent?

+  gfxFloat baseline;
+  // The ascent of first-letter frame's text is not same as the ascent of the
+  // font metrics. Because that uses tight box of the actual glyph.
+  if (mFrame->GetType() == nsGkAtoms::letterFrame) {
+    nscoord tmp = mFrame->GetBaseline() - mFrame->GetUsedBorderAndPadding().top;
+    baseline = mFrame->PresContext()->AppUnitsToGfxUnits(tmp);

I figured out the need to subtract the border+padding here. It's because PaintTextDcorationLine expects the baseline relative to the content-rect, and adds the border+padding to the baseline and frame offset when drawing. Please document that.

nsFirstLetterFrame:
+  aMetrics.ascent = nsHTMLReflowMetrics::ASK_FOR_BASELINE;

I'm not sure why we need this here either. These uses of ASK_FOR_BASELINE might actually be inefficient especially in nsTextFrame.

How about just storing the baseline in nsFirstLetterFrame? Set it during Reflow, provide a method in nsFirstLetterFrame.h to get it.
(In reply to comment #45)
> nsTextFrame:
> +  aMetrics.ascent = nsHTMLReflowMetrics::ASK_FOR_BASELINE;
> 
> Why do we need to do this, why can't we just return mAscent?
> nsFirstLetterFrame:
> +  aMetrics.ascent = nsHTMLReflowMetrics::ASK_FOR_BASELINE;
> 
> I'm not sure why we need this here either. These uses of ASK_FOR_BASELINE might
> actually be inefficient especially in nsTextFrame.

Hey, you said that I should do it in comment 37.
> Since you're implementing nsFirstLetterFrame::GetBaseline(), it should return
> ASK_FOR_BASELINE for aMetrics.ascent in Reflow().
Attached patch Patch v5.2 (obsolete) — Splinter Review
adding nsFirsetLetterFrame::mBaseline, adding document in nsHTMLContainerFrame. And nsTextFrame and nsFirstLetterFrame return the actual ascent to aMetrics at Reflow().
Attachment #314374 - Attachment is obsolete: true
Attachment #314879 - Flags: superreview?(roc)
Attachment #314879 - Flags: review?(roc)
Attachment #314374 - Flags: superreview?(roc)
Attachment #314374 - Flags: review?(roc)
> Hey, you said that I should do it in comment 37.
> > Since you're implementing nsFirstLetterFrame::GetBaseline(), it should return
> > ASK_FOR_BASELINE for aMetrics.ascent in Reflow().

Right. What I should say is that to avoid violating the precondition for GetBaseline(), we shouldn't be implementing GetBaseline() here.

+  virtual nscoord GetBaseline() const { return mAscent; }

We don't need this in nsTextFrame now.

+    nscoord tmp = mFrame->GetBaseline() - mFrame->GetUsedBorderAndPadding().top;

Don't call GetBaseline here. cast mFrame to nsFirstLetterFrame* and call a new method, say GetFirstLetterBaseline, which can just be inline returning mBaseline. I hope that makes sense.
Attached patch Patch v5.3 (obsolete) — Splinter Review
Attachment #314879 - Attachment is obsolete: true
Attachment #314983 - Flags: superreview?(roc)
Attachment #314983 - Flags: review?(roc)
Attachment #314879 - Flags: superreview?(roc)
Attachment #314879 - Flags: review?(roc)
Comment on attachment 314983 [details] [diff] [review]
Patch v5.3

+  // The ascent of first-letter frame's text is not same as the ascent of the
+  // font metrics. Because that uses tight box of the actual glyph.

"may not be the same", "Because that may use the tight box"
Attachment #314983 - Flags: superreview?(roc)
Attachment #314983 - Flags: superreview+
Attachment #314983 - Flags: review?(roc)
Attachment #314983 - Flags: review+
Whiteboard: [reviewed patch in hand]
Attached patch Patch v5.3.1Splinter Review
Thank you, roc.

Let's land this. This fixes the regression. And this is not risky.
Attachment #314983 - Attachment is obsolete: true
Attachment #314993 - Flags: superreview+
Attachment #314993 - Flags: review+
Attachment #314993 - Flags: approval1.9?
Comment on attachment 314993 [details] [diff] [review]
Patch v5.3.1

a1.9=beltzner
Attachment #314993 - Flags: approval1.9? → approval1.9+
landed, and the tests are passed on Mac.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Thanks Masayuki-san and Roc !

verified with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041118 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Flags: wanted1.9.0.x?
Comment on attachment 314993 [details] [diff] [review]
Patch v5.3.1

>+PRBool
>+nsTextFrame::IsFloatingFirstLetterChild()
>+{
>+  if (!GetStateBits() & TEXT_FIRST_LETTER)
As my compiler happened to notice, this expression is a no-op since ! has higher precedence than & - I guess you forgot the ()s?
Depends on: 509956
Whiteboard: [reviewed patch in hand]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: