1237059 - [css-text] Remove the 'true' keyword from text-align[-last]

Status: NEW

(Core :: CSS Parsing and Computation, task, P4)

Description

[Mats Palmgren (inactive)]

This was implemented in bug 929991 behind a pref (disabled by default).
The spec[1] has since removed this feature from these properties, so we
should remove our implementation, which basically means reverting
the changes from bug 929991.

[1] https://drafts.csswg.org/css-text-3/#text-align-property

(I think this might be the last use of the 'true' keyword, in which
case it should be removed.)

Comment 1

[Mats Palmgren (inactive)]

dev-doc-needed: we should remove any mention of 'true' here:
https://developer.mozilla.org/en/docs/Web/CSS/text-align
We can probably do that right now since it was never enabled by default.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-text] Remove tests for text-align[-last]:* true

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-text] Remove the layout implementation for text-align[-last]:* true

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-text] Remove the pref for enabling text-align[-last]:* true

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-text] Remove the style system implementation for text-align[-last]:* true

Comment 6

[Mats Palmgren (inactive)]

The patches corresponds to the changesets in bug 929991 comment 22, in reverse.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63058b179c74

Comment 7

[Anthony Ricaud (:rik)]

Will it still be possible to do the usecase that inspired bug 929991?

Comment 8

[yisibl]

(In reply to Mats Palmgren (:mats) from comment #1)
> dev-doc-needed: we should remove any mention of 'true' here:
> https://developer.mozilla.org/en/docs/Web/CSS/text-align
> We can probably do that right now since it was never enabled by default.

I think Mozilla should push the specification to add the true(unsafe) keyword.

Comment 9

[Jean-Yves Perrier [:teoli]]

I removed 'true' from the doc page. I keep ddn here in case it is finally kept or renamed or whatever (In other words, I will double check what we did once this bug is RESOLVED/SOMETHING :-) )

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #0)
> The spec[1] has since removed this feature from these properties, so we
> should remove our implementation [...]
> 
> [1] https://drafts.csswg.org/css-text-3/#text-align-property

I'm actually not sure this keyword ever actually make it into the spec -- at least, in a local clone of the csswg spec-drafts repo[1], neither of these commands turn up anything related to this keyword:
  hg log -p css-text | grep true
  hg log -p css3-text | grep true

[1] https://hg.csswg.org/drafts/

Comment 11

[Daniel Holbert [:dholbert]]

explaining the commands in comment 10 -- "css3-text" is the old name of "css-text", before 'the great renaming' changeset.  That's where the css-text level 3 spec lives.

There is also css-text-4, and a similar grep command for that directory turns up nothing.

Comment 12

[Daniel Holbert [:dholbert]]

I'm r+'ing patches from a pure "this looks like it's a sane backout patch" perspective.  I haven't followed the spec discussion around this keyword or its removal (or lack-of-introduction, per comment 10), and I didn't find anything about this property being removed/un-suggested from some quick searching (that was the motivation for my grepping in comment 10).

So -- from a "should we do this" perspective, I think you should loop in dbaron, since IIUC he's the one who suggested this feature in the first place, and I suspect he was involved with csswg discussions about this keyword going away.  Setting needinfo=him to make sure he's on-board.

(I'm curious about his thoughts on comment 8, too; and dbaron or mats's response to comment 7.)

Comment 13

[Anthony Ricaud (:rik)]

There is also more discussion in bug 969106.

Comment 14

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

In general, the 'true' keyword has been renamed to 'unsafe' as described in:
https://drafts.csswg.org/css-align/#overflow-values

It's not clear to me that there ever was a spec adding it to text-align.  But I tend to think that there perhaps should be -- I think we should at least discuss whether we want to keep this with the spec editors.

If we keep it, though, we should rename it to unsafe, since the general concept in css-align has been renamed.

Comment 15

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

I tend to think we should probably:
 * keep the feature for now
 * rename it from true to unsafe
 * fix CSSParserImpl::ParseTextAlign to fix the bug that when the pref is disabled, we accept 'true' on its own (I think)
 * poke the spec editors about adding it to css-text-4

Comment 16

[Mats Palmgren (inactive)]

I poked them here:
https://lists.w3.org/Archives/Public/www-style/2016Jan/0278.html

Comment 17

[yisibl]

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #15)
> I tend to think we should probably:
>  * keep the feature for now
>  * rename it from true to unsafe
>  * fix CSSParserImpl::ParseTextAlign to fix the bug that when the pref is
> disabled, we accept 'true' on its own (I think)
>  * poke the spec editors about adding it to css-text-4

I agree with Baron David

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.