Closed
Bug 1370034
Opened 7 years ago
Closed 7 years ago
Implement new CSS properties -moz-window-opacity, -moz-window-transform and -moz-window-transform-origin on Mac for efficient panel animations
Categories
(Core :: CSS Parsing and Computation, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
spohl
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
spohl
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
spohl
:
review+
|
Details |
Our panel animations currently apply an opacity and a transform to the content that is painted inside of the panel. This has bad effects on the window shadow on Mac: The system computes the shadow's shape based on the pixels that are drawn into the window, so we need to ask it to recompute the shadow on every animation frame.
It would be better if we could instead change the opacity and the transform of the window itself. Those would apply to the whole window including the shadow, and they would apply *outside* the pixels inside the window, so the window's "shape" would stay fixed during the animation.
macOS has a public API to change a window's opacity: -[NSWindow setOpacity:].
It also has a private, but widely-used API to change a window's transform: CGSSetWindowTransform.
Instead of re-purposing the existing CSS properties "transform" and "opacity" when used on panels, I'd prefer to add new CSS properties. That's going to make the layout part of this change a lot simpler, because we won't have to add tons of special cases.
I'm calling these properties -moz-window-opacity, -moz-window-transform, and -moz-window-transform-origin. They can be animated using CSS transitions.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
I tried to make these properties CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME but that broke CSS transitions on them. Is that a known issue?
These patches don't implement the new properties for stylo. I'd appreciate help with that.
Flags: needinfo?(xidorn+moz)
Comment 8•7 years ago
|
||
I wasn't aware of that issue before, and I'm not familiar with transition stuff so not sure what's happening there. Redirect the question to birtles.
Flags: needinfo?(xidorn+moz) → needinfo?(bbirtles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
If it's not known, then I may have made a mistake. I still have some test failures on these patches. Once they are fixed, I'll try CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME again.
Flags: needinfo?(bbirtles)
Comment 19•7 years ago
|
||
That may simply be because we never tried animating internal-only properties before. This mechanism was added not very long ago, so I wouldn't be surprised if it is broken in some cases.
Comment 20•7 years ago
|
||
Yeah, there are a bunch of places where we pass CSSEnabledState::eForAllContent unconditionally:
http://searchfox.org/mozilla-central/search?q=CSSEnabledState%3A%3AeForAllContent&case=false®exp=false&path=dom%2Fanimation
Like Xidorn said, this is a fairly new mechanism and we've probably just never tried animating internal-only properties before.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
I'd also note that this patch doesn't do what this comment says:
https://searchfox.org/mozilla-central/source/layout/style/nsCSSProps.h#265-268
i.e., wrapping in CSS_PROP_LIST_EXCLUDE_INTERNAL. I'm honestly a little rusty on the distinctions between these, although I suspect that you may want to use CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME instead (and make animations on it work).
Flags: needinfo?(mstange)
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #28)
> I'd also note that this patch doesn't do what this comment says:
> https://searchfox.org/mozilla-central/source/layout/style/nsCSSProps.h#265-
> 268
> i.e., wrapping in CSS_PROP_LIST_EXCLUDE_INTERNAL.
It does; it puts the properties into the #ifdef CSS_PROP_LIST_EXCLUDE_INTERNAL block that's already there for -moz-window-shadow.
Flags: needinfo?(mstange)
Also, I think using CSS_PROPERTY_INTERNAL flag without the ifdefs doesn't actually prevent exposing the property to the Web. We should probably have assertions verifying that we don't have any properties set up that way.
OK, if it's actually properly internal -- then how does the test you added to property_database.js pass?
Flags: needinfo?(mstange)
Comment 32•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #30)
> Also, I think using CSS_PROPERTY_INTERNAL flag without the ifdefs doesn't
> actually prevent exposing the property to the Web. We should probably have
> assertions verifying that we don't have any properties set up that way.
We have such check: https://dxr.mozilla.org/mozilla-central/rev/a49112c7a5765802096b3fc298069b9495436107/layout/style/nsCSSProps.cpp#33-46
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #31)
> OK, if it's actually properly internal -- then how does the test you added
> to property_database.js pass?
That "test" is inside an "if (false)" block and doesn't actually run. Sorry, I just followed what was done in the context in that file. I can remove the test additions if that would be less misleading.
Flags: needinfo?(mstange)
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #32)
> (In reply to David Baron :dbaron: ⌚️UTC-7 from comment #30)
> > Also, I think using CSS_PROPERTY_INTERNAL flag without the ifdefs doesn't
> > actually prevent exposing the property to the Web. We should probably have
> > assertions verifying that we don't have any properties set up that way.
>
> We have such check:
> https://dxr.mozilla.org/mozilla-central/rev/
> a49112c7a5765802096b3fc298069b9495436107/layout/style/nsCSSProps.cpp#33-46
Yes, and I can confirm that it's working, because I actually ran into that check at some point during development.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #20)
> Yeah, there are a bunch of places where we pass
> CSSEnabledState::eForAllContent unconditionally:
>
> http://searchfox.org/mozilla-central/search?q=CSSEnabledState%3A%3AeForAllContent&case=false®exp=false&path=dom%2Fanimation
Experimentally, I replaced eForAllContent with eIgnoreEnabledState in a few places. In addition to the ones in KeyframeUtils.cpp, I also had to change these two:
http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/style/nsRuleNode.cpp#5614
http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/style/nsTransitionManager.cpp#862
After that, the transitions started working.
If we want to fix this, I'll need some guidance on where to obtain the right enabled state for the various cases. Alternatively, maybe somebody else who already knows the code could fix this?
Or we could go ahead with the current patches and use only CSS_PROPERTY_INTERNAL and not CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME.
Comment 49•7 years ago
|
||
mozreview-review |
Comment on attachment 8874168 [details]
Bug 1370034 - Add nsIWidget::SetWindowOpacity and implement it on Mac.
https://reviewboard.mozilla.org/r/145588/#review152484
::: widget/cocoa/nsCocoaWindow.mm:2172
(Diff revision 4)
> +void
> +nsCocoaWindow::SetWindowOpacity(float aOpacity)
> +{
> + NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> + if (!mWindow)
nit: Use {}
Attachment #8874168 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 50•7 years ago
|
||
mozreview-review |
Comment on attachment 8874171 [details]
Bug 1370034 - Add nsIWidget::SetWindowTransform and implement it on Mac.
https://reviewboard.mozilla.org/r/145594/#review152492
::: widget/cocoa/nsCocoaWindow.mm:2182
(Diff revision 7)
>
> NS_OBJC_END_TRY_ABORT_BLOCK;
> }
>
> +static inline CGAffineTransform
> +GfxMatrixToCGAffineTransform(const gfx::Matrix &m)
nit: & after gfx::Matrix instead of before m
::: widget/cocoa/nsCocoaWindow.mm:2199
(Diff revision 7)
> +void
> +nsCocoaWindow::SetWindowTransform(const gfx::Matrix& aTransform)
> +{
> + NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> + if (!mWindow)
nit: Use {}
Attachment #8874171 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 51•7 years ago
|
||
mozreview-review |
Comment on attachment 8875935 [details]
Bug 1370034 - Allow disabling window transforms using a pref.
https://reviewboard.mozilla.org/r/147334/#review152498
Attachment #8875935 -
Flags: review?(spohl.mozilla.bugs) → review+
Assignee | ||
Comment 52•7 years ago
|
||
Emilio told me that the stylo changes don't need to block the initial landing of this bug. We also don't seem to be using stylo for chrome properties yet.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 60•7 years ago
|
||
mozreview-review |
Comment on attachment 8874167 [details]
Bug 1370034 - Add -moz-window-opacity property, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145586/#review154598
r=dbaron if you've done the following and tests pass:
::: layout/style/test/property_database.js:7908
(Diff revision 5)
> + gCSSProperties["-moz-window-opacity"] = {
> + // domProp: "MozWindowOpacity",
> + inherited: false,
> + type: CSS_TYPE_LONGHAND,
> + initial_values: [ "1", "17", "397.376", "3e1", "3e+1", "3e0", "3e+0", "3e-0" ],
> + other_values: [ "0", "0.4", "0.0000", "-3", "3e-1" ],
> + invalid_values: [ "0px", "1px", "20%", "default", "auto" ]
> + };
> +
Please do a try run with this *outside* the if-false and with the property not marked as internal in nsCSSPropList.h, and make sure all the mochitests in layout/style/test/ pass in that configuration.
Attachment #8874167 -
Flags: review?(dbaron) → review+
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8874169 [details]
Bug 1370034 - Implement UpdateWidgetProperties for top level windows and for popups, and call nsIWidget::SetWindowShadow.
https://reviewboard.mozilla.org/r/145590/#review154600
::: layout/generic/nsFrame.cpp:10481
(Diff revision 5)
> nsIFrame::IsScrolledOutOfView()
> {
> return IsFrameScrolledOutOfView(this);
> }
>
> +static nsIWidget*
This should return already_AddRefed<nsIWidget>, and the last line should be "return mainWidget.forget()". Otherwise you're dropping the reference you acquired and addrefing it again in the caller -- which is both extra refcount traffic and dangerous.
::: layout/generic/nsFrame.cpp:10481
(Diff revision 5)
> +static nsIWidget*
> +GetWindowWidget(nsPresContext* aPresContext)
> +{
> + nsCOMPtr<nsISupports> container = aPresContext->Document()->GetContainer();
> + nsCOMPtr<nsIBaseWindow> baseWindow = do_QueryInterface(container);
> + if (!baseWindow)
> + return nullptr;
> +
> + nsCOMPtr<nsIWidget> mainWidget;
> + baseWindow->GetMainWidget(getter_AddRefs(mainWidget));
> + return mainWidget;
> +}
Please add comments explaining why you need this function rather than one of the existing functions nsPresContext::GetRootWidget or nsPresContext::GetNearestWidget or nsIFrame::GetNearestWidget, or use one of them. (My assumption is it's because you want the *top* widget.)
That said, would it be simpler to use aPresContext->GetRootWidget()->GetTopLevelWidget()?
::: layout/generic/nsFrame.cpp:10498
(Diff revision 5)
> +
> +void
> +nsIFrame::UpdateWidgetProperties()
> +{
> + nsPresContext* presContext = PresContext();
> + if (presContext->GetParentPresContext() || !presContext->IsChrome()) {
Please use nsPresContext::IsRoot() rather than the somewhat-expensive nsPresContext::GetParentPresContext().
::: layout/generic/nsFrame.cpp:10503
(Diff revision 5)
> + if (presContext->GetParentPresContext() || !presContext->IsChrome()) {
> + // Don't do anything for documents that aren't the root chrome document.
> + return;
> + }
> + nsIFrame* rootFrame =
> + presContext->PresShell()->FrameConstructor()->GetRootElementStyleFrame();
please use presContext->FrameConstructor() rather than presContext->PresShell()->FrameConstructor()
Attachment #8874169 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 62•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874167 [details]
Bug 1370034 - Add -moz-window-opacity property, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145586/#review154598
> Please do a try run with this *outside* the if-false and with the property not marked as internal in nsCSSPropList.h, and make sure all the mochitests in layout/style/test/ pass in that configuration.
I've started a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4b01a8541ee03a0a801a2de46e633fb76a97dd
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 68•7 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #62)
> Comment on attachment 8874167 [details]
> Bug 1370034 - Add -moz-window-opacity property, style system parts
> (excluding stylo).
>
> https://reviewboard.mozilla.org/r/145586/#review154598
>
> > Please do a try run with this *outside* the if-false and with the property not marked as internal in nsCSSPropList.h, and make sure all the mochitests in layout/style/test/ pass in that configuration.
>
> I've started a try push at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4b01a8541ee03a0a801a2de46e633fb76a97dd
Whoops, forgot to re-enable the "domProp: ..." lines. Trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82412b33ae6da5bc5b495179535de80756772739
Comment 69•7 years ago
|
||
mozreview-review |
Comment on attachment 8874170 [details]
Bug 1370034 - Add CSS properties -moz-window-transform and -moz-window-transform-origin, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145592/#review154708
::: devtools/shared/css/generated/properties-db.js:1518
(Diff revision 7)
> + "-moz-window-transform": {
> + "isInherited": false,
I'm presuming you generated these changes with "./mach devtools-css-db". If not, you should have done it that way. I'm not reviewing them for substance. (Same for previous patch.)
::: layout/style/nsCSSParser.cpp:16239
(Diff revision 7)
> -CSSParserImpl::ParseTransform(bool aIsPrefixed, bool aDisallowRelativeValues)
> +CSSParserImpl::ParseTransform(bool aIsPrefixed, nsCSSPropertyID aProperty,
> + bool aDisallowRelativeValues)
Do you intend to accept 3-D transform functions here? If so, what are you doing with them?
::: layout/style/nsCSSPropList.h:4507
(Diff revision 7)
> "",
> VARIANT_HN,
> nullptr,
> offsetof(nsStyleUIReset, mWindowOpacity),
> eStyleAnimType_float)
> +CSS_PROP_UIRESET(
You should do the same testing drill for these changes -- run mochitests with them not marked as internal.
::: layout/style/nsRuleNode.cpp:1745
(Diff revision 7)
> NS_NOTREACHED("SetFactor: inappropriate unit");
> }
>
> +static void
> +SetTransformValue(const nsCSSValue& aValue, RefPtr<nsCSSValueSharedList>& aField,
> + RuleNodeCacheConditions& aConditions, nsCSSValueSharedList* aParentValue)
Please make aParentValue nsCSSValueSharedList* const, and then make sure the whole thing wraps at less than 80 lines.
::: layout/style/nsStyleStruct.cpp:4373
(Diff revision 7)
> nsStyleUIReset::CalcDifference(const nsStyleUIReset& aNewData) const
> {
> // ignore mIMEMode
> if (mForceBrokenImageIcon != aNewData.mForceBrokenImageIcon) {
> return nsChangeHint_ReconstructFrame;
> }
> if (mWindowShadow != aNewData.mWindowShadow) {
> // We really need just an nsChangeHint_SyncFrameView, except
> // on an ancestor of the frame, so we get that by doing a
> // reflow.
> return NS_STYLE_HINT_REFLOW;
> }
> if (mUserSelect != aNewData.mUserSelect) {
> return NS_STYLE_HINT_VISUAL;
> }
>
> if (mWindowDragging != aNewData.mWindowDragging) {
> return nsChangeHint_SchedulePaint;
> }
>
> if (mWindowOpacity != aNewData.mWindowOpacity) {
> return nsChangeHint_UpdateWidgetProperties;
> }
>
> + if (!mSpecifiedWindowTransform != !aNewData.mSpecifiedWindowTransform ||
> + (mSpecifiedWindowTransform &&
> + *mSpecifiedWindowTransform != *aNewData.mSpecifiedWindowTransform)) {
> + return nsChangeHint_UpdateWidgetProperties;
> + }
> +
> + for (uint8_t index = 0; index < 2; ++index) {
> + if (mWindowTransformOrigin[index] != aNewData.mWindowTransformOrigin[index]) {
> + return nsChangeHint_UpdateWidgetProperties;
> + }
> + }
> +
> if (mIMEMode != aNewData.mIMEMode) {
> return nsChangeHint_NeutralChange;
This method needs restructuring. I should have caught this in the previous patch. You need to do the right thing when multiple properties change, which this doesn't do. You need to be able to send UpdateWidgetProperties hints along with others.
(Or is UpdateWidgetProperties somehow implied by SchedulePaint?)
Attachment #8874170 -
Flags: review?(dbaron) → review+
Comment 70•7 years ago
|
||
mozreview-review |
Comment on attachment 8874172 [details]
Bug 1370034 - Call SetWindowTransform with the right values from -moz-window-transform(-origin).
https://reviewboard.mozilla.org/r/145596/#review154716
::: layout/generic/nsFrame.cpp:10505
(Diff revision 9)
> + // Compute the -moz-window-transform-origin value in device pixels.
> + float transformOrigin[2];
> + nsStyleTransformMatrix::TransformReferenceBox::DimensionGetter dimensionGetter[] =
> + { &nsStyleTransformMatrix::TransformReferenceBox::Width,
> + &nsStyleTransformMatrix::TransformReferenceBox::Height };
> + for (uint8_t index = 0; index < 2; ++index) {
> + const nsStyleCoord& originValue = uiReset->mWindowTransformOrigin[index];
> + if (originValue.GetUnit() == eStyleUnit_Calc) {
> + const nsStyleCoord::Calc *calc = originValue.GetCalcValue();
> + transformOrigin[index] =
> + NSAppUnitsToFloatPixels((refBox.*dimensionGetter[index])(), appUnitsPerDevPixel) *
> + calc->mPercent +
> + NSAppUnitsToFloatPixels(calc->mLength, appUnitsPerDevPixel);
> + } else if (originValue.GetUnit() == eStyleUnit_Percent) {
> + transformOrigin[index] =
> + NSAppUnitsToFloatPixels((refBox.*dimensionGetter[index])(), appUnitsPerDevPixel) *
> + originValue.GetPercentValue();
> + } else {
> + MOZ_ASSERT(originValue.GetUnit() == eStyleUnit_Coord,
> + "unexpected unit");
> + transformOrigin[index] =
> + NSAppUnitsToFloatPixels(originValue.GetCoordValue(),
> + appUnitsPerDevPixel);
> + }
> + }
Please use a function to share this code with nsDisplayTransform::ComputePerspectiveMatrix
::: layout/generic/nsFrame.cpp:10534
(Diff revision 9)
> + gfx::Matrix result2d;
> + if (!matrix.CanDraw2D(&result2d)) {
> + return gfx::Matrix();
> + }
Seems less than ideal to just drop it on the floor if it has 3-D bits in it. I guess it's chrome-only, though. But maybe at least add a FIXME comment saying that it would have been better to reject 3-D components at parse time? And perhaps also an NS_WARNING or a console message?
::: layout/generic/nsIFrame.h:3886
(Diff revision 9)
> + * Computes a 2D matrix from the -moz-window-transform and
> + * -moz-window-transform-origin properties on aFrame.
> + * Values that don't result in a 2D matrix will be ignored and an identity
> + * matrix will be returned instead.
> + */
> + Matrix GetWidgetTransform();
I'd call this ComputeWidgetTransform rather than GetWidgetTransform. Calling it Get makes it sound like (a) it's fast and (b) it's getting something from the widget.
Attachment #8874172 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 71•7 years ago
|
||
Thanks for the reviews!
Assignee | ||
Comment 72•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874170 [details]
Bug 1370034 - Add CSS properties -moz-window-transform and -moz-window-transform-origin, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145592/#review154708
> I'm presuming you generated these changes with "./mach devtools-css-db". If not, you should have done it that way. I'm not reviewing them for substance. (Same for previous patch.)
Yes I did.
> Do you intend to accept 3-D transform functions here? If so, what are you doing with them?
I'm accepting them and, very late in the pipeline, discard the whole transform (treat it as identity) if it can't render in 2D.
> You should do the same testing drill for these changes -- run mochitests with them not marked as internal.
Done (in the same try push).
> This method needs restructuring. I should have caught this in the previous patch. You need to do the right thing when multiple properties change, which this doesn't do. You need to be able to send UpdateWidgetProperties hints along with others.
>
> (Or is UpdateWidgetProperties somehow implied by SchedulePaint?)
No, UpdateWidgetProperties is not implied by SchedulePaint. What I did here is wrong, will fix.
Assignee | ||
Comment 73•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874172 [details]
Bug 1370034 - Call SetWindowTransform with the right values from -moz-window-transform(-origin).
https://reviewboard.mozilla.org/r/145596/#review154716
> Please use a function to share this code with nsDisplayTransform::ComputePerspectiveMatrix
I think the reason I didn't share it was because I'm treating the z component of the transform-origin differently. But that difference may have been misguided. I'll make another attempt to share it.
> Seems less than ideal to just drop it on the floor if it has 3-D bits in it. I guess it's chrome-only, though. But maybe at least add a FIXME comment saying that it would have been better to reject 3-D components at parse time? And perhaps also an NS_WARNING or a console message?
I can add an NS_WARNING (which doesn't need to be localized) and the FIXME. I don't think it's worth spending time fixing it, though; the fact that these properties are chrome-only and can't even be used by WebExtensions makes these imperfections really insignificant.
> I'd call this ComputeWidgetTransform rather than GetWidgetTransform. Calling it Get makes it sound like (a) it's fast and (b) it's getting something from the widget.
Good call.
Assignee | ||
Comment 74•7 years ago
|
||
mozreview-review |
Comment on attachment 8874167 [details]
Bug 1370034 - Add -moz-window-opacity property, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145586/#review154730
::: layout/style/test/property_database.js:7908
(Diff revision 5)
> + gCSSProperties["-moz-window-opacity"] = {
> + // domProp: "MozWindowOpacity",
> + inherited: false,
> + type: CSS_TYPE_LONGHAND,
> + initial_values: [ "1", "17", "397.376", "3e1", "3e+1", "3e0", "3e+0", "3e-0" ],
> + other_values: [ "0", "0.4", "0.0000", "-3", "3e-1" ],
> + invalid_values: [ "0px", "1px", "20%", "default", "auto" ]
> + };
> +
There are a few test failures involving transitions on -moz-window-opacity. I'll look into them.
Assignee | ||
Comment 75•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874167 [details]
Bug 1370034 - Add -moz-window-opacity property, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145586/#review154598
> I've started a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd4b01a8541ee03a0a801a2de46e633fb76a97dd
Oh, the transition failures only happened because I hadn't adjusted test_transitions_per_property.html to expect these properties to be animatable. With that change, it passes.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 83•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874172 [details]
Bug 1370034 - Call SetWindowTransform with the right values from -moz-window-transform(-origin).
https://reviewboard.mozilla.org/r/145596/#review154716
> I think the reason I didn't share it was because I'm treating the z component of the transform-origin differently. But that difference may have been misguided. I'll make another attempt to share it.
I created nsStyleTransformMatrix::Convert2DPosition and am now calling it both from nsDisplayTransform::ComputePerspectiveMatrix and from nsFrame::ComputeWidgetTransform.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 85•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8874167 [details]
Bug 1370034 - Add -moz-window-opacity property, style system parts (excluding stylo).
https://reviewboard.mozilla.org/r/145586/#review154598
> Oh, the transition failures only happened because I hadn't adjusted test_transitions_per_property.html to expect these properties to be animatable. With that change, it passes.
Final try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b18c9c9a13a019db5cc25ef44acc371a6aebe184 . I had a few 3d coordinate values in the -moz-window-transform-origin test, but we only parse 2d values for the origin. Now that I've moved those 3d values to the invalid_values section, the tests finally pass completely.
Comment 86•7 years ago
|
||
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/c99450001fa1
Add -moz-window-opacity property, style system parts (excluding stylo). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/28e733821d73
Add nsIWidget::SetWindowOpacity and implement it on Mac. r=spohl
https://hg.mozilla.org/integration/autoland/rev/46c5f91672a9
Implement UpdateWidgetProperties for top level windows and for popups, and call nsIWidget::SetWindowShadow. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/aa5be78cd9fc
Add CSS properties -moz-window-transform and -moz-window-transform-origin, style system parts (excluding stylo). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/f84d7ac5105d
Add nsIWidget::SetWindowTransform and implement it on Mac. r=spohl
https://hg.mozilla.org/integration/autoland/rev/193433849675
Allow disabling window transforms using a pref. r=spohl
https://hg.mozilla.org/integration/autoland/rev/37c332642a18
Call SetWindowTransform with the right values from -moz-window-transform(-origin). r=dbaron
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 91•7 years ago
|
||
Backed out for test failures: https://hg.mozilla.org/integration/autoland/rev/0d08acc5a759
Comment 92•7 years ago
|
||
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/5fced871414b
Add -moz-window-opacity property, style system parts (excluding stylo). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/09f895ddacdc
Add nsIWidget::SetWindowOpacity and implement it on Mac. r=spohl
https://hg.mozilla.org/integration/autoland/rev/2cf330925da7
Implement UpdateWidgetProperties for top level windows and for popups, and call nsIWidget::SetWindowShadow. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/1e5fe4feb659
Add CSS properties -moz-window-transform and -moz-window-transform-origin, style system parts (excluding stylo). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/d40fdf7c04eb
Add nsIWidget::SetWindowTransform and implement it on Mac. r=spohl
https://hg.mozilla.org/integration/autoland/rev/3c2d465f021e
Allow disabling window transforms using a pref. r=spohl
https://hg.mozilla.org/integration/autoland/rev/d19edbac8bc9
Call SetWindowTransform with the right values from -moz-window-transform(-origin). r=dbaron
Comment 93•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5fced871414b
https://hg.mozilla.org/mozilla-central/rev/09f895ddacdc
https://hg.mozilla.org/mozilla-central/rev/2cf330925da7
https://hg.mozilla.org/mozilla-central/rev/1e5fe4feb659
https://hg.mozilla.org/mozilla-central/rev/d40fdf7c04eb
https://hg.mozilla.org/mozilla-central/rev/3c2d465f021e
https://hg.mozilla.org/mozilla-central/rev/d19edbac8bc9
Status: ASSIGNED → 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.
Description
•