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)

defect

Tracking

()

VERIFIED FIXED
mozilla1.4alpha

People

(Reporter: nnbugzilla, Assigned: dbaron)

References

()

Details

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

Attachments

(5 files, 5 obsolete files)

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) (deleted) —
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) (deleted) —
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) (deleted) — 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.
Attachment #112880 - Attachment is obsolete: true
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) (deleted) — 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.
This shows how Mozilla displays attachment 112912 [details] before attachment 112895 [details] [diff] [review] (right) and after attachment 112895 [details] [diff] [review] (left).
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).
Blocks: 102100
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) (deleted) — 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 (deleted) — 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. ***
Blocks: 193809
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. ***
Blocks: 178430
*** Bug 156629 has been marked as a duplicate of this bug. ***
Blocks: 146851
*** 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: