1701510 - (compat) margin for media loaded into iframes should be removed

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: Screen Shot 2021-03-25 at 9.26.44 PM.png

Chromium has zero margin for media (image, video etc) loaded into iframes. Firefox adds default margins, which don't align with developer use-cases and also are modifiable, so there's no workaround.

Testcase: data:text/html,<iframe src="https://ipfs.io/ipfs/QmZjhdfoJJy9X1UUW5bs5m4e7HpnbbzjyRNtXURnUAqDzx">

Screenshot attached.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (with a small image)

Thanks, Dietrich!

Here's a self-contained testcase, using a data-URI image inside of an iframe. In Firefox, there's some default margin between the iframe and the top/left of the image. In Chrome, there's no such margin.

Comment 2

[Daniel Holbert [:dholbert]]

WebKit agrees with Chromium (I tested Safari 14 and current Epiphany Tech Preview on Linux).

This WebKit/Chromium behavior goes back at least as far as Safari 4 and Chrome 16 (I tested them using BrowserStack, on MacOS Snow Leopard). So, definitely not a recent change on their side.

The fix here is probably to add body { margin: 0 } to https://searchfox.org/mozilla-central/source/layout/style/ImageDocument.css

Also: we probably don't want this to be guarded by the @media print {} media query, because it would be weird for this behavior to vary between printed vs. screen-viewed iframe elements.

ImageDocument.css is used for top-level image documents as well, though; so this change would mean top-level image documents will lose their default margin as well (when printed, at least; they already don't have any margin when viewed on-screen). That's... probably fine? Interop-wise, it looks like Chrome doesn't add any margins (aside from the user-configurable page margins) when printing top-level image documents. They do center the image in the page, but that's a different question (and the image's "long axis" can go right up to the edge of the page, with no body{}-imposed margins).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1701510: Zero out the 'body' margin for all ImageDocuments (including iframes and printed images). r?emilio

Note that this means images will be slightly closer to the upper-left of the
page, if they're viewed directly and then printed. This shouldn't cause them
to be clipped or cause other issues; they'll still be offset from the page edge
by the printed-page margins, as well as any unprintable areas that we get from
the printer.

Comment 4

[Daniel Holbert [:dholbert]]

Let's see if this breaks any tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=accbbdc6c01bb5b96c289a0d02c7b2f98757bb12

Comment 5

[Daniel Holbert [:dholbert]]

New Try run, with a test tweak to address failures in one mochitest that needs to adapt to this change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e644592d2a8e0c72f3807bc85438240abb9e7645

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Dietrich Ayala (:dietrich) from comment #0)

Chromium has zero margin for media (image, video etc) loaded into iframes. Firefox adds default margins,

Correcting one part of this: from my testing, Firefox actually does not add any margins for videos loaded into iframes. (We do appear to center the video with a "contain" constraint, which may result in some centering-margin added in one axis or the other; Chrome seems to do that too.)

So I think the only behavior that potentially needs changing here is for images (in iframes).

(dietrich, please correct me if I'm wrong or missing something.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c46535ee223
Zero out the 'body' margin for all ImageDocuments (including iframes and printed images). r=emilio

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5c46535ee223

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to Daniel Holbert [:dholbert] from comment #6)

(dietrich, please correct me if I'm wrong or missing something.)

Nah I was just eyeballing, didn't spelunk the source. Thanks for fixing :dholbert!

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1701510 - Support error stacks only in EARLY_BETA_OR_EARLIER) || DEBUG builds; r=#dom-storage

Depends on D114735

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1701510 - Remove the module extra key from LogError; r=#dom-storage

The appening of module extra key was already commented out.

Depends on D114736

Comment 12

[Phabricator Automation]

Comment on attachment 9221103 [details]
Bug 1701510 - Support error stacks only in EARLY_BETA_OR_EARLIER) || DEBUG builds; r=#dom-storage

Revision D114736 was moved to bug 1709352. Setting attachment 9221103 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9221110 [details]
Bug 1701510 - Remove the module extra key from LogError; r=#dom-storage

Revision D114739 was moved to bug 1709352. Setting attachment 9221110 [details] to obsolete.

Comment 14

[Jan Varga [:janv]]

Sorry for the noise.