142562 - problem with <td align=right or align=center

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Frank Tang]

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.

Comment 1

[Frank Tang]

Attachment: screenshot

screenshot

Comment 2

[Chris Waterson]

This WFM on the trunk. Do you have any mods in your tree, ftang?

Comment 3

[Frank Tang]

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

Comment 4

[Frank Tang]

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. 

Comment 5

[Frank Tang]

same problme can be reproduced on 20020408 build on NT4 Ja

Comment 6

[Frank Tang]

change from "nestging table text display porblem" to "problem with <td
align=right or align=center"

Comment 7

[Frank Tang]

Attachment: simplified test cases

Comment 8

[Frank Tang]

Attachment: screenshot of the simplified test cases

notice in the first two table, the last 'n' run out of the table boundary.

Comment 9

[Frank Tang]

smontagu- can you also look at this one?

Comment 10

[ji]

I see the same problem on En W2K with default locale set to Japanese.

Comment 11

[Yuying Long]

Can reproduce it on 05-06 branch build / WinXP-SC as well as on Win2k-SC.

Comment 12

[Yuying Long]

Can not reproduce this on WinME-JA.

Comment 13

[Laura Krietemeyer]

Changing QA Contact

Comment 14

[Winnie Lam]

topembed- since this is not reproducible by QA.  please re-nominate again when
you're able to reproduce it.

Comment 15

[Shanjian Li]

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. 

Comment 16

[Shanjian Li]

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.

Comment 17

[Frank Tang]

> 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 

Comment 18

[Shanjian Li]

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. 

Comment 19

[Shanjian Li]

Attachment: patch

Comment 20

[Shanjian Li]

simon/waterson, could you give r/sr?

Comment 21

[Frank Tang]

could it cause 143557?

Comment 22

[Frank Tang]

does this patch fix 142607?

Comment 23

[Shanjian Li]

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. 

Comment 24

[Simon Montagu :smontagu]

Are these the only places that need fixing out of all the places where we
measure text? 

Comment 25

[Frank Tang]

It looks it does not impact 143557?

Comment 26

[Shanjian Li]

>>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 27

[Simon Montagu :smontagu]

Comment on attachment 83566 [details] [diff] [review]
patch

r=smontagu

Comment 28

[Chris Waterson]

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?

Comment 29

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

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

Comment 30

[Shanjian Li]

Attachment: patch v2

Comment 31

[Shanjian Li]

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 32

[rbs]

Comment on attachment 83907 [details] [diff] [review]
patch v2

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

aFont, ,
     ^^^

+  if (aLangGroup)
+    rv = mContext->GetMetricsFor(aFont, aLangGroup,
*getter_AddRefs(newMetrics));
+  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).

DeviceContextImpl::GetMetricsFor()
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.

Comment 33

[rbs]

-      deviceContext->GetMetricsFor(*plainFont, langGroup, mNormalFont);
-      aRenderingContext.SetFont(mNormalFont); // some users of the struct
expect this state
+      SetFontFromStyle(&aRenderingContext, sc);
+      aRenderingContext.GetFontMetrics(mNormalFont);
       mNormalFont->GetSpaceWidth(mSpaceWidth);

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.

Comment 34

[rbs]

s/existing/exiting/

Comment 35

[Frank Tang]

shanjiang take off today and monday. will be back at tuesday

Comment 36

[Yuying Long]

I can reproduce this problem on linux RH7.2-JA.

Comment 37

[Shanjian Li]

Attachment: patch v3

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.

Comment 38

[Shanjian Li]

rbs, could you review my new patch?

Comment 39

[rbs]

Comment on attachment 84495 [details] [diff] [review]
patch v3

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

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);
aReflowState.rendContext->GetFontMetrics(*getter_AddRefs(fm));


===================================================================
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.

Comment 40

[rbs]

@@ -633,7 +629,7 @@
     aMetrics.ascent += aReflowState.mComputedBorderPadding.top;
     aMetrics.descent += aReflowState.mComputedBorderPadding.bottom;
     aMetrics.height += aReflowState.mComputedBorderPadding.top +
-      aReflowState.mComputedBorderPadding.bottom;
+    aReflowState.mComputedBorderPadding.bottom;

Need to put back the indentation since it is a continuation.

Comment 41

[Shanjian Li]

Comment on attachment 84495 [details] [diff] [review]
patch v3

r=shanjian

Comment 42

[Shanjian Li]

Attachment: patch v4

Comment 43

[Shanjian Li]

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 44

[Shanjian Li]

Comment on attachment 84652 [details] [diff] [review]
patch v4

take rbs's review

Comment 45

[Shanjian Li]

Mr. waterson, could you sr?

Comment 46

[Chris Waterson]

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

Comment 47

[Chris Waterson]

(Also, please run the layout regression tests!)

Comment 48

[Shanjian Li]

Attachment: patch v5

Comment 49

[Shanjian Li]

chris, could you take one more look?

Comment 50

[Chris Waterson]

Comment on attachment 84810 [details] [diff] [review]
patch v5

sr=waterson, assuming layout regression tests pass.

Comment 51

[Frank Tang]

ask for both driver and adt1.0.0+

Comment 52

[Shanjian Li]

I did layout regression test, no problem spotted.

Comment 53

[Shanjian Li]

fix checked into trunk.

Comment 54

[Shanjian Li]

missed a file.

Comment 55

[Shanjian Li]

Attachment: addition patch

Comment 56

[Shanjian Li]

fix checked in. 

Comment 57

[Moied]

Verified fixed with trunk build ID 20020529 using winxp

Comment 58

[Jaime Rodriguez, Jr.]

Do you need to get reviews on the additional patch, or were they carried over
from patch v5?

Comment 59

[Shanjian Li]

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 60

[Shanjian Li]

Comment on attachment 84952 [details] [diff] [review]
addition patch

carrying over r/sr from previous patch, it is just a missing spot.

Comment 61

[Jaime Rodriguez, Jr.]

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!

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 63

[Shanjian Li]

patch checked into branch. 

Comment 64

[Myk Melez [:myk] [@mykmelez]]

Checking this fix into the Mozilla 1.0.1 branch broke BeOS.  It looks like the
same thing happened on the trunk and seawood fixed it like so:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fgfx&file=&filetype=match&who=seawood%25netscape.com&whotype=match&sortby=Date&hours=2&date=week&mindate=05%2F24%2F2002+20%3A00&maxdate=05%2F24%2F2002+20%3A00&cvsroot=%2Fcvsroot

Comment 65

[Jaime Rodriguez, Jr.]

removing extra adt1.0.1 from keywords.

Comment 66

[Moied]

Verified with branch build 20020731 on winxp, adding KW:verified1.0.1