Closed Bug 1485581 Opened 3 years ago Closed 3 years ago
grid item's height affected by img's width in vertical writing mode
Please see the uploaded html. The grid item's height is erroneously enlarged by the iamge's width. Chrome 70 renders the html correctly.
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
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)
Reftest will be uploaded later
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.
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.
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.
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.)
The new patch may address all the mentioned issues. Please have a look.
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.
Uploading a new patch again, with a tiny comment rewording
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-
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 <firstname.lastname@example.org>" 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.)
> 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 <email@example.com>" 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.
> Both are okay, but it's better if 張俊芝 is included. Included how?
I'll use "張俊芝 <firstname.lastname@example.org>" since that's what two of the repositories at https://github.com/Zhang-Junzhi/ use.
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.
Upstream PR merged
Is this something we should consider uplifting to Beta?
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
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.