Marc Attinasi Reporter
	Description • 23 years ago

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.

	Marc Attinasi Reporter
	Comment 1 • 23 years ago

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.

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 2 • 23 years ago

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

	Kai Lahmann (is there, where MNG is)
	Comment 3 • 22 years ago

I would more thing about compleatly REMOVING this nonsence

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 21 years ago

.

	Gérard Talbot
	Comment 5 • 18 years ago

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)."
http://www.w3.org/TR/html401/appendix/notes.html#h-B.1

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.

	j.j.
	Comment 6 • 12 years ago

Seems HTML5 requires this
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-page

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 7 • 11 years ago

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.

	Sahil Chelaramani
	Comment 8 • 11 years ago

Did you mean like this? Removed the quirk mode checks from the file suggested. Please review dbaron.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 9 • 11 years ago

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:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing
https://developer.mozilla.org/en-US/docs/Mochitest

Thanks for giving this a try.  I'd be happy to review a revised patch that addresses the above issues.

	Sahil Chelaramani
	Comment 10 • 11 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 11 • 11 years ago

Comment on attachment 795436 [details] [diff] [review]
Made changes to the previous patch as suggested

r=dbaron, but we should also have tests

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 12 • 11 years ago

Did you make any progress on the tests?

	Mike Hoye [:mhoye]
	Comment 13 • 10 years ago

Sahil - are you still working on this bug?

	Mike Hoye [:mhoye]
	Comment 14 • 10 years ago

OK, I'm going to throw this one back in the pool; thank you for getting it this far, Sahil.

	Mathias De Maré
	Comment 15 • 10 years ago

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.

	Mathias De Maré
	Comment 16 • 10 years ago

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).

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 17 • 10 years ago

Comment on attachment 8480333 [details] [diff] [review]
bug95530.patch

>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?

	Mathias De Maré
	Comment 18 • 10 years ago

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.

	Mathias De Maré
	Comment 20 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 21 • 10 years ago

Comment on attachment 8491727 [details] [diff] [review]
Testcase (rebased and fixed the description)

r=dbaron

(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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 22 • 10 years ago

Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8228c732c7ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55f14ff238ad

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 10 years ago

Both backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/237aeecd181c for mochitest-1 failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=48813751&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=48813414&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=48813787&tree=Mozilla-Inbound

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 24 • 10 years ago

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).

	Mathias De Maré
	Comment 25 • 10 years ago

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?

	Mathias De Maré
	Comment 26 • 10 years ago

The previous testcase called waitForExplicitFinish too late, which results in a race condition.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 27 • 10 years ago

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=5414db8a0da2
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5414db8a0da2

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 28 • 10 years ago

Comment on attachment 8495195 [details] [diff] [review]
Avoid race condition by calling waitForExplicitFinish too late

r=dbaron

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 29 • 10 years ago

And thanks for fixing up the test.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 30 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65970a199843
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aaca52b09d8f

	Carsten Book [:Tomcat]
	Comment 31 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/65970a199843
https://hg.mozilla.org/mozilla-central/rev/aaca52b09d8f

	Jean-Yves Perrier [:teoli]
	Comment 32 • 10 years ago

We triaged recent bugs in need of doc (new process).
Documenting this bug means updating this page (and Fx 35 for devs): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/body

	j.j.
	Comment 33 • 10 years ago

> Documenting this bug means updating this page (and Fx 35 for devs):
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/body

Mozilla Quirks Mode document needs an update also:
https://developer.mozilla.org/en-US/docs/Mozilla_Quirks_Mode_Behavior

	Mathias De Maré
	Comment 34 • 10 years ago

I've updated all 3 of the mentioned pages.