Closed Bug 1485581 Opened Last year Closed Last year

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: zjz, Assigned: zjz)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 9 obsolete files)

Attached file 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.
Blocks: css-grid
Blocks: writing-mode
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.
Let me try to figure this issue out
Assignee: nobody → zjz
Status: NEW → ASSIGNED
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/)
Attachment #9003707 - Attachment filename: fix.patch
Attachment #9003707 - Flags: review?(dholbert) → review?(mats)
New patch
Attachment #9003707 - Attachment is obsolete: true
Attachment #9003745 - Attachment is obsolete: true
Attachment #9003707 - Flags: review?(mats)
Attachment #9003745 - Flags: review?(mats)
Attachment #9003746 - Flags: review?(mats)
Reftest will be uploaded later
Attached patch reftest.diff (obsolete) — Splinter Review
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.
Attachment #9003757 - Flags: review?(mats)
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.
Flags: needinfo?(mats)
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.
(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.
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>
Ah, sorry, forgot to say that in the example above the div is assumed to have been specified with inline-size: max-content
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.
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #9003746 - Attachment is obsolete: true
Attachment #9003757 - Attachment is obsolete: true
Attachment #9003746 - Flags: review?(mats)
Attachment #9003757 - Flags: review?(mats)
Attachment #9005556 - Flags: review?(dbaron)
Flags: needinfo?(mats)
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-.
Attachment #9005556 - Flags: review?(dbaron) → review-
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?
(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.)
Attached patch patch.diff (obsolete) — Splinter Review
The new patch may address all the mentioned issues.

Please have a look.
Attachment #9005556 - Attachment is obsolete: true
Attachment #9005793 - Flags: review?(dbaron)
Attached patch patch.diff (obsolete) — Splinter Review
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.
Attachment #9005793 - Attachment is obsolete: true
Attachment #9005793 - Flags: review?(dbaron)
Attachment #9005799 - Flags: review?(dbaron)
Attached patch patch.diff (obsolete) — Splinter Review
Uploading a new patch again, with a tiny comment rewording
Attachment #9005799 - Attachment is obsolete: true
Attachment #9005799 - Flags: review?(dbaron)
Attachment #9005872 - Flags: review?(dbaron)
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.)
Attachment #9005872 - Flags: review?(dbaron) → review-
Attached patch patch.diff (obsolete) — Splinter Review
Thank you for the review, including the detailed guidence of how to modify the patch.
Attachment #9006451 - Flags: review?(dbaron)
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 <zjz@zjz.name>" or maybe with 張俊芝 included as well?)
Attachment #9006451 - Flags: review?(dbaron) → review+
(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.)
Attached patch patch.diffSplinter Review
> 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 <zjz@zjz.name>" 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.
Attachment #9005872 - Attachment is obsolete: true
Attachment #9006451 - Attachment is obsolete: true
Attachment #9006616 - Flags: review?(dbaron)
> Both are okay, but it's better if 張俊芝 is included.

Included how?
I'll use "張俊芝 <zjz@zjz.name>" since that's what two of the repositories at https://github.com/Zhang-Junzhi/ use.
Okay!
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12863 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/8aa236bd11b2
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Attachment #9006616 - Flags: review?(dbaron) → review+
Is this something we should consider uplifting to Beta?
Flags: needinfo?(dbaron)
Flags: in-testsuite+
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
Flags: needinfo?(dbaron)
Attachment #9006616 - Flags: approval-mozilla-beta?
Comment on attachment 9006616 [details] [diff] [review]
patch.diff

Uplift approved for 63 beta 5.
Attachment #9006616 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.