Open Bug 1237059 Opened 6 years ago Updated 2 years ago

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

Categories

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

task

Tracking

()

Tracking Status
firefox46 --- affected

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat, Whiteboard: [DocArea=CSS])

Attachments

(4 files)

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.)
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.
Keywords: dev-doc-needed
Whiteboard: [DocArea=CSS]
Assignee: nobody → mats
The patches corresponds to the changesets in bug 929991 comment 22, in reverse.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63058b179c74
Will it still be possible to do the usecase that inspired bug 929991?
(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.
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 :-) )
Attachment #8704430 - Flags: review?(dholbert) → review+
Attachment #8704428 - Flags: review?(dholbert) → review+
(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/
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.
Attachment #8704427 - Flags: review?(dholbert) → review+
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.)
Flags: needinfo?(dbaron)
There is also more discussion in bug 969106.
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.
Flags: needinfo?(dbaron)
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
Blocks: 969106
(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
Keywords: site-compat
Type: defect → task
You need to log in before you can comment on or make changes to this bug.