Closed Bug 1451690 Opened 6 years ago Closed 6 years ago

Inspector splitter is not following the mouse in RTL

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(1 file)

STRs:

- in about:config set intl.uidirection to 1
- open inspector
- try to use the splitter between the markup view and the side panels

The splitter moves in the wrong direction compared to the mouse movement.

The splitter used by the inspector is the DevTools SplitBox component [1], which normally adapts to the document's dir property [2]

The inspector document has dir="rtl", but we are loading the SplitBox using the browserRequire created for the whole toolbox [3]. In the context, the global "document" points is the toolbox one, which doesn't have dir="rtl" automatically set.

Interestingly, the animation inspector (another consumer of SplitBox) creates a dedicated browserRequire to load this component [4]. Our options here are:
- use a dedicated browserRequire for the SplitBox in the inspector
- modify SplitBox to pass it a document
- set dir="rtl" on the toolbox document

[1] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/inspector/inspector.js#465-505
[2] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/shared/components/splitter/SplitBox.js#162-164
[3] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/inspector/inspector.js#438-440
[4] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/animationinspector/components/animation-timeline.js#82-92
Comment on attachment 8965271 [details]
Bug 1451690 - Use SplitBox owner document to check for dir = rtl;

https://reviewboard.mozilla.org/r/233992/#review239794

Needs to be rebased as well

::: devtools/client/shared/components/splitter/SplitBox.js:162
(Diff revision 1)
>      let { endPanelControl } = this.props;
>  
>      if (this.state.vert) {
> +      // Use the document owning the SplitBox to detect rtl. The global document might be
> +      // the one bound to the toolbox shared BrowserRequire, which is irrelevant here.
> +      const splitBox = ReactDOM.findDOMNode(this);

We already have the splitBox node from line 154. We can probably remove this variable and do node.ownerDocument
Attachment #8965271 - Flags: review?(gl) → review+
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8965271 [details]
Bug 1451690 - Use SplitBox owner document to check for dir = rtl;

https://reviewboard.mozilla.org/r/233992/#review239794

> We already have the splitBox node from line 154. We can probably remove this variable and do node.ownerDocument

Ah good catch, thanks!
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b401fc737a4f
Use SplitBox owner document to check for dir = rtl;r=gl
https://hg.mozilla.org/mozilla-central/rev/b401fc737a4f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.