Closed Bug 95530 Opened 23 years ago Closed 9 years ago

topmargin and leftmargin attributes on the BODY element should be honored in all modes (not just Quirks mode)


(Core :: Layout, defect)

Not set





(Reporter: attinasi, Unassigned, Mentored)


(Blocks 1 open bug)


(Keywords: dev-doc-complete, html5, Whiteboard: [lang=c++][good next bug])


(3 files, 4 obsolete files)

topmargin and leftmargin attribute support on the BODY element is only
implemented in Quirks mode. Some think that this should not be a quirk, hence
this bug.

Note that topmargin and leftmargin are not part of the HTML spec, and until now,
they were IE_specific attributes.
from bug 9258 (where this originally came up):

------- Additional Comments From David Baron 2001-08-15 15:43 -------

I have to say I don't like the idea of making this quirks mode only.  I think
it's good to keep the list of quirks to a minimum -- and there's no reason
supporting all these extra features breaks standards (any more than all the ones
we already do).

------- Additional Comments From Marc Attinasi 2001-08-15 15:54 -------

dbaron, does the HTML spec say what should be done with attributes that are NOT
part of the standard? I would think that, if we had a true strict-DTD parser,
these attributes would be thrown out anyway. I'm confused, but you know more
than I about this: is it really 'standards compliant' to honor non-standard
attributes (or tags for that matter)?

I'd be more than happy to remove the quirk-check, I was under the impression
(possibly erroneous) that non-standard attributes, and especially IE-specific
ones :), were not allowed in standard mode.

------- Additional Comments From Marc Attinasi 2001-08-15 16:01 -------

... or is this the key to understanding your statement?

> ... (any more than all the ones we already do).

------- Additional Comments From David Baron 2001-08-15 17:40 -------

Partly, although if we have features that don't exist in standard mode, that
will discourage people from using it.

IMO, quirks mode should be used only when we need to deviate from a standard to
make the web work -- otherwise we should act as though we didn't have multiple
modes.  This will keep the number of differences to a minimum and make our lives
much less confusing.

Target Milestone: --- → Future
*** Bug 129069 has been marked as a duplicate of this bug. ***
I would more thing about compleatly REMOVING this nonsence
Assignee: attinasi → misc
Component: Layout → Layout: Misc Code
QA Contact: cpetersen0953 → nobody
Target Milestone: Future → ---
I disagree with this bug. Opera 9.0, Safari 2.02 and Internet Explorer 7 beta 2 now have default body margin set to 8px, just like NS 6.x, NS 7.x, Mozilla 1.x, Firefox 1.x and lots of other browsers. 
That means that not honoring topmargin and leftmargin attributes will render equivalent margins in HTML 4.01 conformant browsers.

> does the HTML spec say what should be done with attributes that are NOT
> part of the standard?

"If a user agent encounters an attribute it does not recognize, it should ignore the entire attribute specification (i.e., the attribute and its value)."

The more invalid markup code Mozilla supports means/equates in reality to less and less reasons to write valid markup code to begin with or to use valid CSS code for presentational purposes. Honoring invalid markup code in standards compliant rendering mode ("strict") is sending a weak message.
Assignee: layout.misc-code → nobody
QA Contact: nobody → layout.misc-code
I think this bug basically comes down to removing the quirks mode check around the relevant code inside BodyRule::MapRuleInfoInto, in the file content/html/content/src/HTMLBodyElement.cpp.

But while doing it we should add some automated tests (probably mochitests) for comment 6.
Whiteboard: [mentor=dbaron]
Did you mean like this? Removed the quirk mode checks from the file suggested. Please review dbaron.
Attachment #795160 - Flags: review?(dbaron)
Comment on attachment 795160 [details] [diff] [review]
Removed Quirk checks from HTMLBodyElement.cpp

Partly, yes, except that you removed too much.  This bug is about the one if containing code for topmargin, leftmargin, bottommargin, rightmargin.  You also removed some additional checks that I'm not familiar with that's related to supporting the marginwidth and marginheight attributes on frame and iframe elements (which passes from nsSubDocumentFrame::GetMarginAttributes to nsDocShell::SetMarginWidth/GetMarginWidth to this code).  I'm not sure off the top of my head what we should do with that code -- it requires checking what the behavior in other browser is, what the spec says, etc.  So it's probably better leaving that to a different bug, although we should probably have one on file if we don't have one already.

Also, the code should be reindented to match that there's no if {} anymore.

And finally, this needs a mochitest, as I mentioned above.  The test probably belongs in content/html/content/test/.  For more information, see:

Thanks for giving this a try.  I'd be happy to review a revised patch that addresses the above issues.
Attachment #795160 - Flags: review?(dbaron) → review-
Hi dbaron, Thanks for the feedback. I have made the changes you asked for. Will upload the tests asap. Please review this, if it is needed.
Attachment #795160 - Attachment is obsolete: true
Attachment #795436 - Flags: review?(dbaron)
Comment on attachment 795436 [details] [diff] [review]
Made changes to the previous patch as suggested

r=dbaron, but we should also have tests
Attachment #795436 - Flags: review?(dbaron) → review+
Did you make any progress on the tests?
Flags: needinfo?(Sahilc.2200)
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
Keywords: dev-doc-needed
Sahil - are you still working on this bug?
Whiteboard: [mentor=dbaron][lang=c++] → [mentor=dbaron][lang=c++][good next bug]
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++][good next bug] → [lang=c++][good next bug]
OK, I'm going to throw this one back in the pool; thank you for getting it this far, Sahil.
Flags: needinfo?(Sahilc.2200)
I had a look at this bug, as I'd like to help resolve it by adding tests.
I was writing a test to set and change the margins.
However, I noticed that changing the 'margin-top' property apparently isn't possible on-the-fly in Firefox (also in quirks mode, where I would expect it to work). Is this a bug as well?

Opening this attachment in Chrome shows "100px" and then "200px".
In Firefox in standard mode, it shows "8px" twice.
In Firefox in quirks mode (remove the doctype line), it shows "100px" twice.
Attached patch bug95530.patch (obsolete) — Splinter Review
I've added a test in attachment, I was wondering if this looks okay?
One additional test I could add is the combination of marginheight and topmargin (if I read the specification, it sounds like marginheight should get precedence).
Attachment #8480333 - Flags: review?(dbaron)
Comment on attachment 8480333 [details] [diff] [review]

>Bug 95530 - add test for checking if the topmargin and leftmargin
>attributes on the BODY element are honored in all modes
>(not just Quirks mode);r=dbaron

This should all be on one line; there's a semantic difference between the first line of a commit message and the remaining lines.

What did you do to test this patch?  Did you run it in the mochitest harness (i.e., with "./mach mochitest-plain content/html/content/test/test_bug95530.html") both before with and without the previous patch?
Flags: needinfo?(mathias.demare)
Yes, that's indeed how I tested it.
Just to be sure, I wanted to test again now, but it appears there's a build issue with mozilla-central right now.
Attachment #8480333 - Attachment is obsolete: true
Attachment #8480333 - Flags: review?(dbaron)
Attachment #8491727 - Flags: review?(dbaron)
Flags: needinfo?(mathias.demare)
I just ran my test again, once without the patch from Sahil, once with the patch.

Without his patch:

3 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure we are in standards mode, not quirks mode. 
4 INFO TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug95530.html | Ensure margin-top matches topmargin - got 8px, expected 100px
5 INFO TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug95530.html | Ensure margin-bottom matches bottommargin - got 8px, expected 150px
6 INFO TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug95530.html | Ensure margin-left matches leftmargin - got 8px, expected 23px
7 INFO TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug95530.html | Ensure margin-right matches rightmargin - got 8px, expected 64px

With his patch:

3 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure we are in standards mode, not quirks mode. 
4 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure margin-top matches topmargin 
5 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure margin-bottom matches bottommargin 
6 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure margin-left matches leftmargin 
7 INFO TEST-PASS | /tests/content/html/content/test/test_bug95530.html | Ensure margin-right matches rightmargin 

So I think it looks good.
Comment on attachment 8491727 [details] [diff] [review]
Testcase (rebased and fixed the description)


(In the future it's probably better to use window.getComputedStyle directly rather than using the computedStyle function from SimpleTest.js, but it's ok for this time.)
Attachment #8491727 - Flags: review?(dbaron) → review+
The failure was:

1397 INFO TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug95530.html | Test timed out. - expected PASS

I suspect you can't do assignment to window.onload because that overwrites some other onload handler, and breaks what happens when you run the tests inside of a folder?  Can you have a look?

I suspect you'll see the problem if you do './mach mochitest-plain content/html/content/test', and it ought to be fixable by just using an onload attribute on body (or using addEventListener).
Flags: needinfo?(dbaron) → needinfo?(mathias.demare)
Hm, I was unable to reproduce the issue here (with './mach mochitest-plain content/html/content/test'). However, I've created a new patch which implements the change you suggest (not assigning to window.onload). It works fine on my machine.

Does this look good?
Attachment #8491727 - Attachment is obsolete: true
Attachment #8495165 - Flags: review?(dbaron)
Flags: needinfo?(mathias.demare)
The previous testcase called waitForExplicitFinish too late, which results in a race condition.
Attachment #8495165 - Attachment is obsolete: true
Attachment #8495165 - Flags: review?(dbaron)
Attachment #8495195 - Flags: review?(dbaron)
Comment on attachment 8495195 [details] [diff] [review]
Avoid race condition by calling waitForExplicitFinish too late

Attachment #8495195 - Flags: review?(dbaron) → review+
And thanks for fixing up the test.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
We triaged recent bugs in need of doc (new process).
Documenting this bug means updating this page (and Fx 35 for devs):
> Documenting this bug means updating this page (and Fx 35 for devs):

Mozilla Quirks Mode document needs an update also:
I've updated all 3 of the mentioned pages.
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.