1312687 - Don't move devtools-side-splitter in the opposite way in RTL locales

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[magicp]

Attachment: do-not-move-splitter-in-the-opposite-way.mp4

Steps to reproduce:
1. Start Nightly in RTL locales (e.g. Arabic)
2. Open DevTools > Memory
3. Switch view to "Aggregate"
4. Take snapshot
5. Click [⁂]individuals-button > View Individual nodes
6. Drag devtools-side-splitter

Acrual Results:
devtools-side-splitter moves in the opposite way.

Expected Results:
devtools-side-splitter should be moved in the same way with mouse.

Comment 1

[Tim Nguyen :ntim]

At https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/h-split-box.js#110 :

You'll need to change the sign of the `relative` variable when the root element's (this.refs.box.ownerDocument.documentElement) `dir` attribute is set to "rtl"

Comment 2

[Nick Fox [:njfox]]

Attachment: Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

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

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

https://reviewboard.mozilla.org/r/116678/#review118442

::: devtools/client/shared/components/h-split-box.js
(Diff revision 1)
>  
>    _onMouseMove(event) {
>      if (!this.state.mouseDown) {
>        return;
>      }
> -

We typically want to space separate if statements to create a clean logical block. Don't remove this new line.

::: devtools/client/shared/components/h-split-box.js:114
(Diff revision 1)
> -    const relative = event.clientX - left;
> -    this.props.onResize(relative / width);
> +    const relativeRTL = right - event.clientX;
> +    const relativeLTR = event.clientX - left;
> +
> +    if (this.refs.box.ownerDocument.documentElement.getAttribute("dir") == "rtl") {
> +      this.props.onResize(relativeRTL / width);
> +    }

} else {

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

https://reviewboard.mozilla.org/r/116678/#review118448

The patch works great! Thanks! Please address :gl's comment about the new line as well.

::: devtools/client/shared/components/h-split-box.js:112
(Diff revision 1)
> +    if (this.refs.box.ownerDocument.documentElement.getAttribute("dir") == "rtl") {
> +      this.props.onResize(relativeRTL / width);
> +    }
> +    else {
> +      this.props.onResize(relativeLTR / width);
> +    }

You can avoid the relativeRTL/relativeLTR variables by using a ternary operator.

Also, I just discovered that you can get the direction using ownerDocument.dir.

const direction = this.refs.box.ownerDocument.dir;
const relative = direction == "rtl" ? right - event.clientX
                                    : event.clientX - left;

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

https://reviewboard.mozilla.org/r/116678/#review118520

::: commit-message-e91de:1
(Diff revision 1)
> +Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales r?fitzgen

Can you change r?fitzgen to r?ntim as well?

Comment 6

[Nick Fox [:njfox]]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116678/diff/1-2/

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

https://reviewboard.mozilla.org/r/116678/#review118548

Thanks!

::: devtools/client/shared/components/h-split-box.js:112
(Diff revision 2)
>      const rect = this.refs.box.getBoundingClientRect();
>      const { left, right } = rect;
>      const width = right - left;
> -    const relative = event.clientX - left;
> +    const direction = this.refs.box.ownerDocument.dir;
> +    const relative = direction == "rtl" ? right - event.clientX
> +                                    : event.clientX - left;

Can you align : and ? in the ternary operator?

Comment 8

[Nick Fox [:njfox]]

Comment on attachment 8842950 [details]
Bug 1312687 - devtools-side-splitter now moves the correct direction in RTL locales

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116678/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edf005cc0946
devtools-side-splitter now moves the correct direction in RTL locales r=ntim

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Is this worth uplifting to soon-to-be-beta 53?

Comment 12

[Nick Fox [:njfox]]

Per our discussion on IRC, I believe it's fine to wait for 54 on this one.