Closed Bug 1454973 Opened 6 years ago Closed 6 years ago

Animation scrubber is not displayed correctly on RTL language versions

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox61 disabled, firefox62+ verified, firefox64 verified, firefox65 verified, firefox66 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox61 --- disabled
firefox62 + verified
firefox64 --- verified
firefox65 --- verified
firefox66 --- verified

People

(Reporter: gpalko, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

RTLscrubber.png
101.46 KB, image/png
Details
animation-inspector-rtl.png
284.48 KB, image/png
Details
Bug 1454973 - Part 1. Support RTL in animation inspector.
59 bytes, text/x-review-board-request
daisuke
: review+
Details
Bug 1454973 - Part 2.Add the animation inspector test of RTL environment.
59 bytes, text/x-review-board-request
daisuke
: review+
Details
Attached image RTLscrubber.png — 
[Environment:]
Windows 10,
Nightly 61.0a1 2018-04-17

[Steps:]
1. Install and open Arabic Firefox Nightly
2. Load https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html
3. Press F12.
4. Select Inspector.
5. Select Animation tab.
6. Inspect the animation scrubber

[Actual Result:]
The scrubber is displayed and moving on the animation node names.(see attached screenshot)

[Expected Result:]
5. The scrubber should be displayed and move on the animation bars
Priority: -- → P3
This is direction problem. This scrubber uses transofrm(translateX) with own offset value. This calculation uses the value which is based on LTR. So We need to modify this calculation[1].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/devtools/client/inspector/animation/components/CurrentTimeScrubberController.js#58
Attached image animation-timeline-rtl.png (obsolete) — 
I discussed with Daisuke and Brian about timeline order in RTL, I think that we need to render timeline from right to left in RTL environment. For example, vis.js which is visialization library will support RTL[1], this library will render timeline from right to left if application specify the rtl option. 

So I'd like to change not only the scrubber position but also the animation timeline in RTL environment in this bug.

[1] https://github.com/almende/vis/issues/112

I think that we need following things to support RTL timeline:

 === Animation list ===
 * Animation tick timeline position calculation
   * Modify left position calculation in RTL
 * Reverse an animation target svg
 * Modify scrubber position calculation

 === Keyframe list ===
 * Keyframe tick timeline position calculation
   * Modify left position calculation in RTL 
 * Modify keyframe progressbar position calculation
 * Reverse animation property target svg

An attachment is the image of expected.
I decided tha I separate this bug to two things:
 1) Fix Animation target panel
 2) Fix Keyframe panel

The second modification (Fix keyframe panel) will be treated in bug 1454979.

So this bug will fix animation target panel issues.

This is WIP code for this:
https://hg.mozilla.org/try/rev/d906742b464a
Marking this as P1 to indicate we want to fix this before shipping this feature (since it makes the inspector very hard to use in RTL builds).
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Priority: P3 → P1
Updating flags as follows:
marking 61 as disabled - since this issue tracks the React version which is currently off for 61, although RTL issues are also found in the old animation inspector as well;
setting 62 as target version for this bug;
status-firefox61: affected → disabled
status-firefox62: --- → affected
Target Milestone: --- → Firefox 62
Version: 61 Branch → Trunk
This bug is related with bug 1456828 since Daisuke will change the animation inspector's implementation drastically. This is experimental patch by using Daisuke's WIP patch:

https://hg.mozilla.org/try/rev/a18f606d78a74103e1782d1519aa8d3ce383ab03
WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbd845ee624f929fd9d542e70854d0235c025997

An attachment is capture of this.

I got the diasuke's feedback:

 * Play button direction ([<] or [>] ?)
 * The scrubber and progress is not applied color style.
 * The "100%" of animated property label need to be display on the right.
 * We need to reverse the negative delay graph.
 * An animation name might need to align the right.
Attachment #8981294 - Attachment is obsolete: true
Product: Firefox → DevTools
Comment on attachment 8986369 [details]
Bug 1454973 - Part 2. Modify the test associated with supporting RTL.

https://reviewboard.mozilla.org/r/251738/#review258110

Thanks Man-san!

I think, this change should include in first patch since if apply first patch only, these tests should be failed.
Attachment #8986369 - Flags: review?(dakatsuka)
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258106

Thanks Man-san!

Please let me cancel this review once, since I found a bug which have not been displayed the tick label of 100% in animated property list header. Could you take a look?

::: devtools/client/inspector/animation/animation.js:103
(Diff revision 1)
>        toggleElementPicker,
>      } = this;
>  
>      const target = this.inspector.target;
> +    const direction =
> +          this.win.getComputedStyle(this.win.document.documentElement).direction;

It seems we can use "dir" property of win.document.
(e.g. SplitBox.js[1])

```
const direction = this.win.document.dir;
```

[1] https://searchfox.org/mozilla-central/source/devtools/client/shared/components/splitter/SplitBox.js#180

::: devtools/client/inspector/animation/animation.js:139
(Diff revision 1)
>            setHighlightedNode,
>            setSelectedNode,
>            simulateAnimation,
>            simulateAnimationForKeyframesProgressBar,
>            toggleElementPicker,
> +          direction,

Please keep alphabetize.
The other parts are as well.

::: devtools/client/inspector/animation/components/CurrentTimeScrubber.js:117
(Diff revision 1)
>        setAnimationsCurrentTime,
>        timeScale,
>      } = this.props;
>  
> -    const time = pageX - this.controllerArea.x < 0 ?
> -                   0 :
> +    let progressRate = (pageX - this.controllerArea.x) / this.controllerArea.width;
> +    if (progressRate < 0.0) {

Sorry, please insert a blank line before this line Because we usually insert before and after "if else", "for", "switch" blocks. (This is not eslint rule, but in case of the animation/grid/flex we are.)

::: devtools/client/inspector/animation/components/CurrentTimeScrubber.js:120
(Diff revision 1)
>  
> -    const time = pageX - this.controllerArea.x < 0 ?
> -                   0 :
> -                   (pageX - this.controllerArea.x) /
> -                     this.controllerArea.width * timeScale.getDuration();
> +    let progressRate = (pageX - this.controllerArea.x) / this.controllerArea.width;
> +    if (progressRate < 0.0) {
> +      progressRate = 0.0;
> +    }
> +    if (progressRate > 1.0) {

If we need this, please use `else if`.

::: devtools/client/inspector/animation/components/CurrentTimeScrubber.js:122
(Diff revision 1)
> -                   0 :
> -                   (pageX - this.controllerArea.x) /
> -                     this.controllerArea.width * timeScale.getDuration();
> +    if (progressRate < 0.0) {
> +      progressRate = 0.0;
> +    }
> +    if (progressRate > 1.0) {
> +      progressRate = 1.0;
> +    }

Likewise.

::: devtools/client/inspector/animation/components/IndicationBar.js:62
(Diff revision 1)
>  
>      return dom.div(
>        {
>          className: `indication-bar ${ className }`,
>          style: {
> -          transform: `translateX(${ offset }px)`,
> +          transform: `translateX(${ tx }px)`,

I'm not sure, but just like TickLavel and TickLine, if magrinInlineStart/End can be used here, it's so nice since we will not consider the rtl/ltr in the code, I think.
What do you think??
Attachment #8986368 - Flags: review?(dakatsuka)
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258106

Thanks! Daisuke.

I'll fix soon.

> It seems we can use "dir" property of win.document.
> (e.g. SplitBox.js[1])
> 
> ```
> const direction = this.win.document.dir;
> ```
> 
> [1] https://searchfox.org/mozilla-central/source/devtools/client/shared/components/splitter/SplitBox.js#180

Thanks, I'll use it.

> I'm not sure, but just like TickLavel and TickLine, if magrinInlineStart/End can be used here, it's so nice since we will not consider the rtl/ltr in the code, I think.
> What do you think??

IndicationBar used marginInlineStart in order to display scrubber on clicked position at the moment.[1] So we will need to adjust this marginInlineStart value if we use marginInlineStart in order to move the scrubber position. However, it is reasonable for me to use marginInlineStart.
[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/client/themes/animation.css#119
Comment on attachment 8986369 [details]
Bug 1454973 - Part 2. Modify the test associated with supporting RTL.

https://reviewboard.mozilla.org/r/251738/#review258110

Thanks!

I'll squash this patch to the patch of part 1.
Attachment #8986369 - Attachment is obsolete: true
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258144

Thanks Man-san!
Please address where I commented as issue.

::: devtools/client/inspector/animation/components/CurrentTimeScrubber.js:123
(Diff revision 2)
> -                   (pageX - this.controllerArea.x) /
> -                     this.controllerArea.width * timeScale.getDuration();
> +    if (progressRate < 0.0) {
> +      progressRate = 0.0;
> +    } else if (progressRate > 1.0) {
> +      progressRate = 1.0;
> +    }
> +    const time = this.props.direction === "ltr"

Please get "direction" as follows:

```
const {
  direction,
  setAnimationsCurrentTime,
  timeScale,
} = this.props;
```

::: devtools/client/inspector/animation/components/IndicationBar.js:56
(Diff revision 2)
>  
>    render() {
>      const { className } = this.props;
>      const { offset } = this.state;
> +    // Adjust margin inline position in order to display on clicked position.
> +    const tx = offset - 6;

Could we change the offset of bar using `transform: translateX(-6px);` in CSS instead?

::: devtools/client/inspector/animation/components/graph/AnimationName.js:32
(Diff revision 2)
>          className: "animation-name",
>        },
>        dom.text(
>          {
>            y: "50%",
> -          x: "100%"
> +          x: direction === "ltr" ? "100%" : "0%",

We might be able to use same value (e.g. 100%) for x attribute. Instead of that, `transform: translateX(-100%);` in CSS can be used for RTL.
Could you comfirm?
Attachment #8986368 - Flags: review?(dakatsuka) → review+
Comment on attachment 8986370 [details]
Bug 1454973 - Part 2.Add the animation inspector test of RTL environment.

https://reviewboard.mozilla.org/r/251740/#review258118

::: devtools/client/inspector/animation/test/browser_animation_current-time-scrubber-rtl.js:6
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +/* import-globals-from current-time-scrubber_head.js */

There are tests that use import-globals-from or eslint-disable-next-line so as to use the common function in new file.
I'd keep the consistency that whether we use import-globals-from or eslint-disable-next-line no-undef. Maybe, import-globals-from is nicer?

::: devtools/client/inspector/animation/test/browser_animation_current-time-scrubber-rtl.js:7
(Diff revision 1)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +/* import-globals-from current-time-scrubber_head.js */
> +

Could you add the explanation for this test here?
Other tests as well.
Attachment #8986370 - Flags: review?(dakatsuka) → review+
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258144

Thanks! daisuke!

> We might be able to use same value (e.g. 100%) for x attribute. Instead of that, `transform: translateX(-100%);` in CSS can be used for RTL.
> Could you comfirm?

Yes!
I confirmed this, so we can reduce js code. thanks.
Comment on attachment 8986370 [details]
Bug 1454973 - Part 2.Add the animation inspector test of RTL environment.

https://reviewboard.mozilla.org/r/251740/#review258118

> There are tests that use import-globals-from or eslint-disable-next-line so as to use the common function in new file.
> I'd keep the consistency that whether we use import-globals-from or eslint-disable-next-line no-undef. Maybe, import-globals-from is nicer?

I addressed it according to other implementation.
Daisuke will land the bug 1431576, this changes contain animation.css, so bug 1431576 will affect to this bug. So I'll land this patches after daisuke landed that patches.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #21)
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=561c690452316a591a94a5bdf82032855722b076&group_state=e
> xpanded

RTL test is failed only if a target is debugging build. I click the most right of the scrubbable area, mouse event might not occur. [1] However, opt build succeeds.  I tried manual test which is clicking the most right of the scrubbable area. This test succeeded. (i.e. I can set the 0ms with clicking most right of the scrubbable area)

[1] https://hg.mozilla.org/try/diff/7c108559b1d2/devtools/client/inspector/animation/test/current-time-scrubber_head.js#l1.36
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258656

::: devtools/client/inspector/animation/test/browser_animation_pause-resume-button_end-time.js:54
(Diff revision 3)
>  }
>  
>  function assertScrubberPosition(panel) {
>    const scrubberEl = panel.querySelector(".current-time-scrubber");
> -  const translateX = parseFloat(scrubberEl.style.transform.match(/-?\d+(\.\d+)?/)[0]);
> +  const computedScrubberEl = getComputedStyle(scrubberEl);
> +  const translateX = parseFloat(computedScrubberEl.transform.match(/-?\d+(\.\d+)?/)[0]);

Ah, sorry I missed this.
We need to change to reflect the marginInlineStart.
Comment on attachment 8986954 [details]
Bug 1454973 - Part 3. Make the edge of scrubber area in RTL to be clickable.

https://reviewboard.mozilla.org/r/252192/#review258658

Thanks Man-san!

Could you move this patch before the test patch like following?

* Part 1. Support RTL in animation inspector.
* Part 2. Make the edge of scrubber area in RTL to be clickable. 
* Part 3. Add the animation inspector test of RTL environment.
Attachment #8986954 - Flags: review?(dakatsuka)
Attachment #8986954 - Attachment is obsolete: true
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258656

> Ah, sorry I missed this.
> We need to change to reflect the marginInlineStart.

Thanks a lot!
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258664


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_pause-resume-button_end-time.js:53
(Diff revision 4)
>    });
>  }
>  
>  function assertScrubberPosition(panel) {
>    const scrubberEl = panel.querySelector(".current-time-scrubber");
> -  const translateX = parseFloat(scrubberEl.style.transform.match(/-?\d+(\.\d+)?/)[0]);
> +  const translateX = parseFloat(scrubberEl.style.marginInlineStart.match(/-?\d+(\.\d+)?/)[0]);

Error: Line 53 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8986368 [details]
Bug 1454973 - Part 1. Support RTL in animation inspector.

https://reviewboard.mozilla.org/r/251736/#review258664

> Error: Line 53 exceeds the maximum line length of 90. [eslint: max-len]

Oops, I fixed this error, and made several code to be clean.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39)
> Comment on attachment 8986368 [details]
> Bug 1454973 - Part 1. Support RTL in animation inspector.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/251736/diff/6-7/

Fix the indent nit.

Try with all tests and all platform: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d1807c65ce0a0aa26971682f576a95fd77bbf5&group_state=expanded
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/030a5574e69b
Part 1. Support RTL in animation inspector. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/7f7a190b3d0d
Part 2.Add the animation inspector test of RTL environment. r=daisuke
Flags: qe-verify+
This issue is fixed in Firefox Release 64.0 and Nightly 66.0a1 (2018-12-18).
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox64: --- → verified
status-firefox66: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: