Closed Bug 1350140 Opened 6 years ago Closed 6 years ago

stylo: Implement all the pending state pseudo-classes.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(6 files)

Bug 1350140: stylo: Implement all the remaining state pseudo-classes.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1350140: Update test expectations.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1350140: Fix computation of border and outline-width when the border or outline style change.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1350140: Avoid dumb copy-constructor.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1350140: Flush the overflow changed tracker when done with restyles.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1350140: more test expectation updates.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
I felt like doing it today.
emilio++
Priority: -- → P1
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 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?
Attachment #8850768 - Flags: review?(cam) → review+
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.
Blocks: 1324647
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.
Attachment #8851760 - Flags: review?(cam) → review+
Comment on attachment 8851761 [details]
Bug 1350140: Avoid dumb copy-constructor.

https://reviewboard.mozilla.org/r/123988/#review126598
Attachment #8851761 - Flags: review?(cam) → review+
Comment on attachment 8851762 [details]
Bug 1350140: Flush the overflow changed tracker when done with restyles.

https://reviewboard.mozilla.org/r/123990/#review126600
Attachment #8851762 - Flags: review?(cam) → review+
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.
Attachment #8851760 - Flags: review+ → review?(cam)
Blocks: 1335990
Blocks: 1335314
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.
Attachment #8851760 - Flags: review?(cam) → review+
Comment on attachment 8851988 [details]
Bug 1350140: more test expectation updates.

https://reviewboard.mozilla.org/r/124246/#review127120
Attachment #8851988 - Flags: review?(cam) → review+
Comment on attachment 8851759 [details]
Bug 1350140: Update test expectations.

https://reviewboard.mozilla.org/r/123984/#review127214
Attachment #8851759 - Flags: review?(emilio+bugs) → review+
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.
Depends on: 1351724
Pushed by ecoal95@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.