Closed
Bug 142562
Opened 22 years ago
Closed 22 years ago
problem with <td align=right or align=center
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: ftang, Assigned: shanjian)
References
()
Details
(Keywords: intl, topembed+, Whiteboard: [adt2 RTM])
Attachments
(5 files, 4 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
shanjian
:
review+
waterson
:
superreview+
jesup
:
approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shanjian
:
review+
shanjian
:
superreview+
jesup
:
approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
screenshot
Comment 2•22 years ago
|
||
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
Reporter | ||
Comment 3•22 years ago
|
||
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
Reporter | ||
Comment 4•22 years ago
|
||
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
Reporter | ||
Comment 5•22 years ago
|
||
same problme can be reproduced on 20020408 build on NT4 Ja
Reporter | ||
Comment 6•22 years ago
|
||
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
Reporter | ||
Comment 7•22 years ago
|
||
Reporter | ||
Comment 8•22 years ago
|
||
notice in the first two table, the last 'n' run out of the table boundary.
Reporter | ||
Comment 9•22 years ago
|
||
smontagu- can you also look at this one?
Comment 10•22 years ago
|
||
I see the same problem on En W2K with default locale set to Japanese.
Comment 11•22 years ago
|
||
Can reproduce it on 05-06 branch build / WinXP-SC as well as on Win2k-SC.
Comment 12•22 years ago
|
||
Can not reproduce this on WinME-JA.
Reporter | ||
Updated•22 years ago
|
Comment 14•22 years ago
|
||
topembed- since this is not reproducible by QA. please re-nominate again when
you're able to reproduce it.
Updated•22 years ago
|
Assignee | ||
Comment 15•22 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•22 years ago
|
||
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.
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Comment 17•22 years ago
|
||
> 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
Assignee | ||
Comment 18•22 years ago
|
||
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.
Assignee | ||
Comment 19•22 years ago
|
||
Assignee | ||
Comment 20•22 years ago
|
||
simon/waterson, could you give r/sr?
Reporter | ||
Comment 21•22 years ago
|
||
could it cause 143557?
Reporter | ||
Comment 22•22 years ago
|
||
does this patch fix 142607?
Assignee | ||
Comment 23•22 years ago
|
||
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•22 years ago
|
||
Are these the only places that need fixing out of all the places where we
measure text?
Reporter | ||
Comment 25•22 years ago
|
||
It looks it does not impact 143557?
Assignee | ||
Comment 26•22 years ago
|
||
>>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•22 years ago
|
||
Comment on attachment 83566 [details] [diff] [review]
patch
r=smontagu
Attachment #83566 -
Flags: review+
Comment 28•22 years ago
|
||
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•22 years ago
|
||
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.)
Assignee | ||
Comment 30•22 years ago
|
||
Attachment #83566 -
Attachment is obsolete: true
Assignee | ||
Comment 31•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
- 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•22 years ago
|
||
s/existing/exiting/
Reporter | ||
Comment 35•22 years ago
|
||
shanjiang take off today and monday. will be back at tuesday
Comment 36•22 years ago
|
||
I can reproduce this problem on linux RH7.2-JA.
Assignee | ||
Comment 37•22 years ago
|
||
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.
Assignee | ||
Comment 38•22 years ago
|
||
rbs, could you review my new patch?
Comment 39•22 years ago
|
||
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•22 years ago
|
||
@@ -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.
Assignee | ||
Comment 41•22 years ago
|
||
Comment on attachment 84495 [details] [diff] [review]
patch v3
r=shanjian
Attachment #84495 -
Flags: review+
Assignee | ||
Updated•22 years ago
|
Attachment #84495 -
Flags: review+
Assignee | ||
Comment 42•22 years ago
|
||
Attachment #83907 -
Attachment is obsolete: true
Attachment #84495 -
Attachment is obsolete: true
Assignee | ||
Comment 43•22 years ago
|
||
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.
Assignee | ||
Comment 44•22 years ago
|
||
Comment on attachment 84652 [details] [diff] [review]
patch v4
take rbs's review
Attachment #84652 -
Flags: review+
Assignee | ||
Comment 45•22 years ago
|
||
Mr. waterson, could you sr?
Comment 46•22 years ago
|
||
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+
Comment 47•22 years ago
|
||
(Also, please run the layout regression tests!)
Assignee | ||
Comment 48•22 years ago
|
||
Attachment #84652 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #84810 -
Flags: review+
Assignee | ||
Comment 49•22 years ago
|
||
chris, could you take one more look?
Comment 50•22 years ago
|
||
Comment on attachment 84810 [details] [diff] [review]
patch v5
sr=waterson, assuming layout regression tests pass.
Attachment #84810 -
Flags: superreview+
Reporter | ||
Comment 51•22 years ago
|
||
ask for both driver and adt1.0.0+
Assignee | ||
Comment 52•22 years ago
|
||
I did layout regression test, no problem spotted.
Assignee | ||
Comment 53•22 years ago
|
||
fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 55•22 years ago
|
||
Assignee | ||
Comment 56•22 years ago
|
||
fix checked in.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 57•22 years ago
|
||
Verified fixed with trunk build ID 20020529 using winxp
Status: RESOLVED → VERIFIED
Comment 58•22 years ago
|
||
Do you need to get reviews on the additional patch, or were they carried over
from patch v5?
Assignee | ||
Comment 59•22 years ago
|
||
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.
Assignee | ||
Comment 60•22 years ago
|
||
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+
Comment 61•22 years ago
|
||
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•22 years ago
|
||
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+
Updated•22 years ago
|
Attachment #84952 -
Flags: approval+
Updated•22 years ago
|
Target Milestone: --- → mozilla1.0.1
Comment 64•22 years ago
|
||
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
Reporter | ||
Updated•22 years ago
|
Comment 66•22 years ago
|
||
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.
Description
•