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

RESOLVED FIXED in mozilla35

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: Marc Attinasi, Unassigned, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla35
dev-doc-complete, html5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][good next bug])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Status: NEW → ASSIGNED

Updated

16 years ago
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
Status: ASSIGNED → NEW
Component: Layout → Layout: Misc Code
QA Contact: cpetersen0953 → nobody
Target Milestone: Future → ---

Comment 5

11 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.
Assignee: layout.misc-code → nobody
QA Contact: nobody → layout.misc-code

Comment 6

5 years ago
Seems HTML5 requires this
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-page
Blocks: 747485
Keywords: html5
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]

Comment 8

4 years ago
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.
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:
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.
Attachment #795160 - Flags: review?(dbaron) → review-

Comment 10

4 years ago
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.
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++]

Updated

4 years ago
Keywords: dev-doc-needed

Comment 13

3 years ago
Sahil - are you still working on this bug?

Updated

3 years ago
Whiteboard: [mentor=dbaron][lang=c++] → [mentor=dbaron][lang=c++][good next bug]
(Assignee)

Updated

3 years ago
Mentor: dbaron@dbaron.org
Whiteboard: [mentor=dbaron][lang=c++][good next bug] → [lang=c++][good next bug]

Comment 14

3 years ago
OK, I'm going to throw this one back in the pool; thank you for getting it this far, Sahil.
Flags: needinfo?(Sahilc.2200)

Comment 15

3 years ago
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

3 years ago
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).
Attachment #8480333 - Flags: review?(dbaron)
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?
Flags: needinfo?(mathias.demare)

Comment 18

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

Comment 19

3 years ago
Created attachment 8491727 [details] [diff] [review]
Testcase (rebased and fixed the description)
Attachment #8480333 - Attachment is obsolete: true
Attachment #8480333 - Flags: review?(dbaron)
Attachment #8491727 - Flags: review?(dbaron)
Flags: needinfo?(mathias.demare)

Comment 20

3 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.
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.)
Attachment #8491727 - Flags: review?(dbaron) → review+
Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8228c732c7ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55f14ff238ad
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
Flags: needinfo?(dbaron)
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)

Comment 25

3 years ago
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?
Attachment #8491727 - Attachment is obsolete: true
Attachment #8495165 - Flags: review?(dbaron)
Flags: needinfo?(mathias.demare)

Comment 26

3 years ago
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.
Attachment #8495165 - Attachment is obsolete: true
Attachment #8495165 - Flags: review?(dbaron)
Attachment #8495195 - Flags: review?(dbaron)
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=5414db8a0da2
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5414db8a0da2
Comment on attachment 8495195 [details] [diff] [review]
Avoid race condition by calling waitForExplicitFinish too late

r=dbaron
Attachment #8495195 - Flags: review?(dbaron) → review+
And thanks for fixing up the test.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65970a199843
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aaca52b09d8f
https://hg.mozilla.org/mozilla-central/rev/65970a199843
https://hg.mozilla.org/mozilla-central/rev/aaca52b09d8f
Status: NEW → RESOLVED
Last Resolved: 3 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): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/body

Comment 33

3 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

Comment 34

3 years ago
I've updated all 3 of the mentioned pages.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.