Closed Bug 1356072 Opened 3 years ago Closed 3 years ago

stylo: Support moz-prefixed cursor values

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: kuoe0.tw)

References

Details

Attachments

(1 file, 1 obsolete file)

There are several moz-prefixed values for cursor property we may need to support.

It seems cursor is a bit different that the keyword definition is in style_traits rather than style, and I'm not completely sure what's the best way here.
Blocks: 1356087
Priority: -- → P2
I'm intent to try this.
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED
Attachment #8863659 - Flags: review?(xidorn+moz)
Attachment #8863660 - Flags: review?(xidorn+moz)
Comment on attachment 8863659 [details]
Bug 1356072 - Make stylo support moz-prefixed cursor values.

https://reviewboard.mozilla.org/r/135454/#review138426

::: servo/components/style_traits/cursor.rs:80
(Diff revision 1)
>      "zoom-in" => ZoomIn = 33,
>      "zoom-out" => ZoomOut = 34,
> +    "-moz-grab" => MozGrab = 35,
> +    "-moz-grabbing" => MozGrabbing = 36,
> +    "-moz-zoom-in" => MozZoomIn = 37,
> +    "-moz-zoom-out" => MozZoomOut = 38,

I have tried to make these lines be gecko-specific. But `[cfg(feature=gecko)]` seems not work with macro.
Comment on attachment 8863659 [details]
Bug 1356072 - Make stylo support moz-prefixed cursor values.

https://reviewboard.mozilla.org/r/135454/#review138426

> I have tried to make these lines be gecko-specific. But `[cfg(feature=gecko)]` seems not work with macro.

You can add `$(#[$attr:meta])*` in the macro to support this.
Comment on attachment 8863660 [details]
Bug 1356072 - Update stylo-failures.

https://reviewboard.mozilla.org/r/135456/#review138434
Attachment #8863660 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8863659 [details]
Bug 1356072 - Make stylo support moz-prefixed cursor values.

https://reviewboard.mozilla.org/r/135454/#review138712

Please try making the new values Gecko-only.
Attachment #8863659 - Flags: review?(xidorn+moz)
Hi Xidorn, I tried to add `#[$attr: meta])*` to the macro (see the latest patch), but I always got the following compilation error:

```
 0:06.52    Compiling style_traits v0.0.1 (file:///Users/kuoe0/Works/Mozilla/gecko-dev/servo/components/style_traits)
 0:06.90 error: local ambiguity: multiple parsing options: built-in NTs expr ('css') or 1 other option.
 0:06.90   --> /Users/kuoe0/Works/Mozilla/gecko-dev/servo/components/style_traits/cursor.rs:79:5
 0:06.90    |
 0:06.90 79 |     #[cfg(feature = "gecko")]
 0:06.90    |     ^
 0:06.90
 0:06.94 error: Could not compile `style_traits`.
```

Do you know what the correct syntax is to make it work?
Flags: needinfo?(xidorn+moz)
Comment on attachment 8863659 [details]
Bug 1356072 - Make stylo support moz-prefixed cursor values.

https://reviewboard.mozilla.org/r/135454/#review139134

This looks good to me. Probably worth double-check with Manish or emilio whether they think this is fine.

::: servo/components/style/properties/gecko.mako.rs:3835
(Diff revision 3)
> +                #[cfg(feature = "gecko")]
> +                Cursor::MozGrab => structs::NS_STYLE_CURSOR_GRAB,
> +                #[cfg(feature = "gecko")]
> +                Cursor::MozGrabbing => structs::NS_STYLE_CURSOR_GRABBING,
> +                #[cfg(feature = "gecko")]
> +                Cursor::MozZoomIn => structs::NS_STYLE_CURSOR_ZOOM_IN,
> +                #[cfg(feature = "gecko")]
> +                Cursor::MozZoomOut => structs::NS_STYLE_CURSOR_ZOOM_OUT,

Do you need `cfg(feature = "gecko")` in `gecko.mako.rs`?
Attachment #8863659 - Flags: review?(xidorn+moz) → review+
emilio, are you fine with the approach in the patch?
Flags: needinfo?(xidorn+moz) → needinfo?(emilio+bugs)
Looks fine to me. I've seen this pattern used elsewhere too, but I forget where...
Flags: needinfo?(emilio+bugs)
Comment on attachment 8863659 [details]
Bug 1356072 - Make stylo support moz-prefixed cursor values.

https://reviewboard.mozilla.org/r/135454/#review139134

> Do you need `cfg(feature = "gecko")` in `gecko.mako.rs`?

No, we don't need it. I think gecko.mako.rs is already gecko only.
Attachment #8863659 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0bb4e2e0a488
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.