Closed Bug 1491731 Opened 3 years ago Closed 3 years ago

[css-text] calc() text-indent doesn't contribute to intrinsic size if it has a percentage

Categories

(Core :: Layout: Block and Inline, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(1 file)

The spec has been clarified that text-indent should resolve as margin:
https://github.com/w3c/csswg-drafts/issues/1597#issuecomment-406001183
That is, calc(Apx + B%) should resolve to Apx in intrinsic sizing.
Attached patch fixSplinter Review
Attachment #9009661 - Flags: review?(dholbert)
It may be worth checking out the tests at https://github.com/web-platform-tests/wpt/pull/11373. There's at least one of the tests (the one without overflow: hidden) we fail. Probably worth a separate bug though.
Comment on attachment 9009661 [details] [diff] [review]
fix

Review of attachment 9009661 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: testing/web-platform/meta/css/vendor-imports/mozilla/mozilla-central-reftests/values3/calc-text-indent-intrinsic-1.html.ini
@@ +1,2 @@
> +[calc-text-indent-intrinsic-1.html]
> +  expected: FAIL

Could you add a brief comment here, to explain that this is only "failing" because it's pending an upstream/reimport cycle that updates the test's expectations?

(I think it's also possible & perhaps-better to make the same change to our 2nd copy of the reftest in our WPT import, rather than marking it as failing, but I'm not 100% sure.)
Attachment #9009661 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (I think it's also possible & perhaps-better to make the same change to our
> 2nd copy of the reftest in our WPT import, rather than marking it as
> failing, but I'm not 100% sure.)

FWIW, dbaron says he slightly prefers making the same change to both copies of the reftest, RE synchronization ergonomics on his end.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> It may be worth checking out the tests at
> https://github.com/web-platform-tests/wpt/pull/11373.

Yeah, I've filed bug 1491896 about using the block's own content-box
as the percentage basis, which is what those tests are about.
That doesn't affect this code though.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> FWIW, dbaron says he slightly prefers making the same change to both copies

Sorry, miscommunication -- dbaron prefers what your patch is already doing. So this is fine.

(Either way is OK, but the update-both-tests method generates automated whine emails to dbaron for a few days.)
Heh, I was just going to say that this file says otherwise:
https://hg.mozilla.org/mozilla-central/raw-file/tip/testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/README

Anyway, we should get rid our local copy and make WPT
the master instead.  See discussion in bug 1490969.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Could you add a brief comment here, to explain that this is only "failing"
> because it's pending an upstream/reimport cycle that updates the test's
> expectations?

Meh, I don't think it's worth it.  An unexpected-pass on reimport +
the commit message should make it clear enough.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0273829e46ea
[css-text-3] Resolve 'text-indent' using a zero percentage basis in intrinsic sizing.  r=dholbert
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/0273829e46ea
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.