1485581 - grid item's height affected by img's width in vertical writing mode

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[張俊芝(ZHANG Junzhi)]

Attachment: test.html

Please see the uploaded html.

The grid item's height is erroneously enlarged by the iamge's width.

Chrome 70 renders the html correctly.

Comment 1

[張俊芝(ZHANG Junzhi)]

The problem seems that ReflowInput::ComputedISize doesn't take img into account.

In vertical writing mode, it maps all computed widths to block sizes, and all computed heights to inline sizes.

Comment 2

[張俊芝(ZHANG Junzhi)]

Let me try to figure this issue out

Comment 3

[張俊芝(ZHANG Junzhi)]

Attachment: Makes nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize "writing-mode responsive".diff

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9003707 [details] [diff] [review]
Makes nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize "writing-mode responsive".diff

I'm away for a couple days (returning Monday) and can't think about this too deeply at the moment -- punting review to mats who's probably a better person to think about this from a grid perspective anyway.

(One drive-by review note: this should ideally include a an automated test, perhaps as a reftest in layout/reftests/css-grid/)

Comment 5

[張俊芝(ZHANG Junzhi)]

Attachment: Makes nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize "writing-mode responsive".diff

Comment 6

[張俊芝(ZHANG Junzhi)]

Attachment: Makes nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize "writing-mode responsive".diff

New patch

Comment 7

[張俊芝(ZHANG Junzhi)]

Reftest will be uploaded later

Comment 8

[張俊芝(ZHANG Junzhi)]

Attachment: reftest.diff

Here is the reftest.

I am putting the reftest under the writing-mode directory.

Since although it was discovered and reported on grid elements, it's essentially related to writing-mode, it could potentially be trigged on other non-grid elements as well, in some conditions.

Comment 9

[張俊芝(ZHANG Junzhi)]

Hello, mats?

I am not sure if the system had failed to automatically show you this bug review request, this post is just made to ensure you see it in case the system had failed to show you this bug.

Thank you.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this makes nsImageFrame switch on the parent's writing mode.  But nsVideoFrame and nsHTMLCanvasFrame switch on their own writing mode.  It seems like these ought to do the same thing -- and I suspect it's to use its own writing-mode, but I'm not 100% sure.

Comment 11

[張俊芝(ZHANG Junzhi)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #10)
> So this makes nsImageFrame switch on the parent's writing mode.  But
> nsVideoFrame and nsHTMLCanvasFrame switch on their own writing mode.  It
> seems like these ought to do the same thing -- and I suspect it's to use its
> own writing-mode, but I'm not 100% sure.

No matter in vertical or horizontal writing modes, an <img> is put the same way, that is, <img> is not rotated by its own writing-mode.

So

<div><img ...></div>

If the block parent is in horizontal mode, the parent's inline seems to *always* be contributed by the img's width.
If the block parent is in vertical mode, the parent's inline seems to *always* be contributed by the img's height.

So I think we might also want to change the nsVideoFrame and nsHTMLCanvasFrame to use their parent's writing mode.

Comment 12

[張俊芝(ZHANG Junzhi)]

Since video and canvas are also always put the same way, no matter it's own writing mode is vertical or horizontal. They behave like an <img>

Comment 13

[張俊芝(ZHANG Junzhi)]

Ah, sorry, forgot to say that in the example above the div is assumed to have been specified with inline-size: max-content

Comment 14

[張俊芝(ZHANG Junzhi)]

You're right, David.

I just gave it a test: I commented out getParent()-> snippet in nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize, and then specified style="horizontal-tb" with the <img> in the test.html, and the result still turned out as expected.

Comment 15

[張俊芝(ZHANG Junzhi)]

You're right, David.

I just gave it a test: I commented out getParent()-> snippet in nsImageFrame::GetMinISize and nsImageFrame::GetPrefISize, and then specified style="writing-mode: horizontal-tb" with the <img> in the test.html, and the result still turned out as expected.

Comment 16

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 9005556 [details] [diff] [review]
patch.diff

>diff --git a/layout/generic/nsFrame.h b/layout/generic/nsFrame.h
>+// FIXME This macro(as well as the struct in the macro content) should go
>+// through a renaming refactoring to reflect the fact that it's actually
>+// displaying a minimum inline size, not a minimum width.

This comment should probably be shorter, and just occur once to refer to both macros.

>diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp
>+  const nsStyleCoord& iSize = this->GetWritingMode().IsVertical() ?
>+                                mIntrinsicSize.height : mIntrinsicSize.width;

Drop the explicit "this->" (once in each function).

>-}
>+}
>\ No newline at end of file

Don't remove the newline at the end of the file.

>diff --git a/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution-ref.html b/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution-ref.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE html>
>+<html>
>+<title>Ref for Bug 1485581 (grid item's height affected by img's width in vertical writing mode)</title>
>+
>+<style>
>+* { margin: 0; padding: 0; }
>+</style>
>+
>+<div style="border: 1px solid blue; height: 150px; width: 202px;">
>+	<div style="border: 1px solid red; height: 148px;">
>+		<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMgAAABkCAMAAAD0WI85AAACwVBMVEUAAAAAAQAAAgAAAwAABAAABQAABgAABwAACAAACQAACgAACwAADAAADQAADgAADwAAEAAAEQAAEgAAEwAAFAAAFgAAFwAAGAAAGQAAGgAAGwAAHAAAHQAAHgAAHwAAIAAAIQAAIgAAIwAAJAAAJQAAJgAAJwAAKAAAKQAAKgAALAAALQAALgAALwAAMAAAMQAAMgAANAAANQAANgAANwAAOAAAOQAAOgAAOwAAPQAAPgAAPwAAQAAAQQAAQgAAQwAARAAARQAARgAARwAASAAASQAASgAASwAATAAATQAATgAATwAAUAAAUQAAUgAAUwAAVAAAVQAAVgAAVwAAWAAAWQAAWgAAWwAAXQAAXgAAXwAAYAAAYQAAYgAAYwAAZQAAZgAAZwAAaAAAaQAAagAAawAAbAAAbQAAbgAAbwAAcAAAcQAAcgAAcwAAdQAAdgAAdwAAeAAAewAAfAAAfQAAfgAAfwAAgAAAggAAhAAAhQAAhgAAhwAAiAAAiQAAigAAiwAAjAAAjQAAjgAAjwAAkAAAkQAAkgAAkwAAlAAAlQAAlwAAmAAAmQAAmgAAmwAAnAAAnQAAngAAnwAAoAAAogAAowAApAAApQAApwAAqAAAqQAAqwAArAAArQAArgAArwAAsAAAsQAAsgAAswAAtAAAtQAAtgAAtwAAuAAAugAAuwAAvAAAvgAAvwAAwAAAwgAAwwAAxAAAxQAAxgAAxwAAyAAAyQAAygAAywAAzAAAzQAAzgAAzwAA0QAA0gAA0wAA1AAA1QAA1gAA1wAA2AAA2QAA2wAA3AAA3QAA3gAA4AAA4QAA4gAA4wAA5AAA5QAA5gAA5wAA6AAA6QAA6gAA6wAA7AAA7QAA7gAA7wAA8AAA8QAA8gAA8wAA9AAA9QAA9gAA9wAA+AAA+QAA+gAA+wAA/AAA/QAA/gAA/wAgwomMAAAAAWJLR0QAiAUdSAAAAAlwSFlzAAALEwAACxMBAJqcGAAAAAd0SU1FB+IIFwgCCFgYj8YAAAbXSURBVHja7Zr9XxRFGMAHjkM44UQR8EhFSUklNTAzDQxBM/FEJV9SM9+wwndFMVRM8wVO6UVNzjsr6QXTxPe3Ik0pJNNEsgzf4uV4OXj+imZ2927nFgL2uFvDZn6Yz8PczHPz9WZ3dr8OgiekIAbCQBgIA2EgDISBMBAGwkAeP4jFHlXm/2KPy/LvKgbSuH2g74D1dSSsWti7T4rFMWpj2aH6yBZeQxn2ZjM6qhRI9VTfmYbpaCYO62O06el+Y6x01NaSG/y5I8i8bKVB9KdxvRAVA3yJPgUwoiN05EThQXq9pTAIX44jE8DE7g34x+iSTEdOlBIOxO/xgJxABwB0SSR8tQ8dmdH9LdEBsacALqnexm3L1MX8iL3cFJPQZVy//Kww45pVEYETCjDIRh+EywXcfNwcqYk6pxjI++gqNHqlkPBNHyrCMwkfuG5NqPokwFLVj3Cz0wphRClaDdDQ1W8LQJ1mGQ9iHYFe2/aGLwa5sgfFm83luDlWl5Lq261SIZDGwX2tcBetIfFK9ECM8Ezm1QKe/0sAFU+NgtdDK2xjBgwHKOyckgBwFh3mQXYhjAXHyNJ6iISlFXsPYDvKUwjEiPYAlPPTX4EeiZFtkU/0qAE4hNI8TfYxqV4VsDn2oKYWNqgr+H7DdPW2i90OQoafQzuVAbkdMhTvI43qRdyC8qUiG8hSVILreBSD6yKy/HdAPv4kPq0MnYT4kUI/7ThoFuQqylQExBKt5fbi0EmkHt+XjgSQ+eh3gEc6dRTeWioLcLkFld6rajUFEJFW33k938/qldg8SJEyINZpnoe4YHIgd9OdTkcCyLAuuHon6Bu0TRwXE3PWuxoWjyxEZ4R+4f0pkBSlQazT0Qd8lE+2wQPoWzoyoxz8yddkWoWq3TBDc8M+cIN/5ih84XTa5lcnzHghyidPKwSkUR2rMEh9Mhpj3GUwGCxgjQvI3KQdi38MMTIj1ZRdS7x7lUND1CAr3OkSbx/5PXo6HeCBKny8bcZl3XyWZ03oyW2Icd6bDMVKglxAQinHjyuLwsIWc4+K9siMDswN6TkHf5hNfiAwoP22kQ2B+ELHq45bbvyMbyaFhC26H0xAboz275+n7DXSYnH5MwYDYSBPGAiTDwyEgTAQBsJA3APSvA21tKGPu0Cu5wxBRVwkS5k2u+dT5tQ9zwUtgQz28uRB5CnTZidJmVNpH96hundpZfAg8pRpW/616T68Q1UERJ4ylQvipxyILGXqYENr10Vqh+fZZ35N3528c46l+tgcqhIg8pQpbUNrowPW7pvtsV8AuR049AtT34DDl6k+NoeqBIhMZUrZ0HU+V8jVHCqArPYsBTiDDjr0EUSXEiAylSllQ/tN5u8VZXxjUg/8Vw0RDlQfBUFkKlPRhlZ5CiLmMt/4ruoukUV5DsZUQRCZylQ0PQ0+E4u4Uss3lgbFFp6KjKx18HNKgshTptQkBw90aKxPGNojZNYdkICkKAYiT5lSk8xGH5IWq9CY629t2kdwqG4EKTEap6BNxsMgU5lSk2wY7TH7k+wxS4XGLDT/Y6Op4JGj+uUdqhtBDPyFOgRkKlN6ktb3ojr3m1UkNFaGcRk1hxz68A61Q3mtJS/+jZ+fL+qiO7igq/bazV0xEZM6uml8PnTn6ZKvRgf93NFBHqyM0HR7bu2f7J2dgTAQBsJAGEhHB7m0pFionC4WpUEKhffUnCYberuEp0SeKgKizyHlJ9eCSOSpIiCZ//KI9V/7r+n/H0gGugbEXR9tAkI5UEtWcHfKkdK2tMpEHSgVMlTtfcEvoRR29O6afI80H5/xjHbkRYecVDJ3g9AOdKt27mbKkdK2dCp9oFTIkNx7+TQ0am6/lYncYe/PPMZmbNT5lNE5xRQuuWultQBCO9CQW0A7UtqWOhwoFTLEPQRIRPpqgBHeRDH9gD86Tw50iTnFFC65a33XAgjtQLNIg+hIJbZUPFBKZdjIWfhluA9f/vKcQ+cUU7h9aUkdqOhIpbZUPFBKZcjmQNLRdfJCnzuuh85TT+WkUrgdROpARUcqtaXiYTkqg4EDWU9Afu0TbSoHPz2Vk0rhKpCtiKzg3KYgUgdKOVKJLW0NJCGiBocERMwppnAViIkcNy0bwM/giFBxu7TEgVKOVGJLWwMJeoV8o1pP5xRTuArkob82LTV0NvnyY2hWBV+RInWglCOV2NLWQBJVqfsWBAfq6ZxiCpft7CeG+A7K+418eV2c5jxfkSJ1oJQjldjS1kD+mBoQvuDOOD2dk0rm7vcRdzhQJ3O2C8QdDtTZnO37RdzhQJ3M2T4QdzhQJ3My+cBAGAgDYSAMhIEwEAbCQFot/wDLHoMxcW0kVwAAAABJRU5ErkJggg==">
>+	</div>
>+</div>
>+
>+</html>
>diff --git a/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution.html b/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/writing-mode/1485581-img-intrinsic-size-contribution.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE html>
>+<html style="writing-mode: vertical-lr;">
>+<title>Bug 1485581 (grid item's height affected by img's width in vertical writing mode)</title>
>+
>+<style>
>+* { margin: 0; padding: 0; }
>+</style>
>+
>+<div style="border: 1px solid blue; display: grid; height: 150px;">
>+	<div style="border: 1px solid red;">

Please don't use red as something that should show up.  Pick a color other than red or green.

>+		<img src="data:image/png;

Using a data url seems unnecessary, and especially one this large.

Why not just use something like ../image/blue-50x100.png (or 100x50) ?

Also, the test should probably end up in web-platform-tests, so could you instead put it in testing/web-platform/tests/css/css-writing-modes, call it something like img-intrinsic-size-contribution-001.html, add the correct metadata, and run "./mach wpt-manifest-update" to update the test manifests?

With those changes, I think this looks good, but I'd like to re-review the revisions, so marking as review-.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and could you also add a test where the img has a different writing-mode from its parent, to test the case of its own writing mode versus its parent's?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The general principle behind my last comment is that you shouldn't write a one-off test and then *not* add it to the regression test suite.)

Comment 20

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

The new patch may address all the mentioned issues.

Please have a look.

Comment 21

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

Oh! Like the border colour, maybe the image also shouldn't be green or red, since whether the image shows up doesn't mean the test passes or fails, it needs to be in a "neutral" colour.

Changing the image to be blue.

Comment 22

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

Uploading a new patch again, with a tiny comment rewording

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 9005872 [details] [diff] [review]
patch.diff

This looks good, except for a few details with the tests:

* they should all (tests and reference) have a link rel="author" line, with your name and URL (either email or homepage)
* the tests should have a link rel="help" associating them with a spec section (or more than one -- could be both writing-modes and grid)
* the tests *could* also have a meta name="assert" describing what they're testing
* the tests and reference should probably also follow the format of beginning with "CSS Test: " etc. and have a title that's a little more descriptive, such as "CSS Test: intrinsic size contributions of images in vertical writing mode"; if you want to link to the bug you could use an additional link rel="help"

See https://wiki.csswg.org/test/format for more details.

I think it's also probably better not to put the reference in the "reference" subdirectory; if you do that I worry about the relative link to the image not working in the built test suite.

r=dbaron with those changes, but I'd probably like to look at the revisions, so marking as review-

(Sorry for the delay getting to this one.)

Comment 24

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

Thank you for the review, including the detailed guidence of how to modify the patch.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 9006451 [details] [diff] [review]
patch.diff

Two minor things:

* a few "No newline at end of file" snuck in to the tests and reference when you made the revisions.  (Windows uses CR-LF as a line *separator*; Unix uses LF as a line *terminator*; version control systems generally follow the Unix conventions and expect the last line to have a newline at the end, and consider data after the final newline to be an unusual state.)

* "parental element's" should be "parent element's" in both meta name="assert"

r=dbaron with both of those things fixed

Also, I noticed (and should have noticed earlier!) that you didn't generate the patch with version control metadata.  The right way to do that depends on the version control system you're using... and we seem to have deleted the relevant documentation... so could you at least say how you want your username and email formatted in the commit author?  (e.g., maybe as "Zhang Junzhi <[e-mail]>" or maybe with 張俊芝 included as well?)

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that the official way to submit patches (as of a month or two ago) is by using Phabricator, briefly documented at https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#submitting-patches , although I actually use the Mercurial extension at https://bitbucket.org/kmaglione/hgext instead of the ones documented there.  But I'm fine with dealing with patch files here, unless you'd prefer to figure out Phabricator.)

Comment 27

[張俊芝(ZHANG Junzhi)]

Attachment: patch.diff

> a few "No newline at end of file" snuck in to the tests and reference

Oops, I forgot to turn off my editor's plugin which automatically deletes all trailing white spaces and all trailing lines.

> "parental element's" should be "parent element's" in both meta name="assert"

Fixed.

> "Zhang Junzhi <[e-mail]>" or maybe with 張俊芝 included as well?

Both are okay, but it's better if 張俊芝 is included.

> But I'm fine with dealing with patch files here, unless you'd prefer to figure out Phabricator.

I'd like to learn Phabricator, because I love to make long term contributions to Mozilla.

But probably these days I will be busy on my own projects, so I prefer to learn it later on(when I have comparatively more free time on here), so that I can quickly become familar with the toolchains used in Mozilla.

FYI, the patch is based on changeset bb0febbdbb25 .

Thanks for the review.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Both are okay, but it's better if 張俊芝 is included.

Included how?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll use "張俊芝 <[e-mail]>" since that's what two of the repositories at https://github.com/Zhang-Junzhi/ use.

Comment 30

[張俊芝(ZHANG Junzhi)]

Okay!

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86548ae6567ded7ee9cc4b2193130ef091306e2c&group_state=expanded

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa236bd11b29e9161d4be7228ce047d6a74fddc
Bug 1485581 - Make nsImageFrame report intrinsic inline sizes in the correct dimension (height) when writing-mode is vertical.  r=dbaron

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12863 for changes under testing/web-platform/tests

Comment 34

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 35

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8aa236bd11b2

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 9006616 [details] [diff] [review]
patch.diff

Approval Request Comment
[Feature/Bug causing the regression]: support for vertical writing-modes
[User impact if declined]: pages in vertical writing modes (e.g., vertical Japanese or Chinese) that depend on intrinsic sizes of images use the wrong dimension (width vs. height) of the image.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: through automated tests, yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's doing basically the same thing that we already do for <canvas> and <video>, and vertical writing-mode isn't widely used yet on the Web.
[String changes made/needed]: no

Comment 39

[Pascal Chevrel:pascalc]

Comment on attachment 9006616 [details] [diff] [review]
patch.diff

Uplift approved for 63 beta 5.

Comment 40

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8614b6f4970a