1407228 - Collapsed sections should be animated when they are reopened

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: reopening collapsed sections.gif

[Affected versions]:
- Firefox 58.0a1 Build ID 20171009220104

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Steps to reproduce]:
1. Start the browser with the profile from prerequisites and open a New Tab.
2. Collapse the Top Sites section.
3. Reopen the section and observe the behavior.

[Expected results]:
- The Top Sites section is animated as it appears.

[Actual results]:
- The Top Sites section appears instantly.

[Notes]:
- Because we have an animation when collapsing sections, it really feels incomplete to not have a similar animation when reopening the sections.
- The issue is also reproducible for the "Recommended by Pocket" and "Highlights" sections.
- Attached a screen recording of the issue.

Comment 1

[Ed Lee :Mardak]

Fixed by https://github.com/mozilla/activity-stream/pull/3808 to be uplifted via bug 1415812.

Comment 2

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/b4d1db669c93

Comment 3

[Ciprian Muresan [:cmuresan], Ecosystem QA]

I have verified that the issue is no longer reproducible using the latest Nightly 59.0a1 (Build ID 20171119220126) on Windows 10 x64, Mac 10.12.6 and Arch Linux x64.

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Bug 1407228 - Collapsed sections should be animated when they are reopened

Comment 5

[Andrei Oprea [:andreio]]

Comment on attachment 8932524 [details] [diff] [review]
Bug 1407228 - Collapsed sections should be animated when they are reopened

>
># HG changeset patch
># User Andrei Oprea <[e-mail]>
># Date 1511553149 -3600
># Node ID 803697bb06a2d0f97ce700a6c8743252e8f4c4a5
># Parent  bff979e2ef09d34ff2dd042dca9d2fca559eb4fd
>Bug 1407228 - Collapsed sections should be animated when they are reopened. r=r1cky
>
>MozReview-Commit-ID: 8TDAMSWrmKD
>
>
>diff --git a/browser/extensions/activity-stream/css/activity-stream-linux.css b/browser/extensions/activity-stream/css/activity-stream-linux.css
>--- a/browser/extensions/activity-stream/css/activity-stream-linux.css
>+++ b/browser/extensions/activity-stream/css/activity-stream-linux.css
>@@ -578,18 +578,17 @@ section.top-sites:not(.collapsed):hover 
>       offset-inline-start: auto;
>       offset-inline-end: 0; } }
> 
> .sections-list .section-empty-state {
>   width: 100%;
>   height: 266px;
>   display: flex;
>   border: 1px solid #D7D7DB;
>-  border-radius: 3px;
>-  margin-bottom: 16px; }
>+  border-radius: 3px; }
>   .sections-list .section-empty-state .empty-state {
>     margin: auto;
>     max-width: 350px; }
>     .sections-list .section-empty-state .empty-state .empty-state-icon {
>       background-size: 50px 50px;
>       background-repeat: no-repeat;
>       background-position: center;
>       fill: rgba(12, 12, 13, 0.6);
>@@ -1246,21 +1245,21 @@ section.top-sites:not(.collapsed):hover 
>     @media (min-width: 224px) {
>       .collapsible-section .section-disclaimer button {
>         position: relative; } }
>     @media (min-width: 416px) {
>       .collapsible-section .section-disclaimer button {
>         position: absolute; } }
> 
> .collapsible-section .section-body {
>-  max-height: 1100px;
>   margin: 0 -7px;
>   padding: 0 7px; }
>   .collapsible-section .section-body.animating {
>-    overflow: hidden; }
>+    overflow: hidden;
>+    pointer-events: none; }
> 
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-down,
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-forward {
>   transition: transform 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
> .collapsible-section.animation-enabled .section-body {
>   transition: max-height 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
>diff --git a/browser/extensions/activity-stream/css/activity-stream-mac.css b/browser/extensions/activity-stream/css/activity-stream-mac.css
>--- a/browser/extensions/activity-stream/css/activity-stream-mac.css
>+++ b/browser/extensions/activity-stream/css/activity-stream-mac.css
>@@ -578,18 +578,17 @@ section.top-sites:not(.collapsed):hover 
>       offset-inline-start: auto;
>       offset-inline-end: 0; } }
> 
> .sections-list .section-empty-state {
>   width: 100%;
>   height: 266px;
>   display: flex;
>   border: 1px solid #D7D7DB;
>-  border-radius: 3px;
>-  margin-bottom: 16px; }
>+  border-radius: 3px; }
>   .sections-list .section-empty-state .empty-state {
>     margin: auto;
>     max-width: 350px; }
>     .sections-list .section-empty-state .empty-state .empty-state-icon {
>       background-size: 50px 50px;
>       background-repeat: no-repeat;
>       background-position: center;
>       fill: rgba(12, 12, 13, 0.6);
>@@ -1246,21 +1245,21 @@ section.top-sites:not(.collapsed):hover 
>     @media (min-width: 224px) {
>       .collapsible-section .section-disclaimer button {
>         position: relative; } }
>     @media (min-width: 416px) {
>       .collapsible-section .section-disclaimer button {
>         position: absolute; } }
> 
> .collapsible-section .section-body {
>-  max-height: 1100px;
>   margin: 0 -7px;
>   padding: 0 7px; }
>   .collapsible-section .section-body.animating {
>-    overflow: hidden; }
>+    overflow: hidden;
>+    pointer-events: none; }
> 
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-down,
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-forward {
>   transition: transform 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
> .collapsible-section.animation-enabled .section-body {
>   transition: max-height 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
>diff --git a/browser/extensions/activity-stream/css/activity-stream-windows.css b/browser/extensions/activity-stream/css/activity-stream-windows.css
>--- a/browser/extensions/activity-stream/css/activity-stream-windows.css
>+++ b/browser/extensions/activity-stream/css/activity-stream-windows.css
>@@ -578,18 +578,17 @@ section.top-sites:not(.collapsed):hover 
>       offset-inline-start: auto;
>       offset-inline-end: 0; } }
> 
> .sections-list .section-empty-state {
>   width: 100%;
>   height: 266px;
>   display: flex;
>   border: 1px solid #D7D7DB;
>-  border-radius: 3px;
>-  margin-bottom: 16px; }
>+  border-radius: 3px; }
>   .sections-list .section-empty-state .empty-state {
>     margin: auto;
>     max-width: 350px; }
>     .sections-list .section-empty-state .empty-state .empty-state-icon {
>       background-size: 50px 50px;
>       background-repeat: no-repeat;
>       background-position: center;
>       fill: rgba(12, 12, 13, 0.6);
>@@ -1246,21 +1245,21 @@ section.top-sites:not(.collapsed):hover 
>     @media (min-width: 224px) {
>       .collapsible-section .section-disclaimer button {
>         position: relative; } }
>     @media (min-width: 416px) {
>       .collapsible-section .section-disclaimer button {
>         position: absolute; } }
> 
> .collapsible-section .section-body {
>-  max-height: 1100px;
>   margin: 0 -7px;
>   padding: 0 7px; }
>   .collapsible-section .section-body.animating {
>-    overflow: hidden; }
>+    overflow: hidden;
>+    pointer-events: none; }
> 
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-down,
> .collapsible-section.animation-enabled .section-title .icon-arrowhead-forward {
>   transition: transform 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
> .collapsible-section.animation-enabled .section-body {
>   transition: max-height 0.5s cubic-bezier(0.07, 0.95, 0, 1); }
> 
>diff --git a/browser/extensions/activity-stream/data/content/activity-stream.bundle.js b/browser/extensions/activity-stream/data/content/activity-stream.bundle.js
>--- a/browser/extensions/activity-stream/data/content/activity-stream.bundle.js
>+++ b/browser/extensions/activity-stream/data/content/activity-stream.bundle.js
>@@ -981,16 +981,19 @@ const VISIBILITY_CHANGE_EVENT = "visibil
> 
> function getFormattedMessage(message) {
>   return typeof message === "string" ? React.createElement(
>     "span",
>     null,
>     message
>   ) : React.createElement(FormattedMessage, message);
> }
>+function getCollapsed(props) {
>+  return props.Prefs.values[props.prefName];
>+}
> 
> class Info extends React.PureComponent {
>   constructor(props) {
>     super(props);
>     this.onInfoEnter = this.onInfoEnter.bind(this);
>     this.onInfoLeave = this.onInfoLeave.bind(this);
>     this.onManageClick = this.onManageClick.bind(this);
>     this.state = { infoActive: false };
>@@ -1108,27 +1111,38 @@ class Disclaimer extends React.PureCompo
>   }
> }
> 
> const DisclaimerIntl = injectIntl(Disclaimer);
> 
> class CollapsibleSection extends React.PureComponent {
>   constructor(props) {
>     super(props);
>+    this.onBodyMount = this.onBodyMount.bind(this);
>     this.onInfoEnter = this.onInfoEnter.bind(this);
>     this.onInfoLeave = this.onInfoLeave.bind(this);
>     this.onHeaderClick = this.onHeaderClick.bind(this);
>     this.onTransitionEnd = this.onTransitionEnd.bind(this);
>     this.enableOrDisableAnimation = this.enableOrDisableAnimation.bind(this);
>     this.state = { enableAnimation: true, isAnimating: false, infoActive: false };
>   }
> 
>   componentWillMount() {
>     this.props.document.addEventListener(VISIBILITY_CHANGE_EVENT, this.enableOrDisableAnimation);
>   }
>+  componentWillUpdate(nextProps) {
>+    // Check if we're about to go from expanded to collapsed
>+    if (!getCollapsed(this.props) && getCollapsed(nextProps)) {
>+      // This next line forces a layout flush of the section body, which has a
>+      // max-height style set, so that the upcoming collapse animation can
>+      // animate from that height to the collapsed height. Without this, the
>+      // update is coalesced and there's no animation from no-max-height to 0.
>+      this.sectionBody.scrollHeight; // eslint-disable-line no-unused-expressions
>+    }
>+  }
>   componentWillUnmount() {
>     this.props.document.removeEventListener(VISIBILITY_CHANGE_EVENT, this.enableOrDisableAnimation);
>   }
>   enableOrDisableAnimation() {
>     // Only animate the collapse/expand for visible tabs.
>     const visible = this.props.document.visibilityState === VISIBLE;
>     if (this.state.enableAnimation !== visible) {
>       this.setState({ enableAnimation: visible });
>@@ -1136,43 +1150,53 @@ class CollapsibleSection extends React.P
>   }
>   _setInfoState(nextActive) {
>     // Take a truthy value to conditionally change the infoActive state.
>     const infoActive = !!nextActive;
>     if (infoActive !== this.state.infoActive) {
>       this.setState({ infoActive });
>     }
>   }
>+  onBodyMount(node) {
>+    this.sectionBody = node;
>+  }
>   onInfoEnter() {
>     // We're getting focus or hover, so info state should be true if not yet.
>     this._setInfoState(true);
>   }
>   onInfoLeave(event) {
>     // We currently have an active (true) info state, so keep it true only if we
>     // have a related event target that is contained "within" the current target
>     // (section-info-option) as itself or a descendant. Set to false otherwise.
>     this._setInfoState(event && event.relatedTarget && (event.relatedTarget === event.currentTarget || event.relatedTarget.compareDocumentPosition(event.currentTarget) & Node.DOCUMENT_POSITION_CONTAINS));
>   }
>   onHeaderClick() {
>-    this.setState({ isAnimating: true });
>-    this.props.dispatch(ac.SetPref(this.props.prefName, !this.props.Prefs.values[this.props.prefName]));
>+    // Get the current height of the body so max-height transitions can work
>+    this.setState({
>+      isAnimating: true,
>+      maxHeight: `${this.sectionBody.scrollHeight}px`
>+    });
>+    this.props.dispatch(ac.SetPref(this.props.prefName, !getCollapsed(this.props)));
>   }
>-  onTransitionEnd() {
>-    this.setState({ isAnimating: false });
>+  onTransitionEnd(event) {
>+    // Only update the animating state for our own transition (not a child's)
>+    if (event.target === event.currentTarget) {
>+      this.setState({ isAnimating: false });
>+    }
>   }
>   renderIcon() {
>     const icon = this.props.icon;
>     if (icon && icon.startsWith("moz-extension://")) {
>       return React.createElement("span", { className: "icon icon-small-spacer", style: { "background-image": `url('${icon}')` } });
>     }
>     return React.createElement("span", { className: `icon icon-small-spacer icon-${icon || "webextension"}` });
>   }
>   render() {
>-    const isCollapsed = this.props.Prefs.values[this.props.prefName];
>-    const { enableAnimation, isAnimating } = this.state;
>+    const isCollapsed = getCollapsed(this.props);
>+    const { enableAnimation, isAnimating, maxHeight } = this.state;
>     const { id, infoOption, eventSource, disclaimer } = this.props;
>     const disclaimerPref = `section.${id}.showDisclaimer`;
>     const needsDisclaimer = disclaimer && this.props.Prefs.values[disclaimerPref];
> 
>     return React.createElement(
>       "section",
>       { className: `collapsible-section ${this.props.className}${enableAnimation ? " animation-enabled" : ""}${isCollapsed ? " collapsed" : ""}` },
>       React.createElement(
>@@ -1188,17 +1212,21 @@ class CollapsibleSection extends React.P
>             this.props.title,
>             React.createElement("span", { className: `icon ${isCollapsed ? "icon-arrowhead-forward" : "icon-arrowhead-down"}` })
>           )
>         ),
>         infoOption && React.createElement(InfoIntl, { infoOption: infoOption, dispatch: this.props.dispatch })
>       ),
>       React.createElement(
>         "div",
>-        { className: `section-body${isAnimating ? " animating" : ""}`, onTransitionEnd: this.onTransitionEnd },
>+        {
>+          className: `section-body${isAnimating ? " animating" : ""}`,
>+          onTransitionEnd: this.onTransitionEnd,
>+          ref: this.onBodyMount,
>+          style: isAnimating && !isCollapsed ? { maxHeight } : null },
>         needsDisclaimer && React.createElement(DisclaimerIntl, { disclaimerPref: disclaimerPref, disclaimer: disclaimer, eventSource: eventSource, dispatch: this.props.dispatch }),
>         this.props.children
>       )
>     );
>   }
> }
> 
> CollapsibleSection.defaultProps = {
>diff --git a/browser/extensions/activity-stream/install.rdf.in b/browser/extensions/activity-stream/install.rdf.in
>--- a/browser/extensions/activity-stream/install.rdf.in
>+++ b/browser/extensions/activity-stream/install.rdf.in
>@@ -3,17 +3,17 @@
> #filter substitution
> 
> <RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>   <Description about="urn:mozilla:install-manifest">
>     <em:id>[e-mail]</em:id>
>     <em:type>2</em:type>
>     <em:bootstrap>true</em:bootstrap>
>     <em:unpack>false</em:unpack>
>-    <em:version>2017.11.03.1438-bf427ddd</em:version>
>+    <em:version>2017.11.24.1191-83e61a5e</em:version>
>     <em:name>Activity Stream</em:name>
>     <em:description>A rich visual history feed and a reimagined home page make it easier than ever to find exactly what you're looking for in Firefox.</em:description>
>     <em:multiprocessCompatible>true</em:multiprocessCompatible>
> 
>     <em:targetApplication>
>       <Description>
>         <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
>         <em:minVersion>@MOZ_APP_VERSION@</em:minVersion>
>

Comment 6

[Andrei Oprea [:andreio]]

Attachment: Bug 1407228 - Collapsed sections should be animated when they are reopened. r=r1cky

Comment 7

[Ed Lee :Mardak]

gchang says this bug has missed 58.0b8. It doesn't have a patch with beta approval flag. Please attach the appropriate patch without install.rdf.in changes and request approval ASAP.

Comment 8

[Andrei Oprea [:andreio]]

Attachment: Bug 1407228 - Collapsed sections should be animated when they are reopened.

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

Comment 9

[Andrei Oprea [:andreio]]

Comment on attachment 8933365 [details]
Bug 1407228 - Collapsed sections should be animated when they are reopened.

Approval Request Comment
[Feature/Bug causing the regression]: https://github.com/mozilla/activity-stream/pull/3617
[User impact if declined]: impacts user experience
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: caret icon on the activity stream page causes the sections to expand/collapse. the animation should always be smooth
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: it has been tested/it is in nightly
[String changes made/needed]: no

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8933365 [details]
Bug 1407228 - Collapsed sections should be animated when they are reopened.

Fix the caret icon on the activity stream page issue. Beta58+.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch against mozilla-beta tip.

Comment 12

[Ed Lee :Mardak]

Attachment: rebased export for mozilla-beta

Fortunately this is the last one, so we can update the install.rdf.in :)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/699b253725ef