Closed Bug 1355924 Opened 7 years ago Closed 7 years ago

The Reload/Stop button should have an animation when changing between states

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Version:
55 Branch
Type:
defect
Points:
8

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Flags:
Tracking Status
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-animation])

Attachments

(1 file, 3 obsolete files)

Bug 1355924 - The refresh/stop button should have an animation when changing between states.
59 bytes, text/x-review-board-request
dao
: review+
Details 
Spec TBD
Flags: qe-verify+
Depends on: 1355925
Priority: -- → P2
QA Contact: jwilliams
User Story: (updated)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
User Story: (updated)
Attached file Loading Indicator.zip (obsolete) — 
Updated refresh/x svg sprites.
Flags: needinfo?(jaws)
Attached file Refresh Icons.zip (obsolete) — 
Updated the refresh/x svg sprites.  I've included both 32x32 and 36x36 versions.
Attachment #8866857 - Attachment is obsolete: true
Iteration: 55.5 - May 15 → 55.6 - May 29
Flags: needinfo?(jaws)
Attachment #8868308 - Flags: feedback?(sfoster)
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

I got conflicts with the toolbarbutton-icon.inc.css changes, so I didn't have a quick way of previewing this. But I think I get the gist and it looks reasonable to me. A couple notes: 

* The .svg file looks pretty bloaty. You can try running it through svgo (npm install -g svgo) but it may need to go back to Eric/Amy to simplify? 

* Does the addition of the isTopLevel condition need to be behind the MOZ_PHOTON_ANIMATIONS flag if it changes existing behavior?
Attachment #8868308 - Flags: feedback?(sfoster) → feedback+
Attachment #8868308 - Flags: review?(sfoster)
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review147124

::: commit-message-d715e:4
(Diff revision 3)
> +Bug 1355924 - The refresh/stop button should have an animation when changing between states.
> +
> +TODO:
> +- Disable the animation when switching tabs

Looks like these are still TODO, do you want to file follow-up bug(s) for them?

::: browser/themes/shared/icons/reload-to-stop.svg:1
(Diff revision 3)
> +<svg xmlns="http://www.w3.org/2000/svg" width="648" height="18">

I still have no idea why this is almost 80KB for what appears to be a fairly simple animation of a couple simple shapes. But it needn't hold up landing. We can update the assets as we refine our process
Attachment #8868308 - Flags: review?(sfoster) → review+
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review147338

::: browser/base/content/browser.js:4521
(Diff revision 3)
>        }
>  
>        this.isBusy = true;
>  
> -      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING)) {
> +      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> +          (!AppConstants.MOZ_PHOTON_ANIMATIONS || aWebProgress.isTopLevel)) {

Please explain what you're doing here. As I understand it, this will prevent users from stopping page loads in iframes, which seems pretty problematic.

::: browser/base/content/browser.xul:776
(Diff revision 3)
>              <toolbarbutton id="reload-button"
>                             class="toolbarbutton-1"
>                             command="Browser:ReloadOrDuplicate"
>                             onclick="checkForMiddleClick(this, event);"
> -                           tooltip="dynamic-shortcut-tooltip"/>
> +                           tooltip="dynamic-shortcut-tooltip">
> +              <xul:box class="toolbarbutton-animatable-box">

nit: drop "xul:"

::: browser/themes/shared/icons/reload-to-stop.svg:2
(Diff revision 3)
> +<svg xmlns="http://www.w3.org/2000/svg" width="648" height="18">
> +  <svg xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 36 36" preserveAspectRatio="xMidYMid meet" style="width:100%;height:100%" x="0" y="0">

nit: drop xmlns attribute (it's already on the root node)

What's the purpose of style="width:100%;height:100%"?

::: browser/themes/shared/icons/reload-to-stop.svg:24
(Diff revision 3)
> +        <path stroke-linecap="round" stroke-linejoin="miter" fill-opacity="0" stroke-miterlimit="10" stroke="context-fill" stroke-opacity="1" stroke-width="2.57144" d="M10.727 10.727l14.546 14.546" opacity="1" style="-moz-user-select:none"/>
> +      </g>
> +      <g mask="url(#e)">
> +        <path stroke-linecap="round" stroke-linejoin="miter" fill-opacity="0" stroke-miterlimit="10" stroke="context-fill" stroke-opacity="1" stroke-width="2.57144" d="M25.273 10.727L10.727 25.273" opacity="1" style="-moz-user-select:none"/>
> +      </g>
> +      <path fill="#0005FF" fill-opacity="1" d="M-3.273 2v11.076h-10.852V2h10.852z" opacity="1" style="-moz-user-select:none"/>

What's the purpose of style="-moz-user-select:none"?

::: browser/themes/shared/toolbarbutton-icons.inc.css:66
(Diff revision 3)
> +}
> +
> +#reload-button > .toolbarbutton-icon,
> +#stop-button > .toolbarbutton-icon {
> +  width: 32px;
> +  height: 28px;

Where do these numbers come from? Note that our button dimensions depend on CSS variables and are handled in toolbarbuttons.inc.css. toolbarbutton-icons.inc.css isn't really supposed to mess with this.

::: browser/themes/shared/toolbarbutton-icons.inc.css:84
(Diff revision 3)
> +#stop-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  height: 26px;
> +  animation-fill-mode: forwards;
> +  animation-iteration-count: 1;
> +  -moz-context-properties: fill;
> +  fill: rgb(77,77,77);

This is the wrong color e.g. for dark lightweight theme. Please drop -moz-context-properties and fill here. Colors are already take care of by generic rules.

::: browser/themes/shared/toolbarbutton-icons.inc.css:121
(Diff revision 3)
>    list-style-image: url("chrome://browser/skin/stop.svg");
>  }
>  %endif
> +#forward-button:-moz-locale-dir(rtl) > .toolbarbutton-icon {
> +  transform: scaleX(-1);
> +}

This looks like merging cruft, please remove.
Iteration: 55.6 - May 29 → 55.7 - Jun 12
:Dao, I've got some SVGO filters working at https://github.com/sfoster/svgo/tree/photon-plugins that will clean up some of the SVG cruft. We could file a follow-up bug to clean/optimize all the SVG animation assets? Or, Jared or I can run each through this tool before re-submitting the patch. I'll add a filter for the redundant xmlns attribute. I think I have the other points covered already or in-progress.
Flags: needinfo?(dao+bmo)
Afaik you haven't landed any of the assets yet, so if you could clean them up before landing them, that would seem preferable to me.
Flags: needinfo?(dao+bmo)
Depends on: 1368653
Comment on attachment 8867298 [details]
Refresh Icons.zip

Moved updated UX specs and assets to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1369868
Attachment #8867298 - Attachment is obsolete: true
Depends on: 1369868
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review151312

::: browser/base/content/browser.js:4546
(Diff revision 5)
>  
> -      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING)) {
> +      // The stop/reload buttons only change due to top-level page
> +      // loads to prevent the icons from rapidly switching states due
> +      // to various subresources loading on a page.
> +      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> +          (!AppConstants.MOZ_PHOTON_ANIMATIONS || (aWebProgress.isTopLevel && aWebProgress.isLoadingDocument))) {

So this comment explains the change, but I still don't think it's okay to not have the stop button available for iframes.

::: browser/base/content/tabbrowser.xml:1128
(Diff revision 5)
>              this.showTab(this.mCurrentTab);
>  
>              var forwardButtonContainer = document.getElementById("urlbar-wrapper");
> -            if (forwardButtonContainer) {
> -              forwardButtonContainer.setAttribute("switchingtabs", "true");
> +            var reloadButton = document.getElementById("reload-button");
> +            if (forwardButtonContainer || stopReloadButton) {
> +              forwardButtonContainer && forwardButtonContainer.setAttribute("switchingtabs", "true");

forwardButtonContainer cannot be null

::: browser/base/content/tabbrowser.xml:1129
(Diff revision 5)
>  
>              var forwardButtonContainer = document.getElementById("urlbar-wrapper");
> -            if (forwardButtonContainer) {
> -              forwardButtonContainer.setAttribute("switchingtabs", "true");
> +            var reloadButton = document.getElementById("reload-button");
> +            if (forwardButtonContainer || stopReloadButton) {
> +              forwardButtonContainer && forwardButtonContainer.setAttribute("switchingtabs", "true");
> +              reloadButton && reloadButton.setAttribute("switchingtabs", "true");

please use if ()

::: browser/themes/shared/icons/reload-to-stop.svg:8
(Diff revision 5)
> +    <defs>
> +      <clipPath id="b">
> +        <path d="M0 0h18v20H0z"/>
> +      </clipPath>
> +      <mask id="e" mask-type="alpha">
> +        <path fill="context-fill" fill-opacity="1" d="M0-2.906A2.907 2.907 0 0 1 2.906 0 2.907 2.907 0 0 1 0 2.906 2.907 2.907 0 0 1-2.906 0 2.907 2.907 0 0 1 0-2.906z" transform="matrix(7.36622 0 0 7.36622 1.27 18.622)" opacity="1"/>

Can you just put fill="context-fill" on the root svg node?

::: browser/themes/shared/icons/reload-to-stop.svg:8
(Diff revision 5)
> +    <defs>
> +      <clipPath id="b">
> +        <path d="M0 0h18v20H0z"/>
> +      </clipPath>
> +      <mask id="e" mask-type="alpha">
> +        <path fill="context-fill" fill-opacity="1" d="M0-2.906A2.907 2.907 0 0 1 2.906 0 2.907 2.907 0 0 1 0 2.906 2.907 2.907 0 0 1-2.906 0 2.907 2.907 0 0 1 0-2.906z" transform="matrix(7.36622 0 0 7.36622 1.27 18.622)" opacity="1"/>

Aren't fill-opacity="1" and opacity="1" no-ops here?

::: browser/themes/shared/toolbarbutton-icons.inc.css:92
(Diff revision 5)
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 9px);
> +  margin-inline-start: 4px;
> +  width: 18px;

where does this number come from?

::: browser/themes/shared/toolbarbutton-icons.inc.css:97
(Diff revision 5)
> +  width: 18px;
> +}
> +
> +#reload-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image,
> +#stop-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  height: 20px;

ditto

::: browser/themes/shared/toolbarbutton-icons.inc.css:134
(Diff revision 5)
> +#reload-button[displaystop] + #stop-button:-moz-locale-dir(rtl) > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  animation-name: stop-to-reload-rtl;
> +}
> +
> +/* XXXjaws "switchingtabs" is to prevent the animation from running when switching from a tab that is loading to one that is loaded, and vice versa
> +           but I'm still seeing the animation occur. Any help here would be appreciated :) */

Bug 1338881?
Attachment #8868308 - Flags: review?(dao+bmo)
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
The icon now properly changes without animation when switching tabs. Bug 1338881 doesn't seem relevant here anymore.

(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8868308 [details]
> Bug 1355924 - The refresh/stop button should have an animation when changing
> between states.
> 
> https://reviewboard.mozilla.org/r/139898/#review151312
> 
> ::: browser/base/content/browser.js:4546
> (Diff revision 5)
> >  
> > -      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING)) {
> > +      // The stop/reload buttons only change due to top-level page
> > +      // loads to prevent the icons from rapidly switching states due
> > +      // to various subresources loading on a page.
> > +      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> > +          (!AppConstants.MOZ_PHOTON_ANIMATIONS || (aWebProgress.isTopLevel && aWebProgress.isLoadingDocument))) {
> 
> So this comment explains the change, but I still don't think it's okay to
> not have the stop button available for iframes.

I haven't removed this part of the patch, and I'm not sure users understand the difference between stopping an iframe vs other subresources.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> The icon now properly changes without animation when switching tabs. Bug
> 1338881 doesn't seem relevant here anymore.

What did you change to solve this?

> > So this comment explains the change, but I still don't think it's okay to
> > not have the stop button available for iframes.
> 
> I haven't removed this part of the patch, and I'm not sure users understand
> the difference between stopping an iframe vs other subresources.

I didn't say there was a difference. The stop button should probably be available in both cases. Iframes are just more obviously problematic, because they can load huge pages whereas the top frame may just be a thin container.
(In reply to Dão Gottwald [::dao] from comment #17)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > The icon now properly changes without animation when switching tabs. Bug
> > 1338881 doesn't seem relevant here anymore.
> 
> What did you change to solve this?

The previous approach always used the animation, and then shortened the duration of the animation down to 0s during when not triggered via network request.

The new approach now only uses the animation when the change is triggered by a network request, otherwise it will always just translate to the last frame of the animation.
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review154418

::: browser/base/content/browser.js:4546
(Diff revision 6)
>  
> -      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING)) {
> +      // The stop/reload buttons only change due to top-level page
> +      // loads to prevent the icons from rapidly switching states due
> +      // to various subresources loading on a page.
> +      if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING) &&
> +          (!AppConstants.MOZ_PHOTON_ANIMATIONS || (aWebProgress.isTopLevel && aWebProgress.isLoadingDocument))) {

Not sure if you're waiting for some other information from me, but this still seems wrong to me.

Perhaps we should skip the animation in those cases?

::: browser/base/content/browser.js:4939
(Diff revision 6)
>    },
>  
>    handleEvent(event) {
> -    // the only event we listen to is "click" on the stop button
> -    if (event.button == 0 &&
> +    if (event.type == "click" &&
> +               event.button == 0 &&
> -        !this.stop.disabled)
> +               !this.stop.disabled)

indentation is messed up

::: browser/base/content/browser.js:4957
(Diff revision 6)
>        return;
>  
>      this._cancelTransition();
> +    if (aDelay && this.animate) {
> +      this.reload.setAttribute("animate", "true");
> +      this.reload.parentNode.addEventListener("animationend", event => {

Please check the event target and the transition property.

::: browser/base/content/browser.js:4970
(Diff revision 6)
>      if (!this._initialized)
>        return;
>  
> +    if (aDelay && this.animate) {
> +      this.reload.setAttribute("animate", "true");
> +      this.reload.parentNode.addEventListener("animationend", event => {

ditto
Attachment #8868308 - Flags: review?(dao+bmo) → review-
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review154420

::: browser/themes/shared/toolbarbutton-icons.inc.css:91
(Diff revision 6)
> +#reload-button > .toolbarbutton-animatable-box,
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 9px);
> +  margin-inline-start: 4px;

Where does this number come from?

::: browser/themes/shared/toolbarbutton-icons.inc.css:92
(Diff revision 6)
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 9px);
> +  margin-inline-start: 4px;
> +  width: 18px; /* the width of each frame within the SVG sprite */

Our icons are 16x16, why is this 18px wide? Not sure if this is causing any inconsistency atm, but this seems like a recipe for trouble.

::: browser/themes/shared/toolbarbutton-icons.inc.css:97
(Diff revision 6)
> +  width: 18px; /* the width of each frame within the SVG sprite */
> +}
> +
> +#reload-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image,
> +#stop-button > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> +  height: 20px; /* the height of each frame within the SVG sprite */

ditto
Depends on: 1373479
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review154420

> Our icons are 16x16, why is this 18px wide? Not sure if this is causing any inconsistency atm, but this seems like a recipe for trouble.

The start and end of the icon animations are 16x16, but the frames in the middle require a larger size. Due to the way that the animatable-box and animatable-image are absolutely positioned on the button, I don't suspect this will cause a problem since it won't affect the size of the button.
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review155564

::: browser/base/content/browser.js:4394
(Diff revision 7)
>  
>    setJSStatus() {
>      // unsupported
>    },
>  
> +  shouldAnimate(aWebProgress, aSwitchingToReload) {

This method name is too generic. Can you just pass aRequest and aWebProgress to switchToStop and switchToReload and let them figure this out?

::: browser/base/content/browser.js:4955
(Diff revision 7)
> +             event.target.animationName == "stop-to-reload")) {
> +          this.reload.removeAttribute("animate");
> +        }
> +      }
> +    }
> +    if (event.type == "click" &&

This seems to be already handled in the switch statement?

::: browser/themes/shared/toolbarbutton-icons.inc.css:90
(Diff revision 7)
> +
> +#reload-button > .toolbarbutton-animatable-box,
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 10px); /* Vertically center the 20px tall animatable image */

I think this relies on the buttons being inside #urlbar-container (which they won't be: bug 1363485) and #urlbar-container having -moz-box-align: center...

::: browser/themes/shared/toolbarbutton-icons.inc.css:91
(Diff revision 7)
> +#reload-button > .toolbarbutton-animatable-box,
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 10px); /* Vertically center the 20px tall animatable image */
> +  /* .toolbarbutton-icon is 16px wide plus 6px padding on each side.

The padding is not static, see --toolbarbutton-inner-padding.
Attachment #8868308 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #23)
> ::: browser/themes/shared/toolbarbutton-icons.inc.css:90
> (Diff revision 7)
> > +
> > +#reload-button > .toolbarbutton-animatable-box,
> > +#stop-button > .toolbarbutton-animatable-box {
> > +  position: absolute;
> > +  overflow: hidden;
> > +  top: calc(50% - 10px); /* Vertically center the 20px tall animatable image */
> 
> I think this relies on the buttons being inside #urlbar-container (which
> they won't be: bug 1363485) and #urlbar-container having -moz-box-align:
> center...

I didn't make any adjustments related to this, as there are no patches on bug 1363485 and it's unclear to me how I would write a patch based on how things might be written in the future. I think this is something that can be fixed either as part of bug 1363485 or as a follow-up.
Try push talos comparison of the latest version of the patch compared against m-c tip,
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ac9182c66a945ef02be0668033e3152ab2df9609&newProject=try&newRevision=e87a9606dbe771cc38fad2110c9e56b712a52c52&framework=1

https://screenshots.firefox.com/hQGhEGV7DSF3tNTS/treeherder.mozilla.org

sessionrestore opt e10s = 3.14% regression (med confidence)
sessionrestore_no_auto_restore opt e10s = 3.36% regression (high confidence)
tart opt e10s = 3.65% regression (med confidence)
tp5o % Processor Time opt e10s = 30.32% regression (high confidence)
ts_paint opt e10s = 2.64% regression (med confidence)
ts_paint_webext opt e10s = 2.41% regression (med confidence)

I thought the sessionrestore regressions would be fixed by bug 1372292.

I think the tart regression is coming from a tab animating the button and switching while the animation is in progress.

The tp5o regression looks very similar to what mconley was talking about in https://bugzilla.mozilla.org/show_bug.cgi?id=1357093#c21

For ts_paint, the current patch delays enabling the animation until "browser-delayed-startup-finished". We could try delaying enabling animation until 2s after startup.

I will have to profile these tests locally to see any other things that might get uncovered.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> (In reply to Dão Gottwald [::dao] from comment #23)
> > ::: browser/themes/shared/toolbarbutton-icons.inc.css:90
> > (Diff revision 7)
> > > +
> > > +#reload-button > .toolbarbutton-animatable-box,
> > > +#stop-button > .toolbarbutton-animatable-box {
> > > +  position: absolute;
> > > +  overflow: hidden;
> > > +  top: calc(50% - 10px); /* Vertically center the 20px tall animatable image */
> > 
> > I think this relies on the buttons being inside #urlbar-container (which
> > they won't be: bug 1363485) and #urlbar-container having -moz-box-align:
> > center...
> 
> I didn't make any adjustments related to this, as there are no patches on
> bug 1363485 and it's unclear to me how I would write a patch based on how
> things might be written in the future. I think this is something that can be
> fixed either as part of bug 1363485 or as a follow-up.

It doesn't make sense to fix this as part of bug 1363485; it's too detached / unrelated. The styling here needs to be more robust. I think you can test by just removing -moz-box-align: center...
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Compare-talos for the latest version,
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8a0a9fd8c1b2ffe44badcdfa8cd5d14072210a72&newProject=try&newRevision=8a0a9fd8c1b2ffe44badcdfa8cd5d14072210a72&framework=1&showOnlyImportant=0

I tested this patch with -moz-box-align:center; removed from the #urlbar-container and didn't see any change in the animation or icon.
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review157986

::: browser/base/content/browser.js:4961
(Diff revision 9)
> -      this._stopClicked = true;
> +          this._stopClicked = true;
> +        }
> +      case "animationend": {
> +        if (event.target.classList.contains("toolbarbutton-animatable-image") &&
> +            (event.animationName == "reload-to-stop" ||
> +             event.animationName == "stop-to-reload")) {

Need to check the RTL variants too.

::: browser/themes/shared/toolbarbutton-icons.inc.css:91
(Diff revision 9)
> +  }
> +}
> +
> +#reload-button[animate],
> +#stop-button[animate] {
> +  position: relative;

Something doesn't work as intended here. E.g. try adding an extra top-padding to the nav-bar and you'll see that the icon moves up while animating.

::: browser/themes/shared/toolbarbutton-icons.inc.css:96
(Diff revision 9)
> +  position: relative;
> +}
> +
> +#reload-button:not([animate]):not([displaystop]) > .toolbarbutton-animatable-box,
> +#reload-button:not([animate])[displaystop] + #stop-button > .toolbarbutton-animatable-box {
> +  display: none;

http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/browser/base/content/browser.css#426-429

Is this still needed?

Looks good otherwise, but I don't think this can land with the remaining perf regressions, tp5o % Processor Time opt e10s in particular.
Attachment #8868308 - Flags: review?(dao+bmo) → review-
Oh, and let's not add stop-static.svg and reload-static.svg. They already exist as stop.svg and reload.svg and will be updated in bug 1355455.
(In reply to Dão Gottwald [::dao] from comment #31)
> Comment on attachment 8868308 [details]
> Bug 1355924 - The refresh/stop button should have an animation when changing
> between states.
> 
> https://reviewboard.mozilla.org/r/139898/#review157986
> 
> ::: browser/base/content/browser.js:4961
> (Diff revision 9)
> > -      this._stopClicked = true;
> > +          this._stopClicked = true;
> > +        }
> > +      case "animationend": {
> > +        if (event.target.classList.contains("toolbarbutton-animatable-image") &&
> > +            (event.animationName == "reload-to-stop" ||
> > +             event.animationName == "stop-to-reload")) {
> 
> Need to check the RTL variants too.
> 
> ::: browser/themes/shared/toolbarbutton-icons.inc.css:91
> (Diff revision 9)
> > +  }
> > +}
> > +
> > +#reload-button[animate],
> > +#stop-button[animate] {
> > +  position: relative;
> 
> Something doesn't work as intended here. E.g. try adding an extra
> top-padding to the nav-bar and you'll see that the icon moves up while
> animating.

I moved the position:relative to the #stop-reload-button and this is fixed now.

> ::: browser/themes/shared/toolbarbutton-icons.inc.css:96
> (Diff revision 9)
> > +  position: relative;
> > +}
> > +
> > +#reload-button:not([animate]):not([displaystop]) > .toolbarbutton-animatable-box,
> > +#reload-button:not([animate])[displaystop] + #stop-button > .toolbarbutton-animatable-box {
> > +  display: none;
> 
> http://searchfox.org/mozilla-central/rev/
> fdb811340ac4e6b93703d72ee99217f3b1d250ac/browser/base/content/browser.
> css#426-429
> 
> Is this still needed?

No, not anymore now that we only set the list-style-image when the [animate] attribute is present.

> Looks good otherwise, but I don't think this can land with the remaining
> perf regressions, tp5o % Processor Time opt e10s in particular.

I talked with Jeff Griffiths and he is OK with the tp5o regression considering that we are doing more work now than we were before and this processor time is because we are now doing a 60fps animation. He would like to land the patches and then re-evaluate after a week or so if we need to backout the animation.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)
> > Looks good otherwise, but I don't think this can land with the remaining
> > perf regressions, tp5o % Processor Time opt e10s in particular.
> 
> I talked with Jeff Griffiths and he is OK with the tp5o regression
> considering that we are doing more work now than we were before and this
> processor time is because we are now doing a 60fps animation. He would like
> to land the patches and then re-evaluate after a week or so if we need to
> backout the animation.

Re-evaluate based on...? Talos already reports a ~25% Processor Time regression, which seems catastrophic enough to hold off landing this. If you do want to go ahead with this, I think you'll need module owner buy-in.
Summary: The Refresh/Stop button should have an animation when changing between states → The Reload/Stop button should have an animation when changing between states
mconley and I have talked with canuckistani and mossop and we are OK to move forward with this considering we are now doing more work compared to before. The Processor Time regression is coming from doing a 60fps animation here, which is having the refresh driver wake up the layout engine on each tick during the animation. We confirmed that lowering the frame rate of the animation reduces the processor time, but we would prefer to have the 60fps animation vs a lower frame rate. The TART regression is happening because during TART we run in ASAP mode which hits the compositor much more often even when there isn't any work to do (it may just return early in those cases) but it still bottlenecks the compositor which does synchronous uploads of textures to the GPU. Since ASAP mode isn't used in practice, this test does show that we are doing more work but we are conscious of our decision here.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36)
> mconley and I have talked with canuckistani and mossop and we are OK to move
> forward with this considering we are now doing more work compared to before.
> The Processor Time regression is coming from doing a 60fps animation here,
> which is having the refresh driver wake up the layout engine on each tick
> during the animation.

I understand we're doing more work, that's obvious. I do not understand why you think a 25% increase is acceptable. If we're lucky this won't affect page load time much in the basic case since the animation happens when we're done loading (but then I guess it might affect rendering performance, or loading of background tabs). Even if page load performance was no concern at all, this will likely still drain batteries. Can you elaborate on why this animation is crucial enough to justify that cost?
Flags: needinfo?(jaws)
(In reply to Dão Gottwald [::dao] from comment #37)
> I understand we're doing more work, that's obvious. I do not understand why
> you think a 25% increase is acceptable.

This is a classic engineering trade-off:

Is the 60fps animation worth the cost of the extra CPU cycles?

I feel like answering this question falls into Product's court, and canuckistani has (at least verbally) weighed in with a "yes". Is that enough to convince you, dao?
Flags: needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) from comment #38)
> (In reply to Dão Gottwald [::dao] from comment #37)
> > I understand we're doing more work, that's obvious. I do not understand why
> > you think a 25% increase is acceptable.
> 
> This is a classic engineering trade-off:
> 
> Is the 60fps animation worth the cost of the extra CPU cycles?
> 
> I feel like answering this question falls into Product's court, and
> canuckistani has (at least verbally) weighed in with a "yes". Is that enough
> to convince you, dao?

At the end of the day this will be enough for me to reluctantly r+ this. I'm not at all /convinced/ since no rationale was given. I think landing this would be a mistake and work directly against the goal of making 57 a release with superior performance (but even without that context I'd consider the perf impact unacceptable.)
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #39)
> (In reply to Mike Conley (:mconley) from comment #38)
> > (In reply to Dão Gottwald [::dao] from comment #37)
> > > I understand we're doing more work, that's obvious. I do not understand why
> > > you think a 25% increase is acceptable.
> > 
> > This is a classic engineering trade-off:
> > 
> > Is the 60fps animation worth the cost of the extra CPU cycles?
> > 
> > I feel like answering this question falls into Product's court, and
> > canuckistani has (at least verbally) weighed in with a "yes". Is that enough
> > to convince you, dao?
> 
> At the end of the day this will be enough for me to reluctantly r+ this. I'm
> not at all /convinced/ since no rationale was given. I think landing this
> would be a mistake and work directly against the goal of making 57 a release
> with superior performance (but even without that context I'd consider the
> perf impact unacceptable.)

I think where I get to with this is, we should land it, we should be able to pref it off, and we should meet and evaluate how it feels in the wild and if we can measure a real-world regression. For animations and cpu / gpu regressions I simply don't trust TALOS enough I want additional real-world evaluation.

I think a good outcome is: 60fps animations actually use less cpu cycles than we're seeing here.

I think a good mitigation strategy would be dropping to 30fps. I'm unhappy that the gpu isn't able to help us more here.

Jared: can we revisit this issue after the patch has been on nightly for a week or so? I'm thinking a discussion a week after next.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #40)
> (In reply to Dão Gottwald [::dao] from comment #39)
> > (In reply to Mike Conley (:mconley) from comment #38)
> > > (In reply to Dão Gottwald [::dao] from comment #37)
> > > > I understand we're doing more work, that's obvious. I do not understand why
> > > > you think a 25% increase is acceptable.
> > > 
> > > This is a classic engineering trade-off:
> > > 
> > > Is the 60fps animation worth the cost of the extra CPU cycles?
> > > 
> > > I feel like answering this question falls into Product's court, and
> > > canuckistani has (at least verbally) weighed in with a "yes". Is that enough
> > > to convince you, dao?
> > 
> > At the end of the day this will be enough for me to reluctantly r+ this. I'm
> > not at all /convinced/ since no rationale was given. I think landing this
> > would be a mistake and work directly against the goal of making 57 a release
> > with superior performance (but even without that context I'd consider the
> > perf impact unacceptable.)
> 
> I think where I get to with this is, we should land it, we should be able to
> pref it off, and we should meet and evaluate how it feels in the wild and if
> we can measure a real-world regression. For animations and cpu / gpu
> regressions I simply don't trust TALOS enough I want additional real-world
> evaluation.
> 
> I think a good outcome is: 60fps animations actually use less cpu cycles
> than we're seeing here.
> 
> I think a good mitigation strategy would be dropping to 30fps. I'm unhappy
> that the gpu isn't able to help us more here.

I think we should also consider not having this animation at all if there's any significant cost. From all photon animations, this one feels like it's more on the "bells and whistles" and "just because we can" spectrum, and to be honest I found it pretty distracting when I tried the patch.
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review158880

::: browser/base/content/browser.js:4986
(Diff revision 11)
> +  startAnimationPrefMonitoring() {
> +    requestIdleCallback(() => {
> +      this.animate = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> +      if (!Services.prefs.getBoolPref("browser.stopReloadAnimation.enabled")) {
> +        this.animate = false;
> +      }

this.animate = Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled") &&
               Services.prefs.getBoolPref("browser.stopReloadAnimation.enabled");

::: browser/base/content/browser.js:5002
(Diff revision 11)
> +                        aRequest instanceof Ci.nsIRequest &&
> +                        aWebProgress.isTopLevel &&
> +                        aWebProgress.isLoadingDocument;
> +
>      this._cancelTransition();
> +    if (shouldAnimate && this.animate)

Please just check this.animate in the shouldAnimate declaration.

::: browser/themes/shared/toolbarbutton-icons.inc.css:87
(Diff revision 11)
> +  position: relative;
> +}
> +
> +#reload-button > .toolbarbutton-animatable-box,
> +#stop-button > .toolbarbutton-animatable-box {
> +  position: absolute;

Would it be better to use postion:fixed here like in bug 1375152?
Attachment #8868308 - Flags: review?(dao+bmo) → review+
Flags: needinfo?(jaws)
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review159174
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review159192
Attachment #8868308 - Flags: review+
Comment on attachment 8868308 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/139898/#review159208
Attachment #8868308 - Flags: review+
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3beaca4665c1
The refresh/stop button should have an animation when changing between states. r=dao
I couldn't figure out why this was refusing to be autolanded, so went ahead and pushed the patch to inbound.
Autoland probably refused because it realized that an opt-only try push wouldn't show the hundreds of "ASSERTION: inline-size less than zero: 'aContainingBlockISize >= 0'" that would fire, and the hundreds of tests in suites that count assertions that would fail as a result.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4630d37dca64 for https://treeherder.mozilla.org/logviewer.html#?job_id=111595514&repo=mozilla-inbound etc. (https://treeherder.mozilla.org/logviewer.html#?job_id=111595119&repo=mozilla-inbound is also cute, since the full log is 82Mb of assertion failures)
Flags: needinfo?(jaws)
Not sure about intermittency, but you also seem to have a failure in browser_1007336_lwthemes_in_customize_mode.js and browser_customizemode_uidensity.js at least on Mac and Windows, https://treeherder.mozilla.org/logviewer.html#?job_id=111595144&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=111602689&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=111602121&repo=mozilla-inbound
Something must have changed out from under you (or in your patch) between the try push and landing, too, since the try push should have caught https://treeherder.mozilla.org/logviewer.html#?job_id=111594628&repo=mozilla-inbound timing out browser_formless_submit_chrome.js and https://treeherder.mozilla.org/logviewer.html#?job_id=111605733&repo=mozilla-inbound timing out browser_temporary_permissions_navigation.js
(In reply to Dão Gottwald [::dao] from comment #41)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #40)
> > (In reply to Dão Gottwald [::dao] from comment #39)
> > > (In reply to Mike Conley (:mconley) from comment #38)
> > > > (In reply to Dão Gottwald [::dao] from comment #37)
> > > > > I understand we're doing more work, that's obvious. I do not understand why
> > > > > you think a 25% increase is acceptable.
> > > > 
> > > > This is a classic engineering trade-off:
> > > > 
> > > > Is the 60fps animation worth the cost of the extra CPU cycles?
> > > > 
> > > > I feel like answering this question falls into Product's court, and
> > > > canuckistani has (at least verbally) weighed in with a "yes". Is that enough
> > > > to convince you, dao?
> > > 
> > > At the end of the day this will be enough for me to reluctantly r+ this. I'm
> > > not at all /convinced/ since no rationale was given. I think landing this
> > > would be a mistake and work directly against the goal of making 57 a release
> > > with superior performance (but even without that context I'd consider the
> > > perf impact unacceptable.)
> > 
> > I think where I get to with this is, we should land it, we should be able to
> > pref it off, and we should meet and evaluate how it feels in the wild and if
> > we can measure a real-world regression. For animations and cpu / gpu
> > regressions I simply don't trust TALOS enough I want additional real-world
> > evaluation.
> > 
> > I think a good outcome is: 60fps animations actually use less cpu cycles
> > than we're seeing here.
> > 
> > I think a good mitigation strategy would be dropping to 30fps. I'm unhappy
> > that the gpu isn't able to help us more here.
> 
> I think we should also consider not having this animation at all if there's
> any significant cost. From all photon animations, this one feels like it's
> more on the "bells and whistles" and "just because we can" spectrum, and to
> be honest I found it pretty distracting when I tried the patch.

Noted, removing it is definitely an option but I'd like feedback first.
No longer depends on: 1368653
Comment on attachment 8883564 [details]
Bug 1355924 - The refresh/stop button should have an animation when changing between states.

https://reviewboard.mozilla.org/r/154498/#review160414

::: browser/themes/shared/toolbarbutton-icons.inc.css:96
(Diff revision 2)
> +     the `width` and `height` CSS properties causes an assertion for
> +     `inline-size less than zero: 'aContainingBlockISize >= 0'`. */
> +  min-width: 18px;
> +  min-height: 20px;
> +  max-width: 18px;
> +  max-height: 20px;

Please group min-width with max-width and min-height with max-height here, and file a bug on the assertion.
Attachment #8883564 - Flags: review?(dao+bmo) → review+
Attachment #8868308 - Attachment is obsolete: true
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66528bf3d70d
The refresh/stop button should have an animation when changing between states. r=dao
https://hg.mozilla.org/mozilla-central/rev/66528bf3d70d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1379465
Depends on: 1379480
Depends on: 1379491
Depends on: 1379492
Depends on: 1379620
Depends on: 1379625
Depends on: 1379896
I have verified this on today nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1380947
Depends on: 1381957
Depends on: 1381993
Depends on: 1384180
Blocks: 1379480
No longer depends on: 1379480
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: