Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682603004:
Don't animate visibility keyframes that don't include "visible" (Closed)
|
Created:
3 years, 10 months ago by alancutter (OOO until 2018) Modified:
3 years, 10 months ago Reviewers:
Eric Willigers CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, rjwright, shans Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't animate visibility keyframes that don't include "visible"
This change updates the requirements for smoothly interpolating
visibility animations to include needing one side to be "visible".
This is to maintain existing behaviour once CSS Transitions is
refactored to use CSSInterpolationTypes in:
https://codereview.chromium.org/2680923005
Relevant spec text:
https://drafts.csswg.org/css-transitions/#animtype-visibility
BUG=681424
Review-Url: https://codereview.chromium.org/2682603004
Cr-Commit-Position: refs/heads/master@{#451548}
Committed: https://chromium.googlesource.com/chromium/src/+/cb8696135be40717770ba4422c140c79851d9f74
Patch Set 1 #Patch Set 2 : git squash commit for _fixVisibilityMerge. #
Total comments: 2
Patch Set 3 : Review changes #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== Ensure single conversion for CSSVisibilityInterpolationType can merge with itself BUG=681424 ========== to ========== Ensure single conversion for CSSVisibilityInterpolationType can merge with itself animations/interpolation/visibility-interpolation.html BUG=681424 ==========
Description was changed from ========== Ensure single conversion for CSSVisibilityInterpolationType can merge with itself animations/interpolation/visibility-interpolation.html BUG=681424 ========== to ========== Don't animate visibility keyframes that don't include "visible" This change updates the requirements for smoothly interpolating visibility animations to include needing one side to be "visible". This is to maintain existing behaviour once CSS Transitions is refactored to use CSSInterpolationTypes in: https://codereview.chromium.org/2680923005 BUG=681424 ==========
alancutter@chromium.org changed reviewers: + ericwilligers@chromium.org
On 2017/02/15 23:43:43, alancutter wrote: Perhaps link to MDN in change description https://developer.mozilla.org/en/docs/Web/CSS/visibility "... One of the start or ending values must therefore be visible or no interpolation can happen."
lgtm https://codereview.chromium.org/2682603004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp (right): https://codereview.chromium.org/2682603004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:33: if (m_start == EVisibility::kVisible || m_end == EVisibility::kVisible) Should this now be DCHECK(m_start == EVisibility::kVisible || m_end == EVisibility::kVisible) ?
Description was changed from ========== Don't animate visibility keyframes that don't include "visible" This change updates the requirements for smoothly interpolating visibility animations to include needing one side to be "visible". This is to maintain existing behaviour once CSS Transitions is refactored to use CSSInterpolationTypes in: https://codereview.chromium.org/2680923005 BUG=681424 ========== to ========== Don't animate visibility keyframes that don't include "visible" This change updates the requirements for smoothly interpolating visibility animations to include needing one side to be "visible". This is to maintain existing behaviour once CSS Transitions is refactored to use CSSInterpolationTypes in: https://codereview.chromium.org/2680923005 Relevant spec text: https://drafts.csswg.org/css-transitions/#animtype-visibility BUG=681424 ==========
Added spec link. https://codereview.chromium.org/2682603004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp (right): https://codereview.chromium.org/2682603004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:33: if (m_start == EVisibility::kVisible || m_end == EVisibility::kVisible) On 2017/02/16 at 00:29:47, Eric Willigers wrote: > Should this now be DCHECK(m_start == EVisibility::kVisible || m_end == EVisibility::kVisible) ? Good idea. Done.
The CQ bit was checked by alancutter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericwilligers@chromium.org Link to the patchset: https://codereview.chromium.org/2682603004/#ps40001 (title: "Review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487551823676140,
"parent_rev": "9cfb02e626c5e82aa1f2ac8528c50778719bd8c3", "commit_rev":
"cb8696135be40717770ba4422c140c79851d9f74"}
Message was sent while issue was closed.
Description was changed from ========== Don't animate visibility keyframes that don't include "visible" This change updates the requirements for smoothly interpolating visibility animations to include needing one side to be "visible". This is to maintain existing behaviour once CSS Transitions is refactored to use CSSInterpolationTypes in: https://codereview.chromium.org/2680923005 Relevant spec text: https://drafts.csswg.org/css-transitions/#animtype-visibility BUG=681424 ========== to ========== Don't animate visibility keyframes that don't include "visible" This change updates the requirements for smoothly interpolating visibility animations to include needing one side to be "visible". This is to maintain existing behaviour once CSS Transitions is refactored to use CSSInterpolationTypes in: https://codereview.chromium.org/2680923005 Relevant spec text: https://drafts.csswg.org/css-transitions/#animtype-visibility BUG=681424 Review-Url: https://codereview.chromium.org/2682603004 Cr-Commit-Position: refs/heads/master@{#451548} Committed: https://chromium.googlesource.com/chromium/src/+/cb8696135be40717770ba4422c14... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/cb8696135be40717770ba4422c14... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
