1341018 - Convert NS_STYLE_TEXT_ALIGN_* and NS_STYLE_VERTICAL_ALIGN_* to an enum class

Status: RESOLVED FIXED

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

Description

[Jeremy Chen ([e-mail])]

As the comments listed in both NS_STYLE_TEXT_ALIGN_* and NS_STYLE_VERTICAL_ALIGN_* definitions said, there are a few html cases where an object can have both types of alignment applied with a single attribute (see [1] as an example). So I plan to combine these two enums into one single enum class. Here's the proposed naming:

enum class StyleAlign : uint8_t {
  // text-justify
  TextAlignStart,
  TextAlignLeft,
  TextAlignRight,
  TextAlignCenter,
  TextAlignJustify,
  TextAlignChar,
  TextAlignEnd,
  TextAlignAuto,
  TextAlignMozCenter,
  TextAlignMozRight,
  TextAlignMozLeft,
  TextAlignMozCenterOrInherit,
  TextAlignUnsafe,
  TextAlignMatchParent,

  // vertical-align
  VerticalAlignBaseLine,
  VerticalAlignSub,
  VerticalAlignSuper,
  VerticalAlignTop,
  VerticalAlignTextTop,
  VerticalAlignMiddle,
  VerticalAlignTextBottom,
  VerticalAlignBottom,
  VerticalAlignMiddleWithBaseline
}

Hi Cameron, do you think this is a good idea?


[1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/dom/html/HTMLLegendElement.cpp#44-51

Comment 1

[Jeremy Chen ([e-mail])]

> 
> enum class StyleAlign : uint8_t {
>   // text-justify
Oops, this should be `text-align`

>   TextAlignStart,
>   TextAlignLeft,
>   TextAlignRight,
>   TextAlignCenter,
>   TextAlignJustify,
>   TextAlignChar,
>   TextAlignEnd,
>   TextAlignAuto,
>   TextAlignMozCenter,
>   TextAlignMozRight,
>   TextAlignMozLeft,
>   TextAlignMozCenterOrInherit,
>   TextAlignUnsafe,
>   TextAlignMatchParent,
> 
>   // vertical-align
>   VerticalAlignBaseLine,
>   VerticalAlignSub,
>   VerticalAlignSuper,
>   VerticalAlignTop,
>   VerticalAlignTextTop,
>   VerticalAlignMiddle,
>   VerticalAlignTextBottom,
>   VerticalAlignBottom,
>   VerticalAlignMiddleWithBaseline
> }

Comment 2

[Jeremy Chen ([e-mail])]

Since text-align-last is using a subset of keywords that text-align uses, we might want to keep the definitions still until we convert text-align-last to use an enum class as well.

Hi Xidorn, these features were more or less touched by you before, any thoughts?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #0)
> Hi Cameron, do you think this is a good idea?

No, it's not. Use Variant [1] at where those two needs to be combined, and define them separately.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> Since text-align-last is using a subset of keywords that text-align uses, we
> might want to keep the definitions still until we convert text-align-last to
> use an enum class as well.
> 
> Hi Xidorn, these features were more or less touched by you before, any
> thoughts?

Any reason text-align-last cannot just use the same enum class at the same time?


[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h

Comment 4

[Cameron McCormack (:heycam)]

I agree with Xidorn that using a Variant would be better to distinguish these two cases, rather than combining them into one enum.

One thing to note is that these values as used for <legend align=""> attribute parsing are not then used as values for text-align or vertical-align themselves, or in functions that work on those values for layout.  Instead it looks like we just do some switching on the values:

  http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/forms/nsFieldSetFrame.cpp#514-536

This makes me think that we should be using a separate enum to represent legend align="" values, rather than trying to re-use the style consts or enums for text-align and vertical-align.

Note also that nsFieldSetFrame here is working on values as returned by GetLogicalAlign(), which converts NS_STYLE_TEXT_ALIGN_{LEFT,RIGHT} into NS_STYLE_TEST_ALIGN_{START,END}.  So if we had a separate enum to represent values of <legend align=""> then we have to add the two logical values (Start and End) or perhaps have a separate enum for the complete set of logical align="" values (Start, End, Center, Top, Bottom).

Unfortunately, nsAttrValue doesn't support storage of attributes values as enum classes -- nsAttrValue::EnumTable has values stored as int16_ts.

Comment 5

[Jeremy Chen ([e-mail])]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #0)
> > Hi Cameron, do you think this is a good idea?
> 
> No, it's not. Use Variant [1] at where those two needs to be combined, and
> define them separately.

Nice! Variant it is!! Thank you. :)
 
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> > Since text-align-last is using a subset of keywords that text-align uses, we
> > might want to keep the definitions still until we convert text-align-last to
> > use an enum class as well.
> > 
> > Hi Xidorn, these features were more or less touched by you before, any
> > thoughts?
> 
> Any reason text-align-last cannot just use the same enum class at the same
> time?

Not particularly, while raising this question, I just don't want to merge text-align-last into the large union set listed in comment 0. Since using Variant is a solution, we can definitely use the same enum class for text-align-last.

> 
> [1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h

Comment 6

[keshavmanghat]

Hi, I would like to work on this bug, it would be my first contribution and any help would be great.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

That would be appreciated.

So in this bug, it is better having two or three patches.

In the first patch, you should create a new enum class, probably in dom/html/HTMLLegendElement.h, probably called LegendAlignValue, which contains { Left, Right, Center, Bottom, Top, InlineStart, InlineEnd }, and use it to replace the usage of NS_STYLE_TEXT_ALIGN_* and NS_STYLE_VERTICAL_ALIGN_* in HTMLLegendElement.cpp[1][2] and nsFieldSetFrame.cpp[3].

In the second patch, you should convert constants NS_STYLE_TEXT_ALIGN_*[4] into an enum class called StyleTextAlign and update all its callsites.

And similarly, in the third patch, you should convert NS_STYLE_VERTICAL_ALIGN_* into StyleVerticalAlign[5].

In the second and third patch, you may need to update the corresponding Servo side code, and add "gecko_enum_prefix" argument to some places[6][7].

Having written the above, I guess it may not as easy as a normal good-first-bug... But it shouldn't be too bad!

Let me know if you have any question. You can just leave comment in this bug and needinfo me, or ping me on IRC in #layout channel. (When I'm not around, emilio should be there to help :)


[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/html/HTMLLegendElement.cpp#44-51
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/layout/forms/nsLegendFrame.cpp#67-77
[3] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/layout/forms/nsFieldSetFrame.cpp#544-563
[4] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/layout/style/nsStyleConsts.h#766-782
[5] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/layout/style/nsStyleConsts.h#838-850
[6] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/servo/components/style/properties/gecko.mako.rs#4662-4664
[7] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/servo/components/style/properties/longhand/inherited_text.mako.rs#109-114

Comment 8

[keshavmanghat]

Thank you, I will get started with that ASAP

Comment 9

[keshavmanghat]

Hey, How do you think I can work on two bugs at the same time, cause I want to work on this one but just submitted another bug for review and creating patches for this one would include changes that I made for the previous one.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Please use the needinfo flag (you can do so via filling :xidorn in the box below) next time, so that I wouldn't overlook the question, thanks.

It is fine that a patch is based on another patch in another bug. If there are code change overlaps, you may include the other bug number in the commit message so that reviewer can check.

Comment 11

[Savo]

:xidorn Hi, it looks like no one has worked on this in a while would it be possible for me to take over? I have submitted a patch for the first part of the refactor.

Comment 12

[Savo]

Attachment: Bug 1341018 - Create enum class LegendAlignValue to replace NS_STYLE_TEXT_ALIGN_* and NS_STYLE_VERTICAL_ALIGN_* in legend code.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Savo, thanks for your interest!

I haven't been heavily involved in style system for a while, but I think the current trend there is to use Rust type via cbindgen rather than creating new enum class. Emilio can probably confirm.

I think you can have a look at https://phabricator.services.mozilla.com/D11580 or https://phabricator.services.mozilla.com/D13886 as examples of how this can be done.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Unassigning the original assignee for now given the inactivity anyway.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)

Savo, thanks for your interest!

I haven't been heavily involved in style system for a while, but I think the current trend there is to use Rust type via cbindgen rather than creating new enum class. Emilio can probably confirm.

I think you can have a look at https://phabricator.services.mozilla.com/D11580 or https://phabricator.services.mozilla.com/D13886 as examples of how this can be done.

We can indeed do that, and it'd be slightly preferable, but it may be easier to keep using the same pattern Savo has been using in bug 1586068 which gets us most of the way there and may be marginally simpler, and then moving the definition to rust is as simple of any of those patches.

That is to say, either way should be fine, though using rust directly is indeed slightly preferable, getting it just to use an enum class makes that trivial, and is progress either way :)

Comment 16

[Savo]

Attachment: Bug 1341018 - getlogicalalign now returns legendalignvalue as suggested by emilio; r?emilio

Depends on D48285

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cbbf288180c
Create enum class LegendAlignValue to replace NS_STYLE_TEXT_ALIGN_* and NS_STYLE_VERTICAL_ALIGN_* in legend code. r=emilio

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Thanks for the patch Savo!

I took the liberty to squash them and land them. There's still work needed here to actually remove the constants, but getting rid of the legend mix is great!

I assume you don't plan to finish this? Let me know if you do and I'll gladly re-assign to you.

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7cbbf288180c

Comment 20

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

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?