Closed Bug 1453258 Opened 4 years ago Closed 4 years ago

calc should be supported in cursor

Categories

(Core :: CSS Parsing and Computation, defect)

59 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ericwilligers, Assigned: xidorn)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

Set cursor inline style to
'url("https://example.com/") 1 calc(2 + 0), copy'

Read cursor inline style and computed style
Example page: https://jsfiddle.net/ericwilligers/ueeb1ene/ 


Actual results:

Inline style is empty string. Computed style is 'auto'


Expected results:

'url("https://example.com/") 1 2, copy'
for both inline style and computed style.
Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID:20180416100103

Managed to reproduce the issue on Ubuntu 16.04 x64 and Windows 10 x64 using the latest Nightly 61.0a1 (2018-04-16).
Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Product: Firefox → Core
(In reply to Eric Willigers from comment #0)
> Expected results:
> 
> 'url("https://example.com/") 1 2, copy'
> for both inline style and computed style.

This is not the expected result though, per spec we should preserve calc() in the specified value, so should be calc(1) calc(2) in the specified style.
Comment on attachment 8968475 [details]
Bug 1453258 - Support calc in cursor.

https://reviewboard.mozilla.org/r/237178/#review242982

Nice cleanup :-).

Are there any tests for this? If so, r=me.

::: servo/components/style/values/generics/pointing.rs:27
(Diff revision 1)
> +///
> +/// https://drafts.csswg.org/css-ui/#cursor
> +#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue)]
> +pub struct Cursor<Image> {
> +    /// The parsed images for the cursor.
> +    pub images: Box<[Image]>,

Doesn't `#[css(iterable, comma)]` allow you to derive this?

::: servo/components/style/values/generics/pointing.rs:71
(Diff revision 1)
> +    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
> +    where
> +        W: Write,
> +    {
> +        self.url.to_css(dest)?;
> +        if let Some((ref x, ref y)) = self.hotspot {

Doesn't this do the same that the derived implementation would do? If so, just derive it.

::: servo/components/style/values/specified/pointing.rs:36
(Diff revision 1)
> +}
> +
> +/// A specified value for the `cursor` property.
> +pub type Cursor = generics::Cursor<CursorImage>;
> +
> +/// A specified value for the `image cursors`.

This comment doesn't make much sense, mind rewording it?

::: servo/components/style/values/specified/pointing.rs:74
(Diff revision 1)
> +        })
> +    }
> +}
> +
> +impl CursorImage {
> +    fn parse_image<'i, 't>(

Just `impl Parse for CursorImage`?
Attachment #8968475 - Flags: review?(emilio) → review+
Comment on attachment 8968475 [details]
Bug 1453258 - Support calc in cursor.

https://reviewboard.mozilla.org/r/237178/#review242982

> Doesn't `#[css(iterable, comma)]` allow you to derive this?

Not really, because you need a comma between the last image and the keyword. I suppose using `ToCss` would generate a whitespace there.

> Doesn't this do the same that the derived implementation would do? If so, just derive it.

the trait `style_traits::ToCss` is not implemented for `(Number, Number)`.
https://hg.mozilla.org/mozilla-central/rev/b00ed74f557b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → xidorn+moz
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.