Closed Bug 20582 Opened 25 years ago Closed 23 years ago

[PATCH] horiz auto margins not correct in presence of padding on parent

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: rob, Assigned: attinasi)

References

(
URL
)

Details

(5 keywords, Whiteboard: [bae:20011126][Hixie-P2] (high profile: css1 bug))

Attachments

(4 files)

little test case for what I think this bug report covers
(deleted), text/html
Details
Simple testcase (strict mode)
(deleted), text/html
Details
Simple and self explanatory testcase. Layout should be the same for the two inner blue boxes.
(deleted), text/html
Details
Suggested patch
(deleted), patch
dbaron
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
When useing padding-left & padding-right of 10% with the <BODY> tag, text is placed correctly, whilst <HR>'s look as if they have a padding-left of 5% and padding-right of 15%. Simple example at: http://library.lspace.org/~rob/padding-test.html Tested with M11 and 1999-12-01-09-M12 on i686-pc-linux-gnu
Assignee: troy → kipp
Also see http://www.co.uk.lspace.org/about-terry/ for an example where 1 of the rules is positioned correctly, but the others aren't. IE 4 & Netscape 4.7 have no problem with these btw.
Severity: trivial → normal
Updating to default Layout Assignee...kipp no longer with us :-(
Why are you re-reassing layout bugs? Do NOT touch layout bugs. The bugs are assigned to Kipp so they can stay neatly organized until we have a new owner for the block/inline code.
mass moving all Kipp's pre-beta bugs to M15. Nisheeth and I will prioritize these and selectively move high-priority bugs into M13 and M14.
Summary: <HR>rules are not obeying the CSS padding of the <BODY> → [BLOCK] <HR>rules are not obeying the CSS padding of the <BODY>
mine! mine mine mine! all mine! whoo-hoo!
Assignee: kipp → buster
If the right and left margins of the hr are set to 0, the hr appears in the right place. (As a result, the width of the hr is 100%. Setting width to 100% doesn't fix the placement, however.) This is on a Mac. I am assuming this bug affects all platforms.
moving all buster m15 bugs to m16.
Target Milestone: M15 → M16
Status: NEW → ASSIGNED
re-assigning some HR bugs to Rod. Thanks, Rod!
Assignee: buster → rods
Status: ASSIGNED → NEW
moving to M18
Target Milestone: M16 → M18
Status: NEW → ASSIGNED
reassigning back to you (buster) its core layout work and it appears that most of these aren't HR frame issues, but are block issues
Assignee: rods → buster
Status: ASSIGNED → NEW
Is anyone working on this? or are you just tossing it over the fence at each other?
Adding the css1 keyword.
Keywords: css1
redistributing bugs across future milestones, sorry for the spam
Target Milestone: M18 → M19
Status: NEW → ASSIGNED
Priority: P3 → P2
Marking 4xp as this works in Nav4 & IE5 and therefore is likely to be relatively widely used and we don't want to break it. Nom. nsbeta3, recc. nsbeta3+.
Keywords: 4xp, nsbeta3
See also bug 38370, which may fix this.
Blocks: 38370
OS: Linux → All
Hardware: PC → All
Summary: [BLOCK] <HR>rules are not obeying the CSS padding of the <BODY> → <HR> rules are not obeying the CSS padding of the <BODY> [BLOCK]
Keywords: relnote2
correctness, backward compat., xbrowser compat.
Keywords: correctness
Whiteboard: [nsbeta3-]
Taking a stab at prioritizing buster's nsbeta3 bugs...
*** Bug 47803 has been marked as a duplicate of this bug. ***
I expected this bug to be plussed, not minused. Nominating for RTM.
Keywords: rtm
rtm is for final last minute crasher bugs, splash screen/menu clean up etc. Please remove [nsbeta3-] if you want to get fixed for final Netscape 6 product and let attinasi know why this is a blocker in detail. Setting to [rtm-]
Whiteboard: [nsbeta3-] → [nsbeta3-][rtm-]
Could the PDT, please, reconsider? * It is very easy to run into this bug when authoring Web pages. * This is an obviously visible part of CSS1 support to which Netscape has committed * Even Nav 4.x gets this right. * IE gets this right, too.
Authors will expect this to work, since this works in Nav 4.x and IE.
Whiteboard: [nsbeta3-][rtm-] → [rtm-]
This is definetly a bug, as netscape 4.x & ie work correctly. I have seen many pages useing this feature, and it galls me that my favourite browser, which claims CSS1 complience is unable to render my homepage correctly.
Just for the record, while this is a CSS1-related issue (hence the keyword) it is not strictly speaking a CSS1 bug. <hr> elements cannot be adequately described using CSS1, and I am not aware of any specification that specifies the look of an <hr> element. Thus this is not really a standards compliance issue. Of course, it is still a perfectly valid bug, since /de jure/ standards are not the only standards we are supposed to follow! :-)
Blocks: html4.01
Although <hr> itself can't be fully expessed in CSS1, the margins of <body> can be expressed in CSS1.
Meant padding. Padding interaction is a CSS1 issue.
The URL cannot be found. It would be great is somebody could attach a test case for this one. I think it might be rather easy to fix, but I want to be sure I'm looking at the right problem.
Keywords: helpwanted
Depends on: 18754
better yet, I think it's already fixed. My fix for bug 18754 fixed this, I believe. I'll attach my little test case...hopefully, someone can verify that what I attach is really what this bug was intended to fix.
This still isnt fixed as of the linux snapshot of 2000-09-17. See http://library.lspace.org/~rob/padding-test.html and http://library.lspace.org/~rob/
Are you sure? This WORKSFORME on Windows 2000 and Linux using commercial builds 2000091805 and 2000091806 respectively, using both the URL given above and the testcase buster attached.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → WORKSFORME
This is now fixed in the test cases - but it is still not fixed "in the wild". Take a look at my homepage http://library.lspace.org/~rob/ for an example of where it breaks down. This is valid xhtml & css. Useing linux snapshot dated: -rw-r--r-- 1 22 22 8496578 Sep 20 09:11 mozilla-i686-pc-linux-gnu.tar.gz
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
QA Contact: petersen → py8ieh=bugzilla
Summary: <HR> rules are not obeying the CSS padding of the <BODY> [BLOCK] → <HR> rules are not obeying the CSS padding of the <BODY> [BLOCK] [STRICT MODE]
Effectively. This in fact appears to be a problem with the strict mode handling of HR elements. I will attach a test case. We are sizing the HR correctly but it is then drawn in the wrong place. Reflow has no effect. Taking QA (CSS issue).
It could be that we just want to migrate my changes from quirks mode to standard mode, make HR an inline element with generated newlines before and after it in all cases, and do the special-case alignment measurement always as well. I just don't know if that jives with the CSS spec or not, but it sure seems to give the behavior we want.
IMHO that is too hacky. I think this should be fixed by making HR be not very special at all, as described in a bug I filed a while ago, whose number escapes me right now...
take a look at http://www.cnn.com and http://www.microsoft.com
Could the relevant person please +/- this bug please? It is a high profile backwards compatability issue that affects top100 sites.
Keywords: top100
Karnaze: Your call: + or -?
Marking nsbeta3-. The urls mentioned are using quirks mode and look ok.
Whiteboard: [rtm-] → [rtm-] [nsbeta3-]
Marking relnote. Nominating ns601 due to the lack of the keywords referring to Mozilla releases. How about setting the left and right margins of hr to 0 in the strict mode UA style sheet? It would probably minimize the impact of the bug. It works with the test case, at least.
Keywords: ns601, relnote
RELEASE NOTE ITEM: The current release of Mozilla does not correctly support the <hr> element in combination with CSS. In particular, when the body uses padding, <hr> elements are prone to being drawn in the wrong place. Work around: Instead of using the <hr> element, use semantic markup and then place borders on that. For example, instead of: <h1> Welcome </h1> <p> Welcome to my page. </p> <hr> <h2> Status </h2> <p> All is well. </p> ...use <h1> Welcome </h1> <p> Welcome to my page. </p> <h2> Status </h2> <p> All is well. </p> ...with a stylesheet containing: h2 { border-top: thin groove; padding-top: 1em; margin-top: 0; } ...adjusting the style as required to get the wanted effect.
Ian, is there something wrong with the easier workaround described in my previous comment? Margins being set to auto is triggers the bug. Setting them to 0 works for me. Making the change in the strict mode UA style sheet would minimize the impact of this bug. Adding Mozilla 0.9 nomination for a real fix.
Keywords: mozilla0.9
Yeah, that would be ok too. However <hr>s are not really semantic so if possible I am trying to make my release note suggestions propose 'Better' ways of doing things.
P2 bugs will not make RTM. Milestone -> Future.
Status: REOPENED → ASSIGNED
P2 bugs will not make RTM. Milestone -> Future.
Target Milestone: M19 → Future
I agree that <hr> isn't exactly structural markup, but the HTML WG chose to leave it in HTML 4 Strict anyway. So far, I haven't seen a good enough reason why the fix to html.css that I suggested couldn't go in. I don't think it is a good idea to give the impression that Mozilla is more broken than it actually is. That's why I suggest this alternative release note item: The current version of Mozilla does not correctly support the "auto" value for margins of the <hr> element. This problem can be avoided by setting the margins of the <hr> element to 0 and the width to 100%.
Keywords: relnote2
Whiteboard: [rtm-] [nsbeta3-] → [rtm-] [nsbeta3-] relnote-devel
Keywords: ns601
Whiteboard: [rtm-] [nsbeta3-] relnote-devel → [rtm-] [nsbeta3-] relnote-devel | Should be fixed by 38370
Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that do not have the "testcase" keyword and yet have an attachement with the word "test" in the description field. Apologies for any mistakes.
Keywords: testcase
Nom. nsbeta1. Correctness of CSS compliance on layout of widely-used element (HR).
Keywords: nsbeta1
No longer blocks: 38370
Depends on: 38370
moving to m0.9.1
Target Milestone: Future → mozilla0.9.1
*** Bug 72191 has been marked as a duplicate of this bug. ***
No longer depends on: 38370
This has nothing to do with <hr> elements and <body> elements. Any element that has padding set which has a child with 'auto' margins will end up having the child incorrectly positioned. It's like the left padding of the parent block is being shared between the right and left sides when calculating the auto margins.
Assignee: buster → attinasi
Blocks: 38370
Status: ASSIGNED → NEW
Summary: <HR> rules are not obeying the CSS padding of the <BODY> [BLOCK] [STRICT MODE] → horiz auto margins not correct in presence of padding on parent
Whiteboard: [rtm-] [nsbeta3-] relnote-devel | Should be fixed by 38370 → [Hixie-P2] (high profile: css1 bug)
Accepting, although P2 layout bugs may not have much of a chance for 0.9.1
Status: NEW → ASSIGNED
Moving to m0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
I just hit this again while making an unrelated test case. This bug is easy to see, but hard to diagnose. :-/ cc'ing waterson: Chris, on your travels around Layout you might want to stay on the look-out for this bug. :-)
by mandate, moving non-crashers and non-datalossers outward.
Target Milestone: mozilla0.9.2 → mozilla1.0
*** Bug 93883 has been marked as a duplicate of this bug. ***
Blocks: 104166
Attached patch Suggested patch (deleted) — — Splinter Review
I was tracing through the test cases and found a suspicious looking part of reflow. This patch fixes the test cases, but I'm a little worried about any regression it could cause. Are there layout tests I could try?
There are regression tests in layout/html/tests/block and layout/html/tests/table. See http://www.mozilla.org/newlayout/regress.html on running them. The patch seems reasonable to me, although I wonder if there are other parallel problems. Changing TM since a promising patch has been attached.
Keywords: patch
Target Milestone: mozilla1.0 → mozilla0.9.7
Hi again, thanks for the URL. I've followed the instructions and got lots and lots of .rgd files. Can't work out how to analyse them though :( sorry. Also noticed that there is a test case for this bug in the bugs directory already but it has padding on both sides unlike the latest test case, so maybe a new test should be added to the tree?
Arunan, the output of the last step in the regression tests is a file, called 'out' in the directions. The RGD files are used internally and you do not have to look at them. If you look at the 'out' file and search for a couple of interesting strings ('bbox' and 'frame state') you will see the testcases that have 'regressed' due to the change. Actually, we get a fair number of false regression reports, so we have to visually check them. Anyway, I'll take the patch and run the tests for you, thanks for the help you have already provided. Also, I'll add the new testcase once this is determined to be fixed.
Summary: horiz auto margins not correct in presence of padding on parent → [PATCH] horiz auto margins not correct in presence of padding on parent
There are some differences in the regression tests with the patch. A few bbox mismatches that look visually OK: file:///s:/mozilla/layout/html/tests/block/bugs/18445.html file:///s:/mozilla/layout/html/tests/block/bugs/40129.html file:///s|/mozilla/layout/html/tests/table/bugs/bug1220.html file:///s|/mozilla/layout/html/tests/table/bugs/bug29157.html file:///s|/mozilla/layout/html/tests/table/bugs/bug67915-1.html and this one that looks different, but better *with* the patch than without: file:///s:/mozilla/layout/html/tests/block/bugs/18445_2.html I think the patch looks fine, sr=attinasi
Comment on attachment 56236 [details] [diff] [review] Suggested patch sr=attinasi
Attachment #56236 - Flags: superreview+
I'd liek to add the testcase http://bugzilla.mozilla.org/attachment.cgi?id=29914&action=view to the regression tests as well, thanks Arunan.
triaging and noting I looked at it
Whiteboard: [Hixie-P2] (high profile: css1 bug) → [bae:20011126][Hixie-P2] (high profile: css1 bug)
You're very welcome. It's nice to put something back in to something I use so much :)
Comment on attachment 56236 [details] [diff] [review] Suggested patch I'm a little puzzled by this patch. I would really think that the correct fix would be to change something in the logic below, that deals with |remainingSpace|, but since this seems to work and it's really not clear to me what all these variables actually *mean*, r=dbaron.
Attachment #56236 - Flags: review+
Checking in nsBlockReflowContext.cpp; /cvsroot/mozilla/layout/html/base/src/nsBlockReflowContext.cpp,v <-- nsBlockReflowContext.cpp new revision: 1.84; previous revision: 1.83
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: