text size change causes bad table layout [font zoom; changing Fonts prefs]

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: sean, Assigned: alexsavulov)

Tracking

({access})

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: se-radar, )

Attachments

(6 attachments)

Reporter

Description

18 years ago
Text zoom is not revertable at http://www.mozilla.org. 
Press ctrl+= and note that the layout gets whacked.
Press ctrl+- to revert the change and note that the layout is still whacked.

Built from the tip around noon today.
Whiteboard: DUPEME

Comment 1

18 years ago
Over to HTML Tables.
Assignee: asa → karnaze
Component: Browser-General → HTMLTables
QA Contact: doronr → amar

Comment 2

18 years ago
*** Bug 82578 has been marked as a duplicate of this bug. ***

Comment 3

18 years ago
Setting OS=All as the duplicate bug 82578 is on Linux.
OS: Windows 2000 → All
*** Bug 82932 has been marked as a duplicate of this bug. ***
Reporter

Comment 5

18 years ago
improving summary (was: viewzoom doesn't work correctly)
Summary: viewzoom doesn't work correctly → font size change causes bad table layout
I don't want to nitpick, but since the menu option is called 'text size' and not
'font size', it would be logical to reflect this in the summary.. :-)

Comment 7

18 years ago
I agree. Updating summary.
Summary: font size change causes bad table layout → text size change causes bad table layout
Whiteboard: DUPEME

Comment 8

18 years ago
Actually 'font size' is also correct, as the same effect can be observed after
changing font size in Prefs.
*** Bug 83326 has been marked as a duplicate of this bug. ***
From bug 83326: Reloading fixes the layout. Probably the only thing that is needed is making the layout do a 
full reflow with the new metrics after zooming.

Comment 11

18 years ago
*** Bug 83706 has been marked as a duplicate of this bug. ***
Adding the obvious search string to the summary in order to ease duplicate
searching.
Summary: text size change causes bad table layout → text size change causes bad table layout [font zoom]
*** Bug 81099 has been marked as a duplicate of this bug. ***

Comment 14

18 years ago
Adding 'mostfreq' keyword as there are now 7 dupes.
Keywords: mostfreq
*** Bug 84943 has been marked as a duplicate of this bug. ***
*** Bug 85393 has been marked as a duplicate of this bug. ***

Comment 17

18 years ago
*** Bug 85378 has been marked as a duplicate of this bug. ***
just curious: what's the status of a fix for this bug? thx!
Summary: text size change causes bad table layout [font zoom] → text size change causes bad table layout [font zoom; changing Fonts prefs]

Comment 19

18 years ago
*** Bug 85493 has been marked as a duplicate of this bug. ***

Comment 20

18 years ago
*** Bug 80757 has been marked as a duplicate of this bug. ***

Comment 21

18 years ago
*** Bug 85918 has been marked as a duplicate of this bug. ***
Assignee

Comment 22

18 years ago
Posted file reduced test case
Assignee

Comment 23

18 years ago
the problem we have here is the CSS property "white-space" set to "nowrap"

(from the stile sheet:)

.linkcell { 
    white-space: nowrap;
}


this is causing the table cell to keep the size even after the fontsize 
decreases

Comment 24

18 years ago
*** Bug 87294 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Keywords: mozilla0.9.3

Comment 25

18 years ago
*** Bug 79070 has been marked as a duplicate of this bug. ***

Comment 26

18 years ago
Test URL from 79070: <http://html-color-codes.com/>

Adding access to keywords from 79070 as this will be noticeable by users with
poor vision.

This bug seems to only occur when the font size is reduced, behavior when font
size is increased is normal.
Keywords: access
*** Bug 86311 has been marked as a duplicate of this bug. ***
Assignee

Comment 29

18 years ago
Need SR (RS) and R for this.

Also tested the with fixed width ( e.g. width : 100px) so the part of the branch 
that comes before "else" works and doesn't have to be changed.

Comment 30

18 years ago
CC'ing Bernd for his review (karnazer is out and Bernd is a table guru in his
own right!)

Comment 31

18 years ago
[s]r=attinasi for the patch.

Comment 32

18 years ago
This patch is wrong. no r from me. It reverts the changes to fix bug 57828
(remember lxr is your friend :-)) The tests has obviously not seen the table
regression tests
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/rtest.bat
, it breaks
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug57828.html

there may be other testcases broken by this patch. I did not execute the
regression test I simply took the guerilla (lxr) approach to verify the patch.
In order to be accepted, a patch has to pass the regression tests, if it does
not pass the differences should be listed and explained. (or in short no 'r'
without 'r'test)
Marc, thanks for the compliment my reading is:
table guru == has no clue in other mozilla code areas.
Assignee

Comment 33

18 years ago
Bernd is right! Forget id=40571.
Hmmm.... I spent a lot of time last week to get a clean regression test using 
the unmodifyed engine twice, but didn't work ( got 5MB files after running 
verify) so I wasn't able to do a regression test yet. But see, Bernd pointing me 
to bug 57828 solved my problem very fast. Ok... forget the patch. Thx Bernd.

Comment 34

18 years ago
just comment out 
http://lxr.mozilla.org/mozilla/source/layout/base/src/nsFrameUtil.cpp#663
#665 or set outputregression != 0

There is viewer command line option for this, but I took the brutal way :-)
Whiteboard: se-radar

Comment 35

18 years ago
*** Bug 86022 has been marked as a duplicate of this bug. ***
Assignee

Comment 37

18 years ago
attachment 42797 [details] [diff] [review] is to be reviewed. 

I changed the block code, made regression testing and I did not see any 
problems during regression (except 14929 - see below). However, I need reviews 
for this be cause there is a lot that might change what I'm not able to see at 
the first sight. So please, everybody that has interrests in this take a look at 
it and maybe you have some knowledge about why should I aply or not this patch. 

The patch is not proposed yet for deployment, is just an ideea.

The change in the html.css causes a small problem with the regression sample for 
bug 14929, but it repairs the behaviour of the last two tabels of regression 
sample for 57828 making mozilla act like IE. Seems to me that this is another 
issue that should be handled sepparately.

Ok volks, need your oppinion!
THX in advance!
Assignee

Comment 38

18 years ago
BTW ... in attachment 42797 [details] [diff] [review] the change in nsInlineFrame.cpp was unintentionaly 
made... sorry 'bout that

Comment 39

18 years ago
Alexandru could you attach a screen shot of the problem in bug 14929
Assignee

Comment 41

18 years ago
reassigning to me sicnce i spent already a lot of time with it
Assignee: karnaze → alexsavulov
Selectors such as td[nowrap] and th[nowrap] do not perform well.  Attribute
mapping code is likely to be faster, I think.  (I'm not 100% sure, though. 
Maybe it's close enough to the same now.)
Assignee

Comment 43

18 years ago
Assignee

Comment 44

18 years ago
The last two tables in regression test sample for bug 57828 are diplayed wrong 
anyway so the patch does not regress 57828. I will open another bug about this 
issue. Need to see if I can do a code patch instead of changing the selectors 
for td and th in html.css.

Need SR + R for patch please! 
Thx!
Assignee

Comment 45

18 years ago
sorry! forget the patch 43605! i have to review it myself!
Damn! Something screwed the things up! Sorry bout that!
It seems to me like the changes to the block code consist of:
 1 & 2. for the nowrap case, setting the computed width to the max element size
    (even though for nowrap they should be the same) and not setting the max
    element size to anything
 3. correcting the uninitialized max element size that you didn't set in parts
    1 & 2 of the patch.

What did this really change, and how does it fix the problem?  Was the max
element size set incorrectly before?  Why?
It was suggested on bug 93161 that the correct fix here might be the following:

Index: nsBlockFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
retrieving revision 3.448
diff -u -d -r3.448 nsBlockFrame.cpp
--- nsBlockFrame.cpp    2001/07/19 23:32:06     3.448
+++ nsBlockFrame.cpp    2001/08/03 17:38:24
@@ -1188,9 +1188,7 @@
         // When no-wrap is true the max-element-size.width is the
         // width of the widest line plus the right border. Note that
         // aState.mKidXMost already has the left border factored in
-        //maxWidth = aState.mKidXMost + borderPadding.right;
-        maxWidth = aState.mMaxElementSize.width +
-          borderPadding.left + borderPadding.right;
+        maxWidth = aState.mKidXMost + borderPadding.right;
       }
       else {
         // Add in border and padding dimensions to already computed
This change certainly makes sense, anyway.
Assignee

Comment 49

18 years ago
Yeah, I know, I think I suggested that :-) Thx for your help David!
Assignee

Updated

18 years ago
Blocks: 89731
Assignee

Updated

18 years ago
Blocks: 93408
Assignee

Comment 51

18 years ago
verbal r= from Peter L.

Comment 52

18 years ago
Alex, I know about the change to nsTableCellFrame. The change to nsBlockFrame 
seems consistent with the comment about the left border. Assumming that it 
passes the block and table regression tests, r=karnaze.

Comment 53

18 years ago
buster cited bugs 38396, 29429, 32471, 27390, 32581 when he commented out the
line in nsBlockFrame.cpp. Have you verified that your changes don't regress
these problems? If so, sr=waterson

Assignee

Updated

18 years ago
No longer blocks: 93408
Assignee

Updated

18 years ago
No longer blocks: 89731
Assignee

Comment 54

18 years ago
marked as fixed
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
 This bug is fixed.. Verified on Build ID: 2001081504 Platform : WIN2K
 
Status: RESOLVED → VERIFIED

Comment 56

18 years ago
Checked into MOZILLA_0_9_2_BRANCH
You need to log in before you can comment on or make changes to this bug.