1350140 - stylo: Implement all the pending state pseudo-classes.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

I felt like doing it today.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Also implements :link, :visited, and :any-link more efficiently, and stops
matching :-moz-read-only in everything that is not read-write, which is kind of
dumb, and probably creates some artifacts.

Review commit: https://reviewboard.mozilla.org/r/123284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123284/

Comment 2

[Bobby Holley (:bholley)]

emilio++

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123284/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123284/diff/2-3/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

The second review request contains a bug in the handling of :any-link. There's still a whole lot of unexpected pass orange:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e105dd09ebbe87687edc17f9f828724430e9077a

This one is with the :any-link issue fixed:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=044fa37988243c68c6148ba005f6ffcbdf0e3292

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

https://reviewboard.mozilla.org/r/123284/#review125744

Nice!

::: dom/events/EventStates.h:294
(Diff revision 3)
> -// #define NS_EVENT_STATE_?????????? NS_DEFINE_EVENT_STATE_MACRO(44)
> -// Element is highlighted (devtools inspector)
> -#define NS_EVENT_STATE_DEVTOOLS_HIGHLIGHTED NS_DEFINE_EVENT_STATE_MACRO(45)
> -// Element is an unresolved custom element candidate
> -#define NS_EVENT_STATE_UNRESOLVED NS_DEFINE_EVENT_STATE_MACRO(46)
> -// Element is transitioning for rules changed by style editor
> +/*
> + * Bits below here do not have Servo-related ordering constraints.
> + *
> + * Remember to change NS_EVENT_STATE_HIGHEST_SERVO_BIT at the top of the file if
> + * this changes!
> + */

I guess any new event state bits we have will need to be handled in Servo too, so I guess we can just get rid of this comment (and NS_EVENT_STATE_HIGHEST_SERVO_BIT).

::: servo/components/style/element_state.rs:15
(Diff revision 3)
> +    /// TODO(emilio): We really really want to use the NS_EVENT_STATE bindings
> +    /// for this.

In the meantime, could we add assertions that the values are the same, like we do for the RestyleHint flags?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Quick update with this: This patch uncovered a bug with how outline-width is inherited. With that (and border inheritance also fixed), I'm still running into some test failures that I need to look into.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: Update test expectations.

Review commit: https://reviewboard.mozilla.org/r/123984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123984/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review commit: https://reviewboard.mozilla.org/r/123986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123986/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: Avoid dumb copy-constructor.

Review commit: https://reviewboard.mozilla.org/r/123988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123988/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review commit: https://reviewboard.mozilla.org/r/123990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123990/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123284/diff/3-4/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

https://reviewboard.mozilla.org/r/123986/#review126586

::: servo/components/style/properties/gecko.mako.rs:440
(Diff revision 1)
>      #[allow(non_snake_case)]
>      pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
>          % if round_to_pixels:
>          let au_per_device_px = Au(self.gecko.mTwipsPerPixel);
> +        % if save_specified_in:
> +            self.gecko.${save_specified_in} = v.0;

Nit: the Mako gunk around here doesn't seem to affect the indenting of the Rust code, so I would deindent this line.  Well, I guess we're a bit inconsistent throughout the file, but let's be consistent within the function at least. :-)

::: servo/components/style/properties/gecko.mako.rs:450
(Diff revision 1)
> +        % if save_specified_in:
> +            self.set_${ident}(Au(other.gecko.${save_specified_in}));
> +        % else:
> +            self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name};
> +        % endif

Can this just be:

  % if save_specified_in:
  self.gecko.${save_specified_in} = other.gecko.${save_specified_in};
  % endif
  self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name};

?  Or do we need to call set_${ident} because we might have a different mTwipsPerPixelValue?  And if that's true, why don't we have to call set_${ident} when save_specified_in wasn't specified, but round_to_pixels is true?

::: servo/components/style/properties/properties.mako.rs:2324
(Diff revision 1)
>      // The initial value of outline width may be changed at computed value time.
> +    //
> +    // FIXME(emilio): Is there a spec backing this out?

s/out/up/

But yes, the initial value of outline-width is medium, but if outline-style is none, then outline-width computes to 0.  Given hidden isn't a valid value of outline-style, it might be worth changing this to check for none explicitly, or to assert or something.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

https://reviewboard.mozilla.org/r/123988/#review126598

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

https://reviewboard.mozilla.org/r/123990/#review126600

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123986/diff/1-2/

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123988/diff/1-2/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123990/diff/1-2/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123986/diff/2-3/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123988/diff/2-3/

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123990/diff/2-3/

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Requesting re-review on this one since it's significantly different from the initial patch.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123986/diff/3-4/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123988/diff/3-4/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123990/diff/3-4/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1350140: more test expectation updates.

Review commit: https://reviewboard.mozilla.org/r/124246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124246/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123284/diff/4-5/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851759 [details]
Bug 1350140: Update test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123984/diff/1-2/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123986/diff/4-5/

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123988/diff/4-5/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123990/diff/4-5/

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

https://reviewboard.mozilla.org/r/123986/#review127118

::: servo/components/style/properties/gecko.mako.rs:828
(Diff revision 5)
> +                    "mBorderStyle[%s]" % side.index,
> +                    border_style_keyword,
> +                    on_set="update_border_%s" % side.ident,
>                      need_clone=True) %>
>  
> +    // This is needed because the initial mComputedBorder value is set to zero.

Thanks for adding/expanding this comment.

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8851988 [details]
Bug 1350140: more test expectation updates.

https://reviewboard.mozilla.org/r/124246/#review127120

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123284/diff/5-6/

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851759 [details]
Bug 1350140: Update test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123984/diff/2-3/

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851760 [details]
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123986/diff/5-6/

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123988/diff/5-6/

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123990/diff/5-6/

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851988 [details]
Bug 1350140: more test expectation updates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124246/diff/1-2/

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851759 [details]
Bug 1350140: Update test expectations.

https://reviewboard.mozilla.org/r/123984/#review127214

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850768 [details]
Bug 1350140: stylo: Implement all the remaining state pseudo-classes.

https://reviewboard.mozilla.org/r/123284/#review125744

> In the meantime, could we add assertions that the values are the same, like we do for the RestyleHint flags?

We can't (at least without a bit more work and some macro hackery), because I hadn't realised the macros don't define plain integers.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcad8df0a68b
stylo: Implement all the remaining state pseudo-classes. r=heycam
https://hg.mozilla.org/integration/autoland/rev/52fff0dd38c1
Update test expectations. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1d26488c7af2
Fix computation of border and outline-width when the border or outline style change. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ebeac2783f24
Avoid dumb copy-constructor. r=heycam
https://hg.mozilla.org/integration/autoland/rev/43cff735bb7c
Flush the overflow changed tracker when done with restyles. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ea75131f2054
more test expectation updates. r=heycam

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fcad8df0a68b
https://hg.mozilla.org/mozilla-central/rev/52fff0dd38c1
https://hg.mozilla.org/mozilla-central/rev/1d26488c7af2
https://hg.mozilla.org/mozilla-central/rev/ebeac2783f24
https://hg.mozilla.org/mozilla-central/rev/43cff735bb7c
https://hg.mozilla.org/mozilla-central/rev/ea75131f2054