Closed Bug 1419695 Opened 7 years ago Closed 6 years ago

Make internal -moz-window-opacity and -moz-window-transform transitionable even when the properties are hidden from content pages.

Categories

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

Product:

Component:

Type:

enhancement

Priority:

P3

Severity:

normal

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files)

Bug 1419695: Make the transition-property code make more sense. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1419695: Allow transitioning chrome-only properties, as long as they're specified as a longhand. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1419695: Hide multiple -moz-window-* properties from content. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1419695: Move TransitionProperty where it belongs. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	xidorn : review+	Details
Bug 1419695: Remove some unused style struct setters. 6 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	xidorn : review+	Details

Emilio Cobos Álvarez (:emilio)

Assignee

Description

•

7 years ago

See bug 1419355.

This is preventing us from hiding them from content pages.

Jet Villegas (inactive)

Updated

•

7 years ago

Priority: -- → P3

Xidorn Quan [:xidorn] UTC+11

Comment 1

•

6 years ago

Bug 1452542 comment 16 indicates that this has been fixed by stylo, and I'm going to update the test in that patch per review comment. It seems everything works just fine, so closing this bug.

Status: NEW → RESOLVED

Closed: 6 years ago

Resolution: --- → WORKSFORME

Xidorn Quan [:xidorn] UTC+11

Updated

•

6 years ago

Status: RESOLVED → REOPENED

Resolution: WORKSFORME → ---

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

6 years ago

Assignee: nobody → emilio

Comment hidden (mozreview-request)

We were working around the lack of alias support during parsing in
TransitionProperty by doing a Gecko lookup. That's a hack and is now gone.

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 7

•

6 years ago

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

Kohei Yoshino [:kohei]

Updated

•

6 years ago

Keywords: site-compat

Xidorn Quan [:xidorn] UTC+11

Comment 8

•

6 years ago

mozreview-review

Comment on attachment 8982555 [details]
Bug 1419695: Remove some unused style struct setters.

https://reviewboard.mozilla.org/r/248542/#review254926

Attachment #8982555 - Flags: review?(xidorn+moz) → review+

Xidorn Quan [:xidorn] UTC+11

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8982551 [details]
Bug 1419695: Make the transition-property code make more sense.

https://reviewboard.mozilla.org/r/248534/#review254928

::: servo/components/style/properties/gecko.mako.rs:3278
(Diff revision 1)
> +                if !gecko.mUnknownProperty.mRawPtr.is_null() {
> +                    unsafe { Atom::from_addrefed(gecko.mUnknownProperty.mRawPtr) };
> +                    gecko.mUnknownProperty.mRawPtr = ptr::null_mut();
> +                }

`unsafe { gecko.mUnknownProperty.clear() }`?

::: servo/components/style/properties/gecko.mako.rs:3286
(Diff revision 1)
> +                }
> +
>                  match servo {
> -                    TransitionProperty::Unsupported(ref ident) => unsafe {
> -                        Gecko_StyleTransition_SetUnsupportedProperty(gecko, ident.0.as_ptr())
> -                    },
> +                    TransitionProperty::Unsupported(ident) => {
> +                        gecko.mProperty = eCSSProperty_UNKNOWN;
> +                        gecko.mUnknownProperty.mRawPtr = ident.0.into_addrefed();

We may want to have some nicer way to assign `Atom` to `RefPtr<nsAtom>` I guess...

::: servo/components/style/properties/gecko.mako.rs:3337
(Diff revision 1)
> +            //
> +            // FIXME(emilio): This is a hack, is this reachable?

Porbably yes because of the code in `set_transition_property` above? Maybe we want a different way to do that...

::: servo/components/style/properties/gecko.mako.rs:3359
(Diff revision 1)
> +            if !transition.mUnknownProperty.mRawPtr.is_null() {
> +                unsafe { Atom::from_addrefed(transition.mUnknownProperty.mRawPtr) };
> +                transition.mUnknownProperty.mRawPtr = ptr::null_mut();
> +            }

`unsafe { transition.mUnknownProperty.clear() }`

::: servo/components/style/properties/helpers/animated_properties.mako.rs:87
(Diff revision 1)
>      /// A shorthand.
>      Shorthand(ShorthandId),
>      /// A longhand transitionable property.
>      Longhand(LonghandId),
> +    /// A custom property.
> +    Custom(Atom),

Maybe using `custom_properties::Name` is better here.

Given that `custom_properties::Name` has a special semantics (name without `--` prefix), maybe we really should have that a separate type rather than an alias, so that we can also avoid impl `ToCss` here.

Attachment #8982551 - Flags: review?(xidorn+moz) → review+

Xidorn Quan [:xidorn] UTC+11

Comment 10

•

6 years ago

mozreview-review

Comment on attachment 8982552 [details]
Bug 1419695: Allow transitioning chrome-only properties, as long as they're specified as a longhand.

https://reviewboard.mozilla.org/r/248536/#review254930

Attachment #8982552 - Flags: review?(xidorn+moz) → review+

Xidorn Quan [:xidorn] UTC+11

Comment 11

•

6 years ago

mozreview-review

Comment on attachment 8982553 [details]
Bug 1419695: Hide multiple -moz-window-* properties from content.

https://reviewboard.mozilla.org/r/248538/#review254932

Attachment #8982553 - Flags: review?(xidorn+moz) → review+

Xidorn Quan [:xidorn] UTC+11

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8982554 [details]
Bug 1419695: Move TransitionProperty where it belongs.

https://reviewboard.mozilla.org/r/248540/#review254934

::: servo/components/style/values/specified/box.rs:798
(Diff revision 1)
> +        TransitionProperty::Shorthand(ShorthandId::All)
> +    }
> +
> +    /// Convert TransitionProperty to nsCSSPropertyID.
> +    #[cfg(feature = "gecko")]
> +    pub fn to_nscsspropertyid(&self) -> Result<::gecko_bindings::structs::nsCSSPropertyID, ()> {

Why not just include `gecko_bindings::structs::nsCSSPropertyID` in this file?

Attachment #8982554 - Flags: review?(xidorn+moz) → review+

Chris Mills [:cmills]

Comment 13

•

6 years ago

Adding dev-doc-needed, just in case

(for any writer that ends up investigating this one, also see the "Intent to Unship: -moz-window-opacity / -moz-window-transform / -moz-window-transform-origin CSS properties." thread on dev.platform)

Keywords: dev-doc-needed

Xidorn Quan [:xidorn] UTC+11

Comment 14

•

6 years ago

The thread is: https://groups.google.com/forum/#!topic/mozilla.dev.platform/ReGRVwuDnhk

(Maybe we should add the address of intent to ship/unship in every such bug somehow.)

Kohei Yoshino [:kohei]

Updated

•

6 years ago

Keywords: site-compat

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 15

•

6 years ago

mozreview-review-reply

Comment on attachment 8982551 [details]
Bug 1419695: Make the transition-property code make more sense.

https://reviewboard.mozilla.org/r/248534/#review254928

> `unsafe { gecko.mUnknownProperty.clear() }`?

We cannot call clear() because nsAtom doesn't implement `RefCounted`. I'll do in a followup while cleaning up the other css stuff.

> Porbably yes because of the code in `set_transition_property` above? Maybe we want a different way to do that...

Yeah...

> Maybe using `custom_properties::Name` is better here.
> 
> Given that `custom_properties::Name` has a special semantics (name without `--` prefix), maybe we really should have that a separate type rather than an alias, so that we can also avoid impl `ToCss` here.

Will do.

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 16

•

6 years ago

mozreview-review-reply

Comment on attachment 8982551 [details]
Bug 1419695: Make the transition-property code make more sense.

https://reviewboard.mozilla.org/r/248534/#review254928

> Will do.

(changing to `custom_properties::Name`). Then I can look into the different type.

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 17

•

6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> Comment on attachment 8982554 [details]
> Bug 1419695: Move TransitionProperty where it belongs.
> 
> https://reviewboard.mozilla.org/r/248540/#review254934
> 
> ::: servo/components/style/values/specified/box.rs:798
> (Diff revision 1)
> > +        TransitionProperty::Shorthand(ShorthandId::All)
> > +    }
> > +
> > +    /// Convert TransitionProperty to nsCSSPropertyID.
> > +    #[cfg(feature = "gecko")]
> > +    pub fn to_nscsspropertyid(&self) -> Result<::gecko_bindings::structs::nsCSSPropertyID, ()> {
> 
> Why not just include `gecko_bindings::structs::nsCSSPropertyID` in this file?

Because I'd need to cfg more... Not a big deal in this case I guess...

Comment 18

•

6 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca50a8ddc432
Make the transition-property code make more sense. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea66c21b87ae
Allow transitioning chrome-only properties, as long as they're specified as a longhand. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6305fd6e16a
Hide multiple -moz-window-* properties from content. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e3e002cbea
Move TransitionProperty where it belongs. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4949db937f1
Remove some unused style struct setters. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc034b4c57e
Use custom_properties::Name in TransitionProperty. r=xidorn

Comment 19

•

6 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e84d69431ec
followup: Update the properties database for devtools. r=me on a CLOSED TREE

Bogdan Tara[:bogdan_tara | bogdant]

Comment 20

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/ca50a8ddc432
https://hg.mozilla.org/mozilla-central/rev/ea66c21b87ae
https://hg.mozilla.org/mozilla-central/rev/d6305fd6e16a
https://hg.mozilla.org/mozilla-central/rev/34e3e002cbea
https://hg.mozilla.org/mozilla-central/rev/c4949db937f1
https://hg.mozilla.org/mozilla-central/rev/abc034b4c57e
https://hg.mozilla.org/mozilla-central/rev/0e84d69431ec

Status: REOPENED → RESOLVED

Closed: 6 years ago → 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla62

Will Bamberg [:wbamberg]

Comment 21

•

6 years ago

We don't think there are any web doc updates needed for this, so I'm removing dev-doc-needed.

Keywords: dev-doc-needed

You need to log in before you can comment on or make changes to this bug.