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)
Core
Layout: Block and Inline
Tracking
()
VERIFIED
FIXED
mozilla1.4alpha
People
(Reporter: nnbugzilla, Assigned: dbaron)
References
()
Details
(Keywords: regression, testcase, Whiteboard: [patch])
Attachments
(5 files, 5 obsolete files)
11.77 KB,
text/html
|
Details | |
1.27 KB,
text/html
|
Details | |
27.55 KB,
image/png
|
Details | |
11.13 KB,
image/png
|
Details | |
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. ***
Assignee | ||
Comment 4•22 years ago
|
||
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.
Assignee | ||
Comment 5•22 years ago
|
||
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
Assignee | ||
Comment 6•22 years ago
|
||
Assignee | ||
Comment 7•22 years ago
|
||
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.
Comment 8•22 years ago
|
||
*** Bug 186779 has been marked as a duplicate of this bug. ***
Comment 9•22 years ago
|
||
Is this bug a duplicate of bug 126774 ?
Comment 11•22 years ago
|
||
*** Bug 186944 has been marked as a duplicate of this bug. ***
Comment 12•22 years ago
|
||
Comment 13•22 years ago
|
||
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
Assignee | ||
Comment 14•22 years ago
|
||
Does IE display those two above each other?
Comment 15•22 years ago
|
||
yes, as opera and phoenix 0.4
Assignee | ||
Comment 16•22 years ago
|
||
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.
Assignee | ||
Comment 17•22 years ago
|
||
The patch fixes attachment 110089 [details] but doesn't change our behavior on attachment
112880 [details], which I now realize is an unrelated issue.
Assignee | ||
Comment 18•22 years ago
|
||
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.)
Assignee | ||
Comment 19•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #112880 -
Attachment is obsolete: true
Assignee | ||
Comment 20•22 years ago
|
||
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.
Assignee | ||
Comment 21•22 years ago
|
||
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
Assignee | ||
Comment 22•22 years ago
|
||
I'd appreciate a screenshot of this in WinIE6.
Assignee | ||
Comment 23•22 years ago
|
||
This shows how Mozilla displays attachment 112912 [details] before attachment 112895 [details] [diff] [review]
(right) and after attachment 112895 [details] [diff] [review] (left).
Comment 24•22 years ago
|
||
Assignee | ||
Comment 25•22 years ago
|
||
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).
Assignee | ||
Comment 26•22 years ago
|
||
Taking bug.
Assignee: block-and-inline → dbaron
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.4alpha
Assignee | ||
Comment 27•22 years ago
|
||
This patch manages to break http://www.tantek.com/projects/resume.html rather
badly (the negative margins on the floating dates don't work).
Assignee | ||
Comment 28•22 years ago
|
||
This fixes the bug on Tantek's resume. (It was just a typo in the
ReflowFloater cleanup.)
Attachment #112895 -
Attachment is obsolete: true
Assignee | ||
Comment 29•22 years ago
|
||
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?
Assignee | ||
Comment 30•22 years ago
|
||
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.
Assignee | ||
Comment 31•22 years ago
|
||
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|.
Assignee | ||
Updated•22 years ago
|
Attachment #112988 -
Flags: superreview?(roc+moz)
Attachment #112988 -
Flags: review?(roc+moz)
Assignee | ||
Comment 32•22 years ago
|
||
(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.
Assignee | ||
Comment 34•22 years ago
|
||
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.
Comment 36•22 years ago
|
||
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.
Comment 37•22 years ago
|
||
*** 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+
Assignee | ||
Comment 40•22 years ago
|
||
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+
Comment 42•22 years ago
|
||
*** Bug 192928 has been marked as a duplicate of this bug. ***
Comment 43•22 years ago
|
||
*** Bug 193176 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 44•22 years ago
|
||
Fix checked in to trunk, 2003-02-22 08:19 PST.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 45•22 years ago
|
||
*** Bug 194496 has been marked as a duplicate of this bug. ***
Comment 46•22 years ago
|
||
*** Bug 194561 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 47•22 years ago
|
||
This does indeed seem to be fixed with build 2003022408.
Assignee | ||
Comment 48•22 years ago
|
||
*** Bug 102100 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 49•22 years ago
|
||
*** Bug 156629 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 50•22 years ago
|
||
*** Bug 186828 has been marked as a duplicate of this bug. ***
Comment 51•22 years ago
|
||
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.
Assignee | ||
Comment 52•22 years ago
|
||
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
Assignee | ||
Comment 53•22 years ago
|
||
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.)
Comment 54•22 years ago
|
||
mere mortal users will be grilled in table hell as soon as they understand what
mew means.
Comment 55•22 years ago
|
||
*** Bug 197258 has been marked as a duplicate of this bug. ***
Comment 56•22 years ago
|
||
*** Bug 197273 has been marked as a duplicate of this bug. ***
Comment 57•22 years ago
|
||
*** Bug 197358 has been marked as a duplicate of this bug. ***
Comment 58•22 years ago
|
||
*** Bug 196539 has been marked as a duplicate of this bug. ***
Comment 59•22 years ago
|
||
*** Bug 197444 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 60•22 years ago
|
||
*** Bug 198623 has been marked as a duplicate of this bug. ***
Comment 61•22 years ago
|
||
*** Bug 146851 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 62•22 years ago
|
||
*** Bug 125108 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 63•22 years ago
|
||
*** Bug 199962 has been marked as a duplicate of this bug. ***
Comment 64•22 years ago
|
||
*** Bug 201621 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 66•22 years ago
|
||
*** Bug 188250 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 67•21 years ago
|
||
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).
Assignee | ||
Comment 68•21 years ago
|
||
*** 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.
Description
•