Closed Bug 929991 Opened 6 years ago Closed 6 years ago

Implement 'true' alignment for text-align

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: mats, Assigned: mats)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files, 4 obsolete files)

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>
Attached patch Style system part (obsolete) — Splinter Review
Attachment #821605 - Flags: review?(cam)
Attachment #821606 - Flags: review?(cam)
Attached patch Layout bitsSplinter Review
Attachment #821607 - Flags: review?(roc)
Attached patch reftests (obsolete) — Splinter Review
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 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.
Attachment #821605 - Flags: review?(cam) → review+
(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 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.
Attachment #821606 - Flags: review?(cam) → review+
Attached patch Style system part, v2 (obsolete) — Splinter Review
(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
Attachment #821605 - Attachment is obsolete: true
Attachment #821671 - Flags: review?(cam)
(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.
(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
Attachment #821606 - Attachment is obsolete: true
Attachment #822184 - Flags: review+
SetIdent(eCSSKeyword_true) it is
Attachment #821671 - Attachment is obsolete: true
Attachment #821671 - Flags: review?(cam)
Attachment #822186 - Flags: review?(cam)
Attached patch testsSplinter Review
Added "true" and "true true" as invalid values (always)
and "true left" as valid/invalid depending on the pref value.
Attachment #822186 - Attachment description: tyle system part, v3 → Style system part, v3
Attachment #822184 - Attachment description: ut it behind a preference (disabled by default), v2 → Put it behind a preference (disabled by default), v2
Hmm, it seems the tests failed for some reason " uncaught exception: Error getting pref at :0" ...
Comment on attachment 822186 [details] [diff] [review]
Style system part, v3

r=me
Attachment #822186 - Flags: review?(cam) → review+
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.
Ah, indeed. Copy-pasting without looking to closely...  new try:
https://tbpl.mozilla.org/?tree=Try&rev=cd447dd5a3c3
Whiteboard: [DocArea=CSS]
Mats: What's the status on turning this pref on? Should we open a bug? We're interested to use that in Gaia.
Flags: needinfo?(matspal)
Blocks: 969106
(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.
Flags: needinfo?(matspal)
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 :-)
Flags: needinfo?(mats)
(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.
Flags: needinfo?(mats) → needinfo?(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/
Flags: needinfo?(dholbert)
Depends on: 1151684
Blocks: 1237059
Removing DDN. This story continues in bug 1237059 (see especially bug 1237059 comment 9).
Keywords: dev-doc-needed
Whiteboard: [DocArea=CSS]
Depends on: 1500885
You need to log in before you can comment on or make changes to this bug.