1784022 - Convert text-emphasis-position #defines to enum classes.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Anurag Kalia]

+++ This bug was initially created as a clone of Bug #1625745 +++

Comment 1

[Anurag Kalia]

@emilio: I would like to work on this bug, but I am not sure how to assign it to myself. Once this goes through, I will work on similar tasks.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hi Anurag! The bug should be auto-assigned to you when you submit a patch. Let me know if you need any help with that.

Comment 3

[Anurag Kalia]

Sure! I have taken a deep dive into the code. Before I dive further and implement this, I wanted to run my approach by you once.

First of all, I see a bug in the current implementation. According to the spec (https://drafts.csswg.org/css-text-decor/#text-emphasis-position-property), Valid rule is [ over | under ] && [ right | left ]? but in the current implementation right | left is a required param. I intend to fix it in the same patch.

Now, over to the implementation:

1. Because there are two groups (over | under) and (right | left), I intend to use bitflags! macro for the same and intend to derive it using #css(bitflags(...)) ala servo/components/style/values/specified/box.rs#ScrollbarGutter.
2. Moreover, the current use case is not well-supported by #css(bitflags(...)). Specifically, we do not have any single flag and we need to supply right|left automatically in their absence. Hence, I intend to modify bitflags derivation too by adding a new flag_groups attribute that specifies various groups of mixed flags (2 groups - left|right and over|under in this case), specifies whether each group is required, and specifies whether each group has a default flag in case it is optional.

Comment 4

[Anurag Kalia]

Let me also know if the explanation is confusing and how to format it to make it more readable.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Anurag Kalia from comment #3)

Sure! I have taken a deep dive into the code. Before I dive further and implement this, I wanted to run my approach by you once.

First of all, I see a bug in the current implementation. According to the spec (https://drafts.csswg.org/css-text-decor/#text-emphasis-position-property), Valid rule is [ over | under ] && [ right | left ]? but in the current implementation right | left is a required param. I intend to fix it in the same patch.

So, that's worth fixing (and other browsers do accept the optional param, see https://wpt.fyi/results/css/css-text-decor/parsing/text-emphasis-position-computed.html). However it might be worth doing it in a follow-up bug instead (just to focus on the refactoring on this one, and then changing behavior on another bug, wdyt?).

Now, over to the implementation:

1. Because there are two groups (over | under) and (right | left), I intend to use bitflags! macro for the same and intend to derive it using #css(bitflags(...)) ala servo/components/style/values/specified/box.rs#ScrollbarGutter.

That sounds good.

2. Moreover, the current use case is not well-supported by #css(bitflags(...)). Specifically, we do not have any single flag and we need to supply right|left automatically in their absence. Hence, I intend to modify bitflags derivation too by adding a new flag_groups attribute that specifies various groups of mixed flags (2 groups - left|right and over|under in this case), specifies whether each group is required, and specifies whether each group has a default flag in case it is optional.

Is this needed? I'd rather avoid complicating the derive code unless we have multiple use cases for it. At a glance, it seems we can avoid such complications with the existing infrastructure, with a small tweak:

We'd mark all the flags as mixed flags, and then validate_mixed can take care of rejecting invalid groups:

impl TextEmphasisPosition {
    fn has_exactly_one_of(&self, left: Self, right: Self) {
        self.intersects(left | right) && !self.contains(left | right)
    }
    fn validate(&self) -> bool {
        self.has_exactly_one_of(Self::OVER, Self::UNDER) && self.has_exactly_one_of(Self::LEFT, Self::RIGHT)
    }
}

That should be enough for our refactoring purposes.

Then in the bug where we make left | right optional, we can change the derive code to pass a mutable reference, and instead do:

fn validate(&mut self) -> bool {
    if !self.has_exactly_one_of(Self::OVER, Self::UNDER) {
        return false;
    }
    if self.intersects(Self::LEFT) {
        return !self.intersects(Self::RIGHT);
    }
    self.remove(Self::RIGHT); // Right is optional and shouldn't be serialized.
    true
}

What do you think? That should be simpler, at a glance, and avoids over-generalizing. If we end up with more flags that need this infrastructure and it becomes complicated to maintain we can add support to the derive code, but till then I think this is more preferable.

Comment 6

[Anurag Kalia]

It makes sense to me and thank you for the wpt dashboard. Did not know something like this existed! I will only do refactoring in this ticket.

Just a small question for the followup validate function. The only valid states afterwards will be: OVER, UNDER, OVER LEFT and UNDER LEFT, if I understand correctly. Can we tweak it to the following?

fn validate(&mut self) -> bool {
    if !self.has_exactly_one_of(Self::OVER, Self::UNDER) {
        return false;
    }
    if self.intersects(Self::LEFT) {
        return !self.intersects(Self::RIGHT);
    }
    self.set(Self::RIGHT); // Right is optional and should be serialized if LEFT does not exist.
    true
}

This will help avoid surprises in usages like this:

(mTextEmphasisPosition & StyleTextEmphasisPosition::RIGHT
? eSideRight
: eSideLeft)

Devs will have to constantly keep remembering RIGHT is optional when the struct does not lend that information readily. But it is possible I am overthinking this and the codebase may have many other examples for the same without problems. 

Anyway, this is a very minor point and I agree with your general approach for now. I will start with it. Thank you for the patience!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think what we should do is probably just not generate StyleTextEmphasisPosition::RIGHT in C++, or add a convenient mozilla::Side Side() const; function or something. The reason why we can't add RIGHT unconditionally if not present is that we should serialize to the shortest form, so if you write over right, we should serialize back just over.

Comment 8

[Anurag Kalia]

Attachment: Bug 1784022 - [refactor] Convert text-emphasis-position #defines to enum classes r=emilio!

Comment 9

[Anurag Kalia]

Attachment: Bug 1784022 - [refactor] Convert text-emphasis-position #defines to enum classes r=emilio!

Comment 10

[Anurag Kalia]

Hi! treeherder has some 5 errors. Are they related to our revision? I ran it locally and it seems to work. So, I am not sure what to do now.

Link: https://treeherder.mozilla.org/push-health/push?repo=try&revision=6464a5f45e787ad25333812f809a8fc9a0bc0598&tab=tests&testGroup=pr&selectedTest=toolkitcontenttestschrometestpopupattributexhtmlapplicationterminatedwithexitcode6&selectedTaskId=391516300&selectedJobName=+toolkit%2Fcontent%2Ftests%2Fchrome%2Ftest_popup_attribute.xhtml+%7C+application+terminated+with+exit+code+6+test-linux1804-64-qr%2Fopt-mochitest-chrome-spi-nw-1proc-1

Comment 11

[Emilio Cobos Álvarez (:emilio)]

They already have some bugs on file. They seem intermittent and totally unrelated to your changes, so patch looks good to land. Thanks!

Comment 12

[Anurag Kalia]

Hi! Just one very last question on this. The lando link says "There has been no attempt to land revisions in this stack. " I am under the impression I don't have access to it. What happens now to get this merged in the main code repo?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Err, d'oh, I just forgot to click the land button for you. You should consider applying for L1 commit access (that would give you try access, see https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/). I'd be happy to vouch for you. You can get L3 access after a while, and then you can land your own patches. Till then, asking me or someone else to land it is totally fine :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edeb8da0c39b
[refactor] Convert text-emphasis-position #defines to enum classes r=emilio

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/edeb8da0c39b