Closed Bug 1456828 Opened 6 years ago Closed 6 years ago

Scroll position remembered in animation properties bar

Categories

(DevTools :: Inspector: Animations, defect, P1)

Product:
DevTools
Component:
Inspector: Animations
Version:
61 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 disabled, firefox62+ verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- disabled
firefox62 + verified

People

(Reporter: laszlo.bialis, Assigned: daisuke)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

scroll position.PNG
83.33 KB, image/png
Details
Bug 1456828 - Part 1: Introduce ProgressInspectionPanel.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1456828 - Part 2: Apply ProgressInspectionPanel to animations.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1456828 - Part 3: Apply ProgressInspectionPanel to keyframes.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1456828 - Part 4: Add a test that whether the scroll amount change.
59 bytes, text/x-review-board-request
gl
: review+
Details
Attached image scroll position.PNG — 
[Environment]: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Also reproducible on MacOS 10.12, Ubuntu 16.04, Win7
[Build ID]: 20180425100122

[Steps:]
1. Load: https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html
3. Press F12.
4. Select Inspector.
5. Select Animation tab.
6. Select <div class="ball longhand"></div>
7. On the animation properties panel for longhand - CSS Animation scroll down to the last animation "padding-top"
8. Select <div class="ball negative-delay"></div>
9. Inspect on the animation properties panel the simple-animation - CSS Animation

[Actual Result]: 
Nothing can be seen in the animation panel as the scroll position is remembered from the previous div class. If one scrolls upwards the "transform" animation property can be seen and afterwards no scrolling is available as none is necessary.

[Expected Result]:
The scroll window position should be reset and should jump to the first available animation property as a new div class is selected.
Needs Triage?
Flags: needinfo?(gl)
Flags: needinfo?(gl) → needinfo?(dakatsuka)
The React-based Animation Inspector is still Nightly-only.
status-firefox60: --- → unaffected
status-firefox61: affected → disabled
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
Assignee: nobody → dakatsuka
Flags: needinfo?(dakatsuka)
I'm sorry for delay. I had investigated this issue.
This looks like layout bug. I filed that bug 1462235.
Depends on: 1462235
Priority: -- → P3
Marking P1 to indicate we want to fix this before shipping the new animation inspector.
Priority: P3 → P1
Status: NEW → ASSIGNED
Attachment #8983033 - Attachment is obsolete: true
Rebased
Rebased and CSS adjustment ( delay/endDelay sign was hidden half ).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=797c27789dec439b2cf57f0fffde3cbda5fbd5e7
Comment on attachment 8983352 [details]
Bug 1456828 - Part 3: Apply ProgressInspectionPanel to keyframes.

https://reviewboard.mozilla.org/r/249250/#review256520


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/test/browser_animation_keyframes-progress-bar.js:88
(Diff revision 3)
>        await clickOnCurrentTimeScrubberController(animationInspector,
>                                                   panel, scrubberPositions[i]);
>        assertPosition(barEl, areaEl, expectedPositions[i], animationInspector);
>      }
>    }
>    console.log("XXXTIME:"+(Date.now() - time));

Error: Infix operators must be spaced. [eslint: space-infix-ops]
Could you review this one, Gabriel?
Flags: needinfo?(gl)
Product: Firefox → DevTools
Comment on attachment 8983350 [details]
Bug 1456828 - Part 1: Introduce ProgressInspectionPanel.

https://reviewboard.mozilla.org/r/249246/#review257206

::: devtools/client/inspector/animation/components/ProgressInspectionPanel.js:15
(Diff revision 4)
> +
> +const TickLabels = createFactory(require("./TickLabels"));
> +const TickLines = createFactory(require("./TickLines"));
> +
> +/**
> + * This class is a panel for the progress of animations or keyframes, supports displaying

s/class/component

::: devtools/client/inspector/animation/components/TickLabels.js:12
(Diff revision 4)
> +const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +/**
> + * This class is intented to show tick labels on the header.

s/class/component

::: devtools/client/inspector/animation/components/TickLabels.js:12
(Diff revision 4)
> +const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +/**
> + * This class is intented to show tick labels on the header.

s/intented/intended

::: devtools/client/inspector/animation/components/TickLines.js:12
(Diff revision 4)
> +const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +/**
> + * This class is intented to show the tick line as the background.

s/class/component
Attachment #8983350 - Flags: review?(gl) → review+
Comment on attachment 8983351 [details]
Bug 1456828 - Part 2: Apply ProgressInspectionPanel to animations.

https://reviewboard.mozilla.org/r/249248/#review257208
Attachment #8983351 - Flags: review?(gl) → review+
Comment on attachment 8983352 [details]
Bug 1456828 - Part 3: Apply ProgressInspectionPanel to keyframes.

https://reviewboard.mozilla.org/r/249250/#review257212
Attachment #8983352 - Flags: review?(gl) → review+
Comment on attachment 8983353 [details]
Bug 1456828 - Part 4: Add a test that whether the scroll amount change.

https://reviewboard.mozilla.org/r/249252/#review257214
Attachment #8983353 - Flags: review?(gl) → review+
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/faa7c8d0c069
Part 1: Introduce ProgressInspectionPanel. r=gl
https://hg.mozilla.org/integration/autoland/rev/2df7feea4ae1
Part 2: Apply ProgressInspectionPanel to animations. r=gl
https://hg.mozilla.org/integration/autoland/rev/0f6a56e95917
Part 3: Apply ProgressInspectionPanel to keyframes. r=gl
https://hg.mozilla.org/integration/autoland/rev/3e53ab8d73b8
Part 4: Add a test that whether the scroll amount change. r=gl
Backed out 4 changesets (bug 1456828) for breaking devtools on devtools/client/debugger/test/mochitest/browser_dbg_sources-iframe-reload.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=183279796&repo=autoland&lineNumber=2549

01:43:27     INFO - TEST-PASS | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js | The path segment of x 100, y 100 should be passing through - 
[task 2018-06-15T01:43:27.315Z] 01:43:27     INFO - Checking interaction for hint[0] of opacity in narrow-keyframes
[task 2018-06-15T01:43:27.316Z] 01:43:27     INFO - Buffered messages finished
[task 2018-06-15T01:43:27.316Z] 01:43:27     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js | stroke-opacity of hintEl for hint[0] of opacity in narrow-keyframes should be 1 while mouse is over the element - 
[task 2018-06-15T01:43:27.317Z] 01:43:27     INFO - Stack trace:
[task 2018-06-15T01:43:27.317Z] 01:43:27     INFO - chrome://mochitests/content/browser/devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js:null:248
[task 2018-06-15T01:43:27.318Z] 01:43:27     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1098
[task 2018-06-15T01:43:27.318Z] 01:43:27     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1089
[task 2018-06-15T01:43:27.318Z] 01:43:27     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:991
[task 2018-06-15T01:43:27.318Z] 01:43:27     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-06-15T01:43:27.319Z] 01:43:27     INFO - TEST-PASS | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js | stroke-opacity of hintEl for hint[0] of opacity in narrow-keyframes should be 0 while mouse is out from the element - 
[task 2018-06-15T01:43:27.320Z] 01:43:27     INFO - Checking hint[1] of opacity in narrow-keyframes
[task 2018-06-15T01:43:27.324Z] 01:43:27     INFO - Checking <title> in hint[1] of opacity in narrow-keyframes

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3e53ab8d73b81d38c177b4fc8169589704e882a7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Backout:
https://treeherder.mozilla.org/logviewer.html#?job_id=183279796&repo=autoland&lineNumber=2549
Flags: needinfo?(dakatsuka)
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc0dac8e5646
Backed out 4 changesets for breaking devtools on devtools/client/debugger/test/mochitest/browser_dbg_sources-iframe-reload.js
Please note that this issue also occurs in the case of the animation graphs panel, just like in the case of the animation properties panel under it.
Bodae, yes I'm understanding.

The cause of error of the backout was the progress bar had stolen the mouse over event.
So I restored the CSS one line.
Please let me make the patches to land again since there are no errors in the try now.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a630d840314
Part 1: Introduce ProgressInspectionPanel. r=gl
https://hg.mozilla.org/integration/autoland/rev/fd5f1c2b4453
Part 2: Apply ProgressInspectionPanel to animations. r=gl
https://hg.mozilla.org/integration/autoland/rev/70c18cb539be
Part 3: Apply ProgressInspectionPanel to keyframes. r=gl
https://hg.mozilla.org/integration/autoland/rev/cea25564c32d
Part 4: Add a test that whether the scroll amount change. r=gl
Flags: needinfo?(gl)
Verified that the problem is fixed on Ubuntu 16.04, Osx 10.13.3 & Windows 10 on latest Nightly 06-18-2018.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: