percentage text-indent should be relative to the containing block of the text, not the containing block of the block

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: dbaron, Assigned: mats)

Tracking

(Blocks 2 bugs, {site-compat})

Trunk
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

In https://github.com/w3c/csswg-drafts/issues/2394 the CSS WG is resolving that percentage text-indent should be resolved against the containing block of the text (or the content width of the element).  We need to change our behavior.  (The old spec text could be read in two different ways, and it actually led to implementation divergence.)
I believe this makes bug 908706 and bug 1306590 invalid, since I think they're both about fixing bugs with the old now-incorrect behavior.
Priority: -- → P3
Duplicate of this bug: 1491896
I've filed a chromium bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=884588

I'd strongly recommend that we *don't* ship this change before
Chrome/Safari does.  All UAs are currently (mostly) compatible
in using the containing block's inline size to resolve text-indent,
so there's a significant web-compat risk here.
Blocks: 1306590
It appears Chrome has landed a fix for this now.
Assignee: nobody → mats
A couple of notes on the test changes:
The test files css/css-text/text-indent/text-indent-percentage-*.html
placed two "X" on top of each other, one of which had red color.
This is bad because it produces different colored AA pixels compared
to the reference, which is just a black "X".  Also, there's no need
for this red reference "X" in the test itself since the reference
shows the expected rendering anyway.

The added fuzz factor on Android for 1377447-1.html is unrelated to
this bug, but it annoyed me to it fail all the time on my Try pushes.
Attachment #9019371 - Flags: review?(emilio)
Comment on attachment 9019371 [details] [diff] [review]
[css-text] Resolve 'text-indent' percentage against the content-box inline-size of the box itself, not its containing block

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

I'm a bit confused about the body { background: white } bits, but assuming it has a reasonable explanation for why are they needed, or they get removed before landing, this looks good to me.

::: testing/web-platform/tests/css/css-text/text-indent/text-indent-percentage-001.xht
@@ +17,5 @@
>  			}
>  			#reference1
>  			{
>  				color: red;
> +				left: 100px; /* see comments for #test1 below */

Thanks for fixing these! I'll mention it in https://github.com/web-platform-tests/wpt/issues/13581 so that nobody duplicates work.

::: testing/web-platform/tests/css/css-text/text-indent/text-indent-percentage-002.html
@@ +7,5 @@
>  <meta name="flags" content="">
>  <link rel="match" href="reference/text-indent-percentage-002-ref.html">
>  <meta name="assert" content="Percentages in text-indent refer to width of the element's content box">
>  <style>
> +body { background: white; }

Hmm, is this line really needed? Isn't the default canvas color white?
Attachment #9019371 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> > +body { background: white; }
> 
> Hmm, is this line really needed? Isn't the default canvas color white?

The tests has "border-right: 10px solid white" which makes the assumption
that the canvas is by default white.   It's pedantic, but there's no spec
that requires that normatively, AFAIK.

(curiosa: NCSA Mosaic and early versions of Netscape/IE browsers didn't
have a white canvas.  I think it was the native platform's "window"
background color. Typically grey, but I think it varied with the theme.)
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2775681c72a3
[css-text] Resolve 'text-indent' percentage against the content-box inline-size of the box itself, not its containing block.  r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13721 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13721
* continuous-integration/appveyor/pr
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/AAHMlX0kQlq9SOAtDbP1DQ)
https://hg.mozilla.org/mozilla-central/rev/2775681c72a3
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: in-testsuite+
Upstream PR merged
Blocks: 1508509
You need to log in before you can comment on or make changes to this bug.