929991 - Implement 'true' alignment for text-align

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Mats Palmgren (inactive)]

As suggested by dbaron in:
http://lists.w3.org/Archives/Public/www-style/2013Oct/0591.html

<quote>
http://dev.w3.org/csswg/css-align/#overflow-values describes the
concepts of 'safe' and 'true' alignment.

The 'text-align' property defaults to 'safe' alignment.  I think it
would be useful to add an option for 'true' alignment.
Syntactically, I think this could be done by adding an optional
'true' keyword that could come before or after the current value
space of the 'text-align' property.  I think this would be a useful
addition to css-text-4.

The use case I heard was that it would be particularly useful when
combined with 'text-overflow: ellipsis', when showing data where the
end is more important than the start.  For example, a constrained
space showing a phone number, where the end of the number might be
considered more important to show than the start, could be aligned
as 'text-align: true right; text-overflow: ellipsis' so that any
overflowing phone numbers would have the start of the number
overflowing (e.g., "0016177616200" being shown as "...7616200").

-David
</quote>

Comment 1

[Mats Palmgren (inactive)]

Attachment: Style system part

Comment 2

[Mats Palmgren (inactive)]

Attachment: Put it behind a preference (disabled by default)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Layout bits

Comment 4

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 821605 [details] [diff] [review]
Style system part

I made "text-align: true right" the canonical form, but 'right true'
is equivalent.

Comment 6

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=d12665c0e4f3
https://tbpl.mozilla.org/?tree=Try&rev=15fa773b241c

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 821605 [details] [diff] [review]
Style system part

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

Can you add values using "true" to the other_values and invalid_values arrays in property_database.js?  r=me with that and the below.

::: layout/style/nsComputedDOMStyle.cpp
@@ +2759,5 @@
>    return val;
>  }
>  
>  CSSValue*
>  nsComputedDOMStyle::DoGetTextAlign()

Just like you factor out the parsing, can you factor out the guts of DoGetTextAlign and DoGetTextAlignLast, since they're nearly the same?

::: layout/style/nsRuleNode.cpp
@@ +3993,5 @@
> +        }
> +      } else if (eCSSUnit_String == textAlignValue->GetUnit()) {
> +        NS_NOTYETIMPLEMENTED("align string");
> +      }
> +    } else if (eCSSUnit_Inherit == textAlignValue->GetUnit()) {

This should check for eCSSUnit_Unset too, since text-align is an inherited property so 'unset' means the same as 'inherit'.

@@ +4002,5 @@
>                  parentText->mTextAlign,
>                  NS_STYLE_TEXT_ALIGN_DEFAULT, 0, 0, 0, 0);
> +  }
> +
> +  // text-align-last: enum, pair(enum|string), inherit, initial

"pair(string)"

@@ +4015,5 @@
> +      if (textAlignLastValue->GetIntValue() == NS_STYLE_TEXT_ALIGN_TRUE) {
> +        textAlignLastValue = &textAlignLastValuePair.mYValue;
> +      }
> +    }
> +  } else if (eCSSUnit_Inherit == textAlignLastValue->GetUnit()) {

Check for 'unset' here too.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Can you add values using "true" to the other_values and invalid_values
> arrays in property_database.js?

Oh, you'll need to do this in a section of the file at the bottom, since those values will be behind a pref.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 821606 [details] [diff] [review]
Put it behind a preference (disabled by default)

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

r=me with the comments below addressed.

::: layout/base/nsLayoutUtils.cpp
@@ +95,5 @@
>  using mozilla::image::Orientation;
>  
>  #define FLEXBOX_ENABLED_PREF_NAME "layout.css.flexbox.enabled"
>  #define STICKY_ENABLED_PREF_NAME "layout.css.sticky.enabled"
> +#define TEXT_ALIGN_TRUE_ENABLED_PREF_NAME "layout.css.text_align_true.enabled"

Maybe "layout.css.text-align-true.enabled" or "layout.css.text-align-true-value.enabled"?  (Suggesting the latter since we have "layout.css.unset-value.enabled".)  It seems hyphens are more prevalent in pref names, including those that include the (hyphenated) named of a property in the pref name.

@@ +238,5 @@
> +  }
> +
> +  // OK -- now, stomp on or restore the "true" entry in the keyword tables,
> +  // depending on whether the pref is enabled vs. disabled.
> +  if (sIndexOfTrueInTextAlignTable >= 0) {

I think you can just MOZ_ASSERT that you found true in the tables.

::: layout/style/nsCSSParser.cpp
@@ +9733,5 @@
>    if (!ParseVariant(left, VARIANT_KEYWORD, aTable)) {
>      return false;
>    }
>  
> +  if (!Preferences::GetBool("layout.css.text_align_true.enabled", false)) {

Instead of calling GetBool in here, can you add a method to nsLayoutUtils that returns whether the pref's value is true, and which caches the value?  See for example nsLayoutUtils::UnsetValueEnabled and various other methods on nsLayoutUtils that do it this way.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Style system part, v2

(In reply to Cameron McCormack (:heycam) from comment #7)
> Can you add values using "true" to the other_values and invalid_values
> arrays in property_database.js?  r=me with that and the below.

I don't know how to test stuff that is preffed off by default in
property_database.js -- I don't think we support that.  Or do you mean
explicitly test "true right" is invalid?

I added a simple CSSOM test in the reftest for know... and, uhm,
it did indeed detect a bug... I should have used NS_STYLE_TEXT_ALIGN_TRUE
instead of eCSSKeyword_true in the call to ValueToKeywordEnum... I guess
that could just be SetIdent(eCSSKeyword_true) if you prefer.


> Just like you factor out the parsing, can you factor out the guts of
> DoGetTextAlign and DoGetTextAlignLast, since they're nearly the same?

Good suggestion, fixed.


> This should check for eCSSUnit_Unset too, since text-align is an inherited
> property so 'unset' means the same as 'inherit'.

Fixed.


> > +  // text-align-last: enum, pair(enum|string), inherit, initial
> 
> "pair(string)"

I assume you mean pair(enum) - fixed.

> Check for 'unset' here too.

Fixed

Comment 11

[Mats Palmgren (inactive)]

Attachment: reftests + simple CSSOM test

https://tbpl.mozilla.org/?tree=Try&rev=d17107fbfc87

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Mats Palmgren (:mats) from comment #10)
> I don't know how to test stuff that is preffed off by default in
> property_database.js -- I don't think we support that.  Or do you mean
> explicitly test "true right" is invalid?

Oh, well you could indeed "true true" as an invalid value regardless of the pref.  But the way you can add 
"true right" as a valid value if the pref is set is by doing things like in the very last if-block of the file.  So like:

  if (SpecialPowers.getBoolPref("layout.css.text-align-true.enabled")) {
    gCSSProperties["text-align"].other_values.push("true left");
    ...
  }

> I added a simple CSSOM test in the reftest for know... and, uhm,
> it did indeed detect a bug... I should have used NS_STYLE_TEXT_ALIGN_TRUE
> instead of eCSSKeyword_true in the call to ValueToKeywordEnum... I guess
> that could just be SetIdent(eCSSKeyword_true) if you prefer.

Yes SetIdent(eCSSKeyword_true) is better.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Put it behind a preference (disabled by default), v2

(In reply to Cameron McCormack (:heycam) from comment #9)
> Maybe "layout.css.text-align-true.enabled" or
> "layout.css.text-align-true-value.enabled"?  (Suggesting the latter since we
> have "layout.css.unset-value.enabled".)  It seems hyphens are more prevalent
> in pref names, including those that include the (hyphenated) named of a
> property in the pref name.

Fixed (layout.css.text-align-true-value.enabled)

> I think you can just MOZ_ASSERT that you found true in the tables.

Fixed

> Instead of calling GetBool in here, can you add a method to nsLayoutUtils
> that returns whether the pref's value is true, and which caches the value? 

Fixed

Comment 14

[Mats Palmgren (inactive)]

Attachment: Style system part, v3

SetIdent(eCSSKeyword_true) it is

Comment 15

[Mats Palmgren (inactive)]

Attachment: tests

Added "true" and "true true" as invalid values (always)
and "true left" as valid/invalid depending on the pref value.

Comment 16

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a93651513804

Comment 17

[Mats Palmgren (inactive)]

Hmm, it seems the tests failed for some reason " uncaught exception: Error getting pref at :0" ...

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 822186 [details] [diff] [review]
Style system part, v3

r=me

Comment 19

[Cameron McCormack (:heycam)]

You're using "layout.css.text-align-true-value.enabled" in the reftest manifest and in all.js, but "layout.css.text-align-true.enabled" in property_database.js.

Comment 20

[Mats Palmgren (inactive)]

Ah, indeed. Copy-pasting without looking to closely...  new try:
https://tbpl.mozilla.org/?tree=Try&rev=cd447dd5a3c3

Comment 21

[Mats Palmgren (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa12689a261f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/477cac85b593
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f25d161ad864
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/25d231fef57c

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aa12689a261f
https://hg.mozilla.org/mozilla-central/rev/477cac85b593
https://hg.mozilla.org/mozilla-central/rev/f25d161ad864
https://hg.mozilla.org/mozilla-central/rev/25d231fef57c

Comment 23

[Anthony Ricaud (:rik)]

Mats: What's the status on turning this pref on? Should we open a bug? We're interested to use that in Gaia.

Comment 24

[Mats Palmgren (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #23)
> Mats: What's the status on turning this pref on? Should we open a bug? We're
> interested to use that in Gaia.

Thanks for reminding me, I have filed bug 969106.

Comment 25

[Jean-Yves Perrier [:teoli]]

Hi!

I'm planning to document this, and I would like to be sure if I have correctly understand where this 'true' is coming from. So are the following assertions correct?

1. This 'true' value is one of the two values of <overflow-position>, defined at http://dev.w3.org/csswg/css-align/#typedef-overflow-position
2. The keyword 'safe' is not implemented in text-align (in Gecko).
3. Once 'safe' is implemented, the syntax for text-align (CSS Text level 4) will be: start | end | left | right | center | justify | match-parent | start end && <overflow-position>? This is what should appear in the draft.
4. As implemented in this bug, the canonical order is equivalent of the one defined by the syntax true? && [ start | end | left | right | center | justify | match-parent | start end ] . This what I understand from comment #5.
5. The initial value for the <overflow-position> part of any property using it depends of the type of the container (and only it).
6. Several properties introduced in CSS Flexbox L1, but now defined in CSS Box Alignement L3, will make use of <overflow-position>;  it is not yet the case in Gecko.

Hope to be not far away from the truth :-)

Comment 26

[Mats Palmgren (inactive)]

(In reply to Jean-Yves Perrier [:teoli] from comment #25)
> 1. This 'true' value is one of the two values of <overflow-position>,
> defined at http://dev.w3.org/csswg/css-align/#typedef-overflow-position

Correct, but we implemented 'true' on the 'text-align' and 'text-align-last'
properties (where it used to live).  The new properties* having 'safe|true'
are not generally implemented implemented in Gecko, although some are for
flex containers (perhaps not the <overflow-position> part though - you
ahve to ask dholbert)

Anyway, we won't expose our implementation of 'true' to the web in
its current form.

(*)
http://dev.w3.org/csswg/css-align/#property-index

> 2. The keyword 'safe' is not implemented in text-align (in Gecko).

Correct.

> 3. Once 'safe' is implemented, the syntax for text-align (CSS Text level 4)
> will be: start | end | left | right | center | justify | match-parent |
> start end && <overflow-position>? This is what should appear in the draft.

I'm guessing <overflow-position> is only intended for the properties
at (*) and will NOT be added to the text-align properties.
You have to ask fantasai for an authoritative answer.

> 4. As implemented in this bug, the canonical order is equivalent of the one
> defined by the syntax true? && [ start | end | left | right | center |
> justify | match-parent | start end ] . This what I understand from comment
> #5.

Yes.

> 5. The initial value for the <overflow-position> part of any property using
> it depends of the type of the container (and only it).

Yes, as far as I understand http://dev.w3.org/csswg/css-align/#overflow-values
(the spec uses the term "default" rather than "initial" though; I guess it's
more appropriate since 'overflow-position' isn't a property per se)

> 6. Several properties introduced in CSS Flexbox L1, but now defined in CSS
> Box Alignement L3, will make use of <overflow-position>;  it is not yet the
> case in Gecko.

I believe so; dholbert should know more about the implementation status for
flex containers.

It's probably not worth documenting this until we have implemented
<overflow-position> on some CSS Box Alignment properties.

Comment 27

[Daniel Holbert [:dholbert]]

We haven't implemented the <overflow-position> a.k.a. "true | safe" values for the align-[self|items|content] or justify-content properties (the properties that were originally defined in CSS Flexbox that have now been spun off into the Box Alignment spec).

For now, we've only implemented the values listed for these properties in the flexbox spec itself, http://www.w3.org/TR/css-flexbox-1/

Comment 28

[yisibl]

I updated a simple note: https://developer.mozilla.org/en-US/docs/Web/CSS/text-align

Comment 29

[Florian Scholz (Open Web Docs)]

Removing DDN. This story continues in bug 1237059 (see especially bug 1237059 comment 9).