Closed Bug 80817 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: sean, Assigned: alexsavulov)

References

(
URL
)

Details

(Keywords: access, Whiteboard: se-radar)

Attachments

(6 files)

reduced test case
305 bytes, text/html
Details
repairs this wrong behaviour for NOWRAP table cells
606 bytes, patch
Details | Diff | Splinter Review
repairs 80817, helps getting rid of the additional code in nsTableCellFrame.cpp for 57828 (see following comment)
4.48 KB, patch
Details | Diff | Splinter Review
the difference caused by the change of td[nowrap] rule in html.css when runing regression for bug 14929 (file:///s|/mozilla/layout/html/tests/table/bugs/bug14929.htm)
28.03 KB, image/jpeg
Details
the patch - I need SR + R
3.32 KB, patch
Details | Diff | Splinter Review
proposed patch (need sr= and r=)
2.41 KB, patch
Details | Diff | Splinter Review 
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
Over to HTML Tables.
Assignee: asa → karnaze
Component: Browser-General → HTMLTables
QA Contact: doronr → amar
*** Bug 82578 has been marked as a duplicate of this bug. ***
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. ***
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.. :-)
I agree. Updating summary.
Summary: font size change causes bad table layout → text size change causes bad table layout
Whiteboard: DUPEME
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.
*** 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. ***
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. ***
*** Bug 85378 has been marked as a duplicate of this bug. ***
just curious: what's the status of a fix for this bug? thx!
Keywords: mozilla0.9.2, nsCatFood
Summary: text size change causes bad table layout [font zoom] → text size change causes bad table layout [font zoom; changing Fonts prefs]
*** Bug 85493 has been marked as a duplicate of this bug. ***
*** Bug 80757 has been marked as a duplicate of this bug. ***
*** Bug 85918 has been marked as a duplicate of this bug. ***
Attached file reduced test case — 

    
    

    
  
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

    
    

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

    
  
Keywords: mozilla0.9.3

    
    

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

    
    

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

    
    

    
  
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.

    
    

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

    
    

    
  
[s]r=attinasi for the patch.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
*** Bug 86022 has been marked as a duplicate of this bug. ***

    
    

    
  
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!

    
    

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

    
    

    
  
Alexandru could you attach a screen shot of the problem in bug 14929

    
    

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

    
    

    
  


  

    
    

    
  
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!

    
    

    
  
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.

    
    

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

    
  
Blocks: 89731

    
  
Blocks: 93408

    
    

    
  
verbal r= from Peter L.

    
    

    
  
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.

    
    

    
  
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



    
  
No longer blocks: 93408

    
  
No longer blocks: 89731

    
    

    
  
marked as fixed
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
 This bug is fixed.. Verified on Build ID: 2001081504 Platform : WIN2K
 
Status: RESOLVED → VERIFIED

    
    

    
  
Checked into MOZILLA_0_9_2_BRANCH

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: