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

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: magicp.jp, Assigned: njfox)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
Firefox 54
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Reporter

Updated

3 years ago

Updated

2 years ago
Keywords: good-first-bug
Whiteboard: [lang=js

Updated

2 years ago
Whiteboard: [lang=js → [lang=js]
Reporter

Updated

2 years ago

Comment 1

2 years ago
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 hidden (mozreview-request)
Attachment #8842950 - Flags: review?(nfitzgerald) → review?(ntim.bugs)

Comment 3

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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;
Attachment #8842950 - Flags: review?(ntim.bugs)

Comment 5

2 years ago
mozreview-review
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?

Updated

2 years ago
Assignee: nobody → nick
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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?
Attachment #8842950 - Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edf005cc0946
devtools-side-splitter now moves the correct direction in RTL locales r=ntim
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edf005cc0946
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Is this worth uplifting to soon-to-be-beta 53?
Assignee

Comment 12

2 years ago
Per our discussion on IRC, I believe it's fine to wait for 54 on this one.
Flags: needinfo?(nick)

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.