Closed Bug 1378076 Opened 7 years ago Closed 7 years ago

stylo: make border-XX-style, cursor, -moz-user-select animatable

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(4 files, 6 obsolete files)

Bug 1378076 - Part 4: add tests for moz prefixed properties.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1378076 - Part 5: remove test fail annotations from meta in wpt.
59 bytes, text/x-review-board-request
hiro
: review+
Details
pseudo-cssanimation.html
533 bytes, text/html
Details
Servo PR 17797
41 bytes, text/x-github-pull-request
Details | Review
Support following properties animatable discretely. * border-top-style * border-right-style * border-bottom-style * border-left-style * content * cursor * list-style-type * -moz-appearance * -moz-user-select
Comment on attachment 8885250 [details] Bug 1378076 - Part 1: make border-XX-style animatable. https://reviewboard.mozilla.org/r/156114/#review161448 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:73 (Diff revision 1) > impl AnimatableLonghand { > /// Returns true if this AnimatableLonghand is one of the discretely animatable properties. > pub fn is_discrete(&self) -> bool { > match *self { > % for prop in data.longhands: > - % if prop.animation_value_type == "discrete": > + % if prop.animation_value_type == "discrete" and not prop.logical: I don't think this is a right thing to do. ::: servo/components/style/properties/longhand/border.mako.rs:32 (Diff revision 1) > ${helpers.predefined_type("border-%s-style" % side[0], "BorderStyle", > "specified::BorderStyle::none", > - need_clone=True, > alias=maybe_moz_logical_alias(product, side, "-moz-border-%s-style"), > spec=maybe_logical_spec(side, "style"), > - animation_value_type="none", logical=side[1])} > + animation_value_type="discrete", logical=side[1])} We should set discrete only if the property is not logical here?
Attachment #8885250 - Flags: review?(hikezoe)
Comment on attachment 8885254 [details] Bug 1378076 - Part 3: make -moz-user-select property animatable. https://reviewboard.mozilla.org/r/156122/#review161458
Attachment #8885254 - Flags: review?(hikezoe) → review+
Comment on attachment 8885256 [details] Bug 1378076 - Part 4: add tests for moz prefixed properties. https://reviewboard.mozilla.org/r/156126/#review161460
Attachment #8885256 - Flags: review?(hikezoe) → review+
Comment on attachment 8885257 [details] Bug 1378076 - Part 5: remove test fail annotations from meta in wpt. https://reviewboard.mozilla.org/r/156128/#review161462 ::: commit-message-93dd4:13 (Diff revision 1) > +However, we can not remove annotations for 'content' property since the test in > +wpt uses pseudo element for this property, also the animation for pseudo element > +does not work well yet. > +https://bugzilla.mozilla.org/show_bug.cgi?id=1358575 I think animation on pseudo elements has worked fine now, what is the problem exactly?
ni? ^
Flags: needinfo?(dakatsuka)
Comment on attachment 8885255 [details] Bug 1378076 - Part 6: make -moz-appearance property animatable. https://reviewboard.mozilla.org/r/156124/#review161464 ::: commit-message-7b439:7 (Diff revision 1) > +To realize this, we address for non_upper_case_globals rule of Rust since the > +prefix of Gecko filed is ThemeWidgetType_NS_THEME. > +Actually, we can think two ways to fix. > +1. Add #[arrow(non_upper_case_globals)] attribute. > +2. Force to change to upper case. > +In this time, we use 2 since don't want to use such the attribute for the > +coding style consistency even generated code. Any reasons why we can't use custom_consts?
Comment on attachment 8885252 [details] Bug 1378076 - Part 3: make list-style-type animatable. https://reviewboard.mozilla.org/r/156118/#review161466 I am confident this should be reviewed by Xidorn. ::: commit-message-10412:27 (Diff revision 1) > + target.animate({ listStyleType: ["none", "inherit"] }, > + { duration: 1000, delay: -500}); If this part is 'target.style.listStyleType = "inherit";', it works? Then I guess we should fix animation codes.
Attachment #8885252 - Flags: review?(hikezoe)
Comment on attachment 8885251 [details] Bug 1378076 - Part 2: make cursor animatable. https://reviewboard.mozilla.org/r/156116/#review161472 ::: servo/components/style/properties/gecko.mako.rs:4629 (Diff revision 1) > + let hotspot = > + if gecko_cursor_image.mHaveHotspot { > + Some((gecko_cursor_image.mHotspotX, gecko_cursor_image.mHotspotY)) > + } else { > + None > + }; set_cursor() does not touch mHaveHotspot at all, it means that we need to fix the setter?
Attachment #8885251 - Flags: review?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > Comment on attachment 8885257 [details] > Bug 1378076 - Part 8: remove test fail annotations from meta in wpt. > > https://reviewboard.mozilla.org/r/156128/#review161462 > > ::: commit-message-93dd4:13 > (Diff revision 1) > > +However, we can not remove annotations for 'content' property since the test in > > +wpt uses pseudo element for this property, also the animation for pseudo element > > +does not work well yet. > > +https://bugzilla.mozilla.org/show_bug.cgi?id=1358575 > > I think animation on pseudo elements has worked fine now, what is the > problem exactly? Thank you for your reviewing, Hiro. I attached attachment 8885543 [details] as a test code. The document has two animated elements, one is normal div, another one is pseudo. Although the normal div can animate, pseudo does not work well. (Looks only 'opacity' property works)
Flags: needinfo?(dakatsuka)
Comment on attachment 8885252 [details] Bug 1378076 - Part 3: make list-style-type animatable. https://reviewboard.mozilla.org/r/156118/#review161466 > If this part is 'target.style.listStyleType = "inherit";', it works? Then I guess we should fix animation codes. Yes, this works without my changes. So, we should fix in animation related place(e.g. Servo_GetComputedKeyframeValues or nsComputedDOMStyle::GetStyleContext)??
Comment on attachment 8885252 [details] Bug 1378076 - Part 3: make list-style-type animatable. https://reviewboard.mozilla.org/r/156118/#review161466 > Yes, this works without my changes. > So, we should fix in animation related place(e.g. Servo_GetComputedKeyframeValues or nsComputedDOMStyle::GetStyleContext)?? inherit in keyframes is handled here [1]. https://hg.mozilla.org/mozilla-central/file/1b065ffd8a53/servo/components/style/properties/helpers/animated_properties.mako.rs#l565 You can check the value there. If the value is stale value, you will need to check that the parent style is surely restyled when we call Element.animate().
Comment on attachment 8885252 [details] Bug 1378076 - Part 3: make list-style-type animatable. https://reviewboard.mozilla.org/r/156118/#review161466 > inherit in keyframes is handled here [1]. > https://hg.mozilla.org/mozilla-central/file/1b065ffd8a53/servo/components/style/properties/helpers/animated_properties.mako.rs#l565 > > You can check the value there. If the value is stale value, you will need to check that the parent style is surely restyled when we call Element.animate(). Thank you very much! I'll check there.
Comment on attachment 8885255 [details] Bug 1378076 - Part 6: make -moz-appearance property animatable. https://reviewboard.mozilla.org/r/156124/#review161464 > Any reasons why we can't use custom_consts? I think, we can't use custom_consts since this value is not for prefix. For example, if gecko_constant_prefix is "ThemeWidgetType_NS_THEME" and custom_consts is {"menulist": "CUSTOM_CONST"}, gecko_constant in data.py[1] returns "ThemeWidgetType_NS_THEME_CUSTOM_CONST". Likewise, casted_constant_name returns "U8_ThemeWidgetType_NS_THEME_CUSTOM_CONST" (if cast_type is u8). In the code where implements clone_XX for single keyword, we use casted_constant_name to define the constant variables[2], and use gecko_constant to map the value. So, this is a problem. The constant variables need to avoid non_upper_case_globals like U8_THEME_WIDGET_TYPE_NS_THEME_MENULIS, but mapped value should be struct::ThemeWidgetType_NS_THEME_MENULIST[3]. Even if we drop gecko_constant_prefix then define custom_consts for all values, we have same problem. [1] https://searchfox.org/mozilla-central/source/servo/components/style/properties/data.py#114 [2] https://searchfox.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs#389 [3] https://searchfox.org/mozilla-central/source/servo/components/style/gecko/generated/structs_debug.rs#38638
Summary: stylo: make border-XX-style, content, cursor, list-style-type, -moz-appearance, -moz-user-select animatable → stylo: make border-XX-style, cursor, -moz-user-select animatable
Attachment #8885252 - Attachment is obsolete: true
Attachment #8885253 - Attachment is obsolete: true
Attachment #8885253 - Flags: review?(hikezoe)
Attachment #8885255 - Attachment is obsolete: true
Attachment #8885255 - Flags: review?(hikezoe)
Comment on attachment 8885257 [details] Bug 1378076 - Part 5: remove test fail annotations from meta in wpt. https://reviewboard.mozilla.org/r/156128/#review164074
Attachment #8885257 - Flags: review?(hikezoe) → review+
Comment on attachment 8885251 [details] Bug 1378076 - Part 2: make cursor animatable. https://reviewboard.mozilla.org/r/156116/#review164078 I am wondering we have no test cases that passed by the setter change?
Attachment #8885251 - Flags: review?(hikezoe) → review+
Comment on attachment 8885250 [details] Bug 1378076 - Part 1: make border-XX-style animatable. https://reviewboard.mozilla.org/r/156114/#review164086 ::: servo/components/style/properties/longhand/border.mako.rs:32 (Diff revision 2) > ${helpers.predefined_type("border-%s-style" % side[0], "BorderStyle", > "specified::BorderStyle::none", > - need_clone=True, > alias=maybe_moz_logical_alias(product, side, "-moz-border-%s-style"), > spec=maybe_logical_spec(side, "style"), > - animation_value_type="none", logical=side[1])} > + animation_value_type="discrete" if not side[1] else "none", Should we define 'logical' name variable instead of using side[1] directly?
Attachment #8885250 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29) > Comment on attachment 8885251 [details] > Bug 1378076 - Part 2: make cursor animatable. > > https://reviewboard.mozilla.org/r/156116/#review164078 > > I am wondering we have no test cases that passed by the setter change? Yeah, although I threw to try-server, no error occurred. Also although looked tests over, could not find such the test. However, we can make in bug 1371151.
No, I am not saying about animation, just about normal style.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #37) > No, I am not saying about animation, just about normal style. Yeah, but bug 1371151 can test both set_XX and clone_XX since this test checks valid inherit value. So, we can set cursor style with hotspot to parent element (as normal style) , then can check either the value is set correctly. Or should we add a test in another wpt which tests normal style?
I understand what you mean, but it's just code coverage. If the setter had still some problems, it should be caught by a test case without animation. Imagine that we made some regressions for the property, if there is no test cases without animations, we can't know the reason immediately. I don't blame you at all, I am just wondering and surprised we have no test cases for it.
Attachment #8885250 - Attachment is obsolete: true
Attachment #8885251 - Attachment is obsolete: true
Attachment #8885254 - Attachment is obsolete: true
Pushed by dakatsuka@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/badb0a0b6d99 Part 4: add tests for moz prefixed properties. r=hiro https://hg.mozilla.org/integration/autoland/rev/84a5ef89a71f Part 5: remove test fail annotations from meta in wpt. r=hiro
Attached file Servo PR 17797 —
https://hg.mozilla.org/mozilla-central/rev/badb0a0b6d99 https://hg.mozilla.org/mozilla-central/rev/84a5ef89a71f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: