Closed Bug 142562 Opened 22 years ago Closed 22 years ago

problem with <td align=right or align=center


(Core :: Layout, defect)

Windows 2000
Not set





(Reporter: ftang, Assigned: shanjian)




(Keywords: intl, topembed+, Whiteboard: [adt2 RTM])


(5 files, 4 obsolete files)

Layout table bug 
I think this is a bad regresion
I am using branch 2002050306 build
the "18 April 20" and "2 May 2002" in the sub area title display outside the
right corner of the table.
Attached image screenshot
This WFM on the trunk. Do you have any mods in your tree, ftang?
Summary: nestging table text display porblem → nestging table text display porblem
I saw it first on my 20020503 branch window build. I can still see it on the
20020506 branch window build. I am running it on WinXP SC version
This looks like not a layout problem but a GFX font measurement problme on
Simplified Chinese version of WinXP . Probably not reproduceable on English
windows. (not sure.)

I try the following WINDOW build. and they DO NOT work neither:
20020506 branch build
20020503 branch build
20020424 branch build
20020402 branch build
20020319 build
20020311 build
6.2 20011003 0.9.4 branch build
6.1 rtm 0.9.2 20010726 build

The following build work
20020506 brach MacOS X build
6.0 build on window - 20001108 build

so it is a regression on SC WinXP from n6.0. But somehow we ship with it in n6.1
and n6.2 

reassign to shanjian to investigate. 
Assignee: attinasi → shanjian
same problme can be reproduced on 20020408 build on NT4 Ja
change from "nestging table text display porblem" to "problem with <td
align=right or align=center"

Summary: nestging table text display porblem → problem with <td align=right or align=center
Attached file simplified test cases
notice in the first two table, the last 'n' run out of the table boundary.
smontagu- can you also look at this one?
I see the same problem on En W2K with default locale set to Japanese.
Can reproduce it on 05-06 branch build / WinXP-SC as well as on Win2k-SC.
Can not reproduce this on WinME-JA.
Changing QA Contact
QA Contact: petersen → moied
Keywords: nsbeta1, topembed
topembed- since this is not reproducible by QA.  please re-nominate again when
you're able to reproduce it.
Keywords: topembedtopembed-
Keywords: topembed-topembed+
I could reproduce it on my machine. It does not matter if windows is localized
or not, but you have to set default locale to be CJK. 

I checked string measurement yesterday, and under Englist locale and chinese
locale, font and width are exactly the same. I could not understand why the
result is different. Need more investigation.
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2]
Blocks: 141008
Keywords: intl
> It does not matter if windows is localized
>or not, but you have to set default locale to be CJK. 
currently, this is a window only bug, right ?

what do you mean "set default locale to be CJK"? do you mean change setting in
window's control panel ?

pay attention to gSystemLocale in nsFontMetricsWin.cpp 
The problem is because in some situations, langGroup is not considered and
locale langGroup is being used. I need to work out a more complete patch. 
Attached patch patch (obsolete) — Splinter Review
simon/waterson, could you give r/sr?
could it cause 143557?
does this patch fix 142607?
yes. This bug and that one may or may not be caused by the same code. My patch
fixed all such problems under layout/html/base/src directory. 
Are these the only places that need fixing out of all the places where we
measure text? 
It looks it does not impact 143557?
>>Are these the only places that need fixing out of all the places where >>we
measure text? 
Yes for HTML layout. No for xul. But since lang attribute is not implemented in
xul yet, we don't have to worry it right now. 
Comment on attachment 83566 [details] [diff] [review]

Attachment #83566 - Flags: review+
So it seems like it doesn't make sense to store the font metrics with the
rendering context anymore. There's one place you didn't change in
layout/html/base/src, namely in nsHTMLReflowState -- intentional? Also, what
about mathml and nsCaret.cpp?
If we need to go through this much code to get the right font (and do it in
multiple places), maybe the API should be changed so that this code can be in
one place?

(I'm trying to avoid the temptation of rehashing the discussion in bug 105199.)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #83566 - Attachment is obsolete: true
patch v2 change nsIRenderingContext API. Now SetFont takes an additional
langgroup parameter. I was hesitant to do so because so many files are involved.
To force people considering of language when setting font should be a good thing
to do. So I did it now. Together with the API change, the change to use language
group is more extensive and complete. 
Comment on attachment 83907 [details] [diff] [review]
patch v2

-NS_IMETHODIMP nsRenderingContextGTK::SetFont(const nsFont& aFont)
+NS_IMETHODIMP nsRenderingContextGTK::SetFont(const nsFont& aFont, , nsIAtom*

aFont, ,

+  if (aLangGroup)
+    rv = mContext->GetMetricsFor(aFont, aLangGroup,
+  else
+    rv = mContext->GetMetricsFor(aFont, *getter_AddRefs(newMetrics));

There isn't much point in having these two functions (whith the ensuing |if| in
several places).

already has a safeguard to default to the locale's langgroup. 

Since the point of the patch is to give more exposure to the langGroup, it
might be best to remove the other API althogher (while callers with |null| can
still recover the old stuff).

+static void SetFontFromStyle(nsIRenderingContext* aRC, nsIStyleContext* aSC) 

What about putting this handy function in nsFrame instead?
This way, people who later want to change the default |null| that you
have sprinkled around could call it.
-      deviceContext->GetMetricsFor(*plainFont, langGroup, mNormalFont);
-      aRenderingContext.SetFont(mNormalFont); // some users of the struct
expect this state
+      SetFontFromStyle(&aRenderingContext, sc);
+      aRenderingContext.GetFontMetrics(mNormalFont);

It also helps to retain the above comment (or a variant thereof). I had added
earlier it because I once fall in the trap of thinking that the font that is set
in the RC is just for the purpose of getting |mNormalFont| & |mSpaceWidth|, and
that another font could override it after that. Whereas there is a futher nuance
in that some callers actually assume |mNormalFont| to also be the current font
in the RC upon existing the constructor of the nsTextStyle struct where this
call is being made.
shanjiang take off today and monday. will be back at tuesday
I can reproduce this problem on linux RH7.2-JA.
Attached patch patch v3 (obsolete) — Splinter Review
Moved handy function SetFontFromStyle to nsFrame.cpp so that all other files in
layout/html/base/src can benefit from this function. Redundant checking in
nsRenderingContext is removed.
rbs, could you review my new patch?
Comment on attachment 84495 [details] [diff] [review]
patch v3

-NS_IMETHODIMP nsRenderingContextGTK::SetFont(const nsFont& aFont)
+NS_IMETHODIMP nsRenderingContextGTK::SetFont(const nsFont& aFont, , nsIAtom*

Typo still there in...
Index: gfx/src/gtk/nsRenderingContextGTK.cpp
Index: gfx/src/photon/nsRenderingContextPh.cpp

Index: layout/html/base/src/nsFrame.cpp
+void SetFontFromStyle(nsIRenderingContext* aRC, nsIStyleContext* aSC) 
+  aRC->SetFont(font->mFont, langGroup);

Since this is now going to be a heavily used function, let's add a
null-check because it has been observed that ReResolution of style data
is not yet fail-safe & some weird things sometimes happen with the
Style System:
+  if (font)
+    aRC->SetFont(font->mFont, langGroup);

Index: layout/html/base/src/nsInlineFrame.cpp
+    nsCOMPtr<nsIStyleContext> styleContext;
+    GetStyleContext(getter_AddRefs(styleContext));
+    SetFontFromStyle(aReflowState.rendContext, styleContext);
+    aReflowState.rendContext->GetFontMetrics(*getter_AddRefs(fm));

To save some cycles, I will just do:
SetFontFromStyle(aReflowState.rendContext, mStyleContext);

In nsIDeviceContext, there isn't much point in keeping these two now.
I suggest to remove the second one which has ceased to be relevant.

  NS_IMETHOD GetMetricsFor(const nsFont& aFont, nsIAtom* aLangGroup,
			    nsIFontMetrics*& aMetrics) = 0;
  NS_IMETHOD GetMetricsFor(const nsFont& aFont, nsIFontMetrics*& aMetrics) = 0;

r=rbs applies from now on.
@@ -633,7 +629,7 @@
     aMetrics.ascent +=;
     aMetrics.descent += aReflowState.mComputedBorderPadding.bottom;
     aMetrics.height += +
-      aReflowState.mComputedBorderPadding.bottom;
+    aReflowState.mComputedBorderPadding.bottom;

Need to put back the indentation since it is a continuation.
Comment on attachment 84495 [details] [diff] [review]
patch v3

Attachment #84495 - Flags: review+
Attachment #84495 - Flags: review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #83907 - Attachment is obsolete: true
Attachment #84495 - Attachment is obsolete: true
took rbs's suggestion. But DeviceContext interface was not changed, that's
because GetMetricsFor was used in several other modules. I don't want to
complicate this patch any more. It can be done in the time when we add language
attribute support to xul.
Comment on attachment 84652 [details] [diff] [review]
patch v4

take rbs's review
Attachment #84652 - Flags: review+
Mr. waterson, could you sr?
Comment on attachment 84652 [details] [diff] [review]
patch v4

I like it: nice work shanjian. A couple nits below.

Index: gfx/src/mac/nsRenderingContextMac.cpp
>-	if (mContext)
>-		mContext->GetMetricsFor(aFont, mGS->mFontMetrics);
>+  if (mContext) {
>+    mContext->GetMetricsFor(aFont, aLangGroup, mGS->mFontMetrics);
>+	}

This shouldn't be using hard tabs, but it does. Don't re-indent it.

>Index: gfx/src/photon/nsRenderingContextPh.cpp
>-NS_IMETHODIMP nsRenderingContextPh :: SetFont( const nsFont& aFont ) 
>+NS_IMETHODIMP nsRenderingContextPh :: SetFont( const nsFont& aFont, , nsIAtom* aLangGroup ) 

Umm, didn't rbs tell you about this twice already? :-)

>Index: gfx/src/qt/nsRenderingContextQT.cpp
>-  nsresult rv = mContext->GetMetricsFor(aFont,*getter_AddRefs(newMetrics));
>+  nsresult rv = mContext->GetMetricsFor( aFont, aLangGroup, *getter_AddRefs(newMetrics) );

Don't enforce your style here. Keep it consistent.

>Index: layout/html/base/src/nsFrame.cpp
>+// a handy utility to set font
>+void SetFontFromStyle(nsIRenderingContext* aRC, nsIStyleContext* aSC) 
>+  const nsStyleFont *font = (const nsStyleFont*)
>+    aSC->GetStyleData(eStyleStruct_Font);
>+  const nsStyleVisibility* visibility = (const nsStyleVisibility*) 
>+    aSC->GetStyleData(eStyleStruct_Visibility);

Why whould |font| or |visibility| ever be null? (They shouldn't be. So
assert if they are.) Also, re-organize this logic: if you can't get the
|font|, why bother trying to get the |visibility|?

>+  nsCOMPtr<nsIAtom> langGroup;
>+  if (visibility && visibility->mLanguage) {
>+    visibility->mLanguage->GetLanguageGroup(getter_AddRefs(langGroup));
>+  }
>+  if (font)
>+    aRC->SetFont(font->mFont, langGroup);
>+// end handy utility

Remove this comment. It's obvious that it is the end of the function. :-)
Attachment #84652 - Flags: needs-work+
(Also, please run the layout regression tests!)
Attached patch patch v5Splinter Review
Attachment #84652 - Attachment is obsolete: true
Attachment #84810 - Flags: review+
chris, could you take one more look?
Comment on attachment 84810 [details] [diff] [review]
patch v5

sr=waterson, assuming layout regression tests pass.
Attachment #84810 - Flags: superreview+
ask for both driver and adt1.0.0+
Keywords: adt1.0.0, approval
I did layout regression test, no problem spotted.

fix checked into trunk.
Closed: 22 years ago
Resolution: --- → FIXED
missed a file.
Resolution: FIXED → ---
Attached patch addition patchSplinter Review
fix checked in. 
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Verified fixed with trunk build ID 20020529 using winxp
Do you need to get reviews on the additional patch, or were they carried over
from patch v5?
The addition patch is basically a missing spot of API change. In patch v5, we
have many other similar changes made. Because it is nothing new,  I don't think
we need to go over r/sr for it. 
Comment on attachment 84952 [details] [diff] [review]
addition patch

carrying over r/sr from previous patch, it is just a missing spot.
Attachment #84952 - Flags: superreview+
Attachment #84952 - Flags: review+
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Driver's approval. Pls check this in asap. thanks!
Blocks: 143047
Whiteboard: [adt2] → [adt2 RTM]
Keywords: adt1.0.1
Comment on attachment 84810 [details] [diff] [review]
patch v5

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword
Attachment #84810 - Flags: approval+
Target Milestone: --- → mozilla1.0.1
patch checked into branch. 
Blocks: 146292
No longer blocks: 141008
removing extra adt1.0.1 from keywords.
Keywords: adt1.0.1
Verified with branch build 20020731 on winxp, adding KW:verified1.0.1
Keywords: verified1.0.1
You need to log in before you can comment on or make changes to this bug.