Last Comment Bug 95530 - topmargin and leftmargin attributes on the BODY element should be honored in all modes (not just Quirks mode)
: topmargin and leftmargin attributes on the BODY element should be honored in ...
Status: RESOLVED FIXED
[lang=c++][good next bug]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla35
Assigned To: Nobody; OK to take it and work on it
:
Mentors: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
: 129069 (view as bug list)
Depends on:
Blocks: quirks-mode-spec
  Show dependency treegraph
 
Reported: 2001-08-15 17:47 PDT by Marc Attinasi
Modified: 2014-10-08 11:43 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed Quirk checks from HTMLBodyElement.cpp (4.22 KB, patch)
2013-08-25 07:55 PDT, Sahil Chelaramani
dbaron: review-
Details | Diff | Splinter Review
Made changes to the previous patch as suggested (4.69 KB, patch)
2013-08-26 07:41 PDT, Sahil Chelaramani
dbaron: review+
Details | Diff | Splinter Review
bug95530_updatelive.html (1.09 KB, text/html)
2014-08-27 20:55 PDT, Mathias De Maré
no flags Details
bug95530.patch (2.60 KB, patch)
2014-08-27 21:03 PDT, Mathias De Maré
no flags Details | Diff | Splinter Review
Testcase (rebased and fixed the description) (2.55 KB, patch)
2014-09-18 12:25 PDT, Mathias De Maré
dbaron: review+
Details | Diff | Splinter Review
Testcase (using addEventListener) (2.64 KB, patch)
2014-09-25 03:23 PDT, Mathias De Maré
no flags Details | Diff | Splinter Review
Avoid race condition by calling waitForExplicitFinish too late (2.64 KB, patch)
2014-09-25 04:09 PDT, Mathias De Maré
dbaron: review+
Details | Diff | Splinter Review

Description Marc Attinasi 2001-08-15 17:47:11 PDT
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.
Comment 1 Marc Attinasi 2001-08-15 17:49:21 PDT
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.

Comment 2 Christopher Aillon (sabbatical, not receiving bugmail) 2002-03-05 18:58:55 PST
*** Bug 129069 has been marked as a duplicate of this bug. ***
Comment 3 Kai Lahmann (is there, where MNG is) 2002-03-20 07:49:38 PST
I would more thing about compleatly REMOVING this nonsence
Comment 4 Boris Zbarsky [:bz] 2003-04-22 11:28:57 PDT
.
Comment 5 Gérard Talbot 2006-02-20 19:43:34 PST
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.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-08-20 19:06:28 PDT
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.
Comment 8 Sahil Chelaramani 2013-08-25 07:55:15 PDT
Created attachment 795160 [details] [diff] [review]
Removed Quirk checks from HTMLBodyElement.cpp

Did you mean like this? Removed the quirk mode checks from the file suggested. Please review dbaron.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-08-25 10:40:55 PDT
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.
Comment 10 Sahil Chelaramani 2013-08-26 07:41:41 PDT
Created attachment 795436 [details] [diff] [review]
Made changes to the previous patch as suggested

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.
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-08-26 21:44:03 PDT
Comment on attachment 795436 [details] [diff] [review]
Made changes to the previous patch as suggested

r=dbaron, but we should also have tests
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-11-20 00:16:22 PST
Did you make any progress on the tests?
Comment 13 Mike Hoye [:mhoye] 2014-04-14 07:52:02 PDT
Sahil - are you still working on this bug?
Comment 14 Mike Hoye [:mhoye] 2014-07-16 11:50:35 PDT
OK, I'm going to throw this one back in the pool; thank you for getting it this far, Sahil.
Comment 15 Mathias De Maré 2014-08-27 20:55:02 PDT
Created attachment 8480328 [details]
bug95530_updatelive.html

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.
Comment 16 Mathias De Maré 2014-08-27 21:03:22 PDT
Created attachment 8480333 [details] [diff] [review]
bug95530.patch

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).
Comment 17 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-16 14:13:50 PDT
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?
Comment 18 Mathias De Maré 2014-09-18 12:21:49 PDT
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.
Comment 19 Mathias De Maré 2014-09-18 12:25:14 PDT
Created attachment 8491727 [details] [diff] [review]
Testcase (rebased and fixed the description)
Comment 20 Mathias De Maré 2014-09-24 05:27:17 PDT
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 21 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-24 14:44:55 PDT
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.)
Comment 22 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-24 14:48:41 PDT
Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8228c732c7ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55f14ff238ad
Comment 24 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-24 16:04:44 PDT
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).
Comment 25 Mathias De Maré 2014-09-25 03:23:28 PDT
Created attachment 8495165 [details] [diff] [review]
Testcase (using addEventListener)

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?
Comment 26 Mathias De Maré 2014-09-25 04:09:07 PDT
Created attachment 8495195 [details] [diff] [review]
Avoid race condition by calling waitForExplicitFinish too late

The previous testcase called waitForExplicitFinish too late, which results in a race condition.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-26 15:12:31 PDT
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=5414db8a0da2
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5414db8a0da2
Comment 28 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-27 12:49:41 PDT
Comment on attachment 8495195 [details] [diff] [review]
Avoid race condition by calling waitForExplicitFinish too late

r=dbaron
Comment 29 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-27 12:51:03 PDT
And thanks for fixing up the test.
Comment 30 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2014-09-27 13:02:43 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65970a199843
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aaca52b09d8f
Comment 32 Jean-Yves Perrier [:teoli] 2014-10-06 23:28:14 PDT
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
Comment 33 j.j. 2014-10-07 05:17:59 PDT
> 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
Comment 34 Mathias De Maré 2014-10-08 11:43:17 PDT
I've updated all 3 of the mentioned pages.

Note You need to log in before you can comment on or make changes to this bug.