Closed
Bug 1312687
Opened 8 years ago
Closed 7 years ago
Don't move devtools-side-splitter in the opposite way in RTL locales
Categories
(DevTools :: Memory, defect)
DevTools
Memory
Tracking
(firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)
RESOLVED
FIXED
Firefox 54
People
(Reporter: magicp.jp, Assigned: njfox)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(2 files)
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.
Blocks: memory-frontend
Updated•7 years ago
|
Keywords: good-first-bug
Whiteboard: [lang=js
Updated•7 years ago
|
Whiteboard: [lang=js → [lang=js]
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Comment 1•7 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) |
Updated•7 years ago
|
Attachment #8842950 -
Flags: review?(nfitzgerald) → review?(ntim.bugs)
Comment 3•7 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•7 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•7 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•7 years ago
|
Assignee: nobody → nick
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 7•7 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•7 years ago
|
Keywords: checkin-needed
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/edf005cc0946
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Comment 12•7 years ago
|
||
Per our discussion on IRC, I believe it's fine to wait for 54 on this one.
Flags: needinfo?(nick)
Updated•7 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•