Closed Bug 186593 Opened 22 years ago Closed 22 years ago

change max-element-width (minimum width) calculation for floaters

Categories

(Core :: Layout: Block and Inline, defect, P1)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: nnbugzilla, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: regression, testcase, Whiteboard: [patch])

Attachments

(5 files, 5 obsolete files)

partially simplified testcase from www.dpreview.com
11.77 KB, text/html
Details
basic testcase for shrink-wrapping and floats
1.27 KB, text/html
Details
Mozilla before/after screenshots of attachment 112912
27.55 KB, image/png
Details
IE6 Screenshot of attachment 112912 - 1280x1024
11.13 KB, image/png
Details
patch
29.27 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review 
To Reproduce:
Visit http://www.dpreview.com.
See the text and images run over the right "border".

Expected:
Text and images should fit within the borders.

The site seems to layout just fine with NS 7.01, and also with older builds of
Mozilla.  This seems to have started only recently.  Build 2002121308 doesn't
have this problem, but today's build (2002122308) does.
*** Bug 186631 has been marked as a duplicate of this bug. ***
David, seems that the NS_BLOCK_WRAP_SIZE chillout begins
Attached file testcase (obsolete) — 

    
  
Keywords: testcase

    
    

    
  
In the old code, this was an incremental reflow bug (try doing text zoom in and
out, and notice that the layout changes to the new layout).  My changes made the
initial reflow
 * consistent with later incremental reflows, and
 * consistent with what we do for left floats.

I've only looked at the testcase so far, though.

    
    

    
  
Comment on attachment 110067 [details]
testcase

This testcase doesn't seem like a correct simplification of the URL in
question.

        Attachment #110067 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 110089 [details]
partially simplified testcase from www.dpreview.com

Even this testcase has weird problems before my changes:  if you zoom the text
quite small, you'll see the problem, and it won't go away as you make the text
larger again until you get quite a bit above the threshhold that triggered the
problem.

My inclination is to postpone this bug until we separate intrinsic width
calculation from reflow.

    
    

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

    
    

    
  
Is this bug a duplicate of bug 126774 ?

    
    

    
  
Dupes from Mac & Linux -> OS All
OS: Windows 98 → All
Hardware: PC → All

    
    

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

    
    

    
  

      
      
      
      

        Attached file
          
        testcase (obsolete)
        — 
      

    


  

    
    

    
  
the mew in the testcase shold be just 200px + border + padding but not 2 times it
Assignee: table → block-and-inline
Component: Layout: Tables → Layout: Block & Inline
Keywords: regression
QA Contact: amar → ian
Summary: Table contents go past borders → nested right floaters cause wrong mew

    
    

    
  
Does IE display those two above each other?

    
    

    
  
yes, as opera and phoenix 0.4 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (untested) (obsolete)
        — 
          — Splinter Review
      

    


  
This is an untested patch that changes our approach to the way floaters affect
max-element-width.  It reduces the max-element-width by no longer requiring
that floaters (in most cases, although we didn't get it right in all cases) had
to have something next to them.

This has been discussed a good bit in other bugs, such as bug 156629, bug
120087 comment 40, and bug 178430.

    
    

    
  
The patch fixes attachment 110089 [details] but doesn't change our behavior on attachment
112880 [details], which I now realize is an unrelated issue.

    
    

    
  
Ignore this patch.  The code that I commented saying that it should be
sufficient is not functioning.  (The testcase on bug 97383 doesn't work correctly.)

    
    

    
  
Comment on attachment 112880 [details]
testcase

I see no logic that could explain why this testcase should be displayed with
the boxes above each other unless the window is narrow.  (Did you try those
other browsers with left floaters instead?)  Please discuss this testcase
further on a separate bug, as it is unrelated to this bug.

    
    

    
  
Mmm.  The problem with the patch is that the mew isn't cached in a way that's
picked up by state recovery.  I either need to make that code ensure that the
mew of a floater is included in the mew of the line containing its placeholder
or that we do a separate pass in state recovery to recover floater mews.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes the state recovery issue but I still need to compare the testcases
in other bugs to browsers that I don't have, so don't expect me to make any
further progress on this.

        Attachment #112884 -
        Attachment is obsolete: true

    
    

    
  


  
I'd appreciate a screenshot of this in WinIE6.

    
    

    
  
So I think overall this patch makes us much closer to IE's behavior, although IE
has what I would say are clearly bugs that I definitely don't want to imitate
(in particular, massive differences between left/right floater handling in a
bunch of the cases).

    
    

    
  
Taking bug.
Assignee: block-and-inline → dbaron
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.4alpha

    
    

    
  
This patch manages to break http://www.tantek.com/projects/resume.html rather
badly (the negative margins on the floating dates don't work).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes the bug on Tantek's resume.	(It was just a typo in the
ReflowFloater cleanup.)

        Attachment #112895 -
        Attachment is obsolete: true

    
    

    
  
I went through the testcases in this bug and a bunch of the bugs linked to it. 
We're closer to WinIE in more cases than we're farther from it.  This patch will
make it so there's one, perhaps somewhat common, case in which we'll have tables
*narrower* than WinIE (as opposed to wider, like now), which will cause vertical
gaps where we push text that doesn't fit next to a float down.  This will only
happen for left floats -- see the first and seventh testcases in attachment
112912 [details] (and see the screenshots above).

I think this is an improvement (it improves compatibility on this bug, bug
102100, bug 120087, and bug 156629), but it will introduce instances of having
vertical gaps (in testcases like the simplified testcase in attachment 105253 [details] on
bug 178430, where this patch makes us push the rule-image below the floating
image, or the similar testcase on bug 102216 (where IE actually cuts off part of
the image)).  I'd rather not imitate WinIE fully since WinIE's behavior is
significantly different for right and left floaters and has additional bugs on
top of that.

Any thoughts?

    
    

    
  
I ran the regression tests with this patch.  The results are pretty even both ways:

The following pages change from being different from Windows IE to the
same as Windows IE (testing with 5.5):
http://lxr.mozilla.org/mozilla/source/layout/html/tests/block/bugs/97383.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/block/printing/150652.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/block/printing/163614.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug57828-2.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug97383.html?raw=1

The following pages change from being the same as Windows IE to
different from Windows IE (testing with 5.5):
http://lxr.mozilla.org/mozilla/source/layout/html/tests/block/bugs/97383-a.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug26553.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug57828.html?raw=1
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/printing/bug59280-1.html?raw=1

On the following we change from one reasonable behavior to another while
WinIE is hilariously buggy:
http://lxr.mozilla.org/mozilla/source/layout/html/tests/table/bugs/bug7113.html?raw=1

So I think the main advantages of this patch are:
 * it makes our behavior more internally consistent (it doesn't depend on
whether the float in question is in the same block as the chunk of text in question)
 * I think it eliminates some incremental reflow bugs we have (I need to find
bug numbers, though).
 * It moves us to a behavior that is more likely to be standardizable (it's
currently unspecified), and is (over the range of testcases) probably closer to
that of WinIE (which is quite buggy in some cases).

I'd like to land this once we open for 1.4alpha.

    
    

    
  
Oh, I forgot some of the most important advantages:
 * It simplifies the code in question significantly.
 * It moves us to a model that would be much easier to implement in a world
where min-intrinsic-width and max-intrinsic-width calculation were separate from
|Reflow|.

        Attachment #112988 -
        Flags: superreview?(roc+moz)

        Attachment #112988 -
        Flags: review?(roc+moz)

    
    

    
  
(The disadvantage, of course, is that it makes us different from older versions
of Mozilla.  However, I think this isn't a very strong disadvantage, since we're
almost definitely going to have to do that anyway when we move intrinsic width
calculations out of Reflow.)

    
    

    
  
Sounds good. During 1.4alpha we can see what kind of feedback we get. If
backwards compatibilty turns out to be more important than we thought, or if WG
feedback says that the standard is likely to go in a different direction, we can
revert.

It would be good to prognosticate a bit to see what standardization might be
done in this area. The *worst* thing we could do is to break backward
compatibility in one release and then break it again in some future release.

    
    

    
  
Well, I'm the one with the action item to develop rules for intrinsic width
calculation, so supposedly I have a better idea of what standardization is
likely to happen than anyone else.  I think the most likely possibilities are
that it stays undefined (but perhaps only undefined for boxes containing floats,
rather than all boxes) or that it follows the approach I'm implementing here.  I
think any other reasonable approach would be far too complicated to describe in
a standard, although maybe there's something I haven't thought of.

    
    

    
  
OK, so in that case, the risk that standarization might go in a different
direction is remote indeed :-). I'm sold.

    
    

    
  
I expect backwards compatibility to be the major issue, the expected bug report
will look like:

page <insert a url here> does not work anymore in moz1.4a. It did work in 1.3
and all previous versions. By the way IE and opera render the page as mozilla
did in the past.

If that happens, I would like to dismiss the bug because it was previously a bug
and we need to break that page in order to be compatible with CSSxx.xx. 
A solution where we need to admit that the only reason for the regression is a
internal structure change seems to me suboptimal.

So David, please try to add something to the specs that are currently developed.

Robert I don't believe that backout will be an option as soon as we go further
on the separation of the intrinsic width calculations from reflow.

    
    

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

    
    

    
  
bernd, I understand the concern. It's a very good point that if we do this, we
should evaluate the situation with respect to 'regressions' before we check in
major restructuring of MEW.

Still, we don't promise to maintain backward compatibility with things that the
spec is unclear on or leaves undefined, so I still think we can proceed with
caution.

    
    

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

Looks good. Just one question: why in "nscoord mew = rc.GetMaxElementWidth()
+		 aFloaterCache->mOffsets.left +
aFloaterCache->mOffsets.right;", did you switch from adding margins to adding
the relative positioning offsets?

        Attachment #112988 -
        Flags: superreview?(roc+moz)

        Attachment #112988 -
        Flags: superreview+

        Attachment #112988 -
        Flags: review?(roc+moz)

        Attachment #112988 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
Oops.  That was the other half of the search-replace error that I fixed
earlier.

        Attachment #112988 -
        Attachment is obsolete: true

    
    

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

Ah, good :-). r+sr=roc+moz

        Attachment #113832 -
        Flags: superreview+

        Attachment #113832 -
        Flags: review+
Blocks: 192928
No longer blocks: 192928

    
    

    
  
*** Bug 192928 has been marked as a duplicate of this bug. ***
Blocks: 193176

    
    

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

    
    

    
  
Fix checked in to trunk, 2003-02-22 08:19 PST.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

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

    
    

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

    
    

    
  
This does indeed seem to be fixed with build 2003022408.

    
    

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

    
    

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

    
    

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

    
    

    
  
Ought to amplify the summary to stem the tide of dupes. Mere mortal users don't
know what mew means or where to look it up.

    
    

    
  
The dupes are mostly older bugs, and very few of the bugs were filed with
testcases good enough to show the problems were the same anyway.
Summary: nested right floaters cause wrong mew → change max-element-width (minimum width) calculation for floaters

    
    

    
  
And anyway, one needs to understand what these terms are (or at least the
concepts -- some of them have many names) to tell if something is a duplicate of
this bug.  Any summary that could be understood by typical users would also
describe many bugs that aren't duplicates of this bug.  (See, for example, the
incorrect duplicates of bug 156629, which weren't even due to the summary.)

    
    

    
  
mere mortal users will be grilled in table hell as soon as they understand what
mew means.

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
verified fixed 
Status: RESOLVED → VERIFIED

    
    

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

    
    

    
  
FWIW, this caused existing bug 102216 and bug 178430 to be marked wontfix, and
regressions bug 196408 (->evang) and bug 207279 (which may be fixable, but isn't
easy).

    
    

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

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: