1328895 - high DPI : Issues when resizing the Inspector sidebar

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

Attachment: drag-splitter.mov

[Note]:
- Win 10 64-bit and Ubuntu 16.04 64-bit are not affected

[Affected versions]:
- Nightly 53.0a1 2016-01-05

[Affected platforms]:
- Mac OS X 10.11.5

[Steps to reproduce]:
1. Open Inspector on any page
2. Drag right/left the splitter between the markup-view and the rule-view
3. Dock to side Developer Tools and drag the splitter again

[Expected result]:
- Markup-view and rule-view can be easily splitted 

[Actual result]:
- The rule-view is moved to the left when clicking the splitter and starting to drag to the right/left, then it follows the mouse cursor

[Regression range]:
Last good revision: 1539be3e8e5b74e6a69d880765b2d7166f38599c
First bad revision: 01c4f310d039f0f3b4b08a0e6ce2afb1892f31de
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1539be3e8e5b74e6a69d880765b2d7166f38599c&tochange=01c4f310d039f0f3b4b08a0e6ce2afb1892f31de

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1315667

Comment 1

[Julian Descottes [:jdescottes]]

Happens on all platforms with a high DPI monitor.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

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

Comment 3

[Julian Descottes [:jdescottes]]

Might be missing something but I think we should just use clientX/Y instead of screenX/Y. Those coordinates remain absolute (so not impacted by hovering iframes etc...) but remain consistent with the other metrics when the page zoom changes.

Comment 4

[Sébastien Blin [:sblin] [:amarok]]

I wrote the exact same patch on the previous bug (https://bug1315667.bmoattachments.org/attachment.cgi?id=8809916), but you can read https://bugzilla.mozilla.org/show_bug.cgi?id=1315667#c6

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

https://reviewboard.mozilla.org/r/103670/#review104590

The patch looks good and doesn't break anything when I am testing it. But, the problem is that I can't reproduce the original report using STR from comment #0. Can anyone confirm that the bug is actually fixed before I R+ it?

Honza

::: devtools/client/shared/components/splitter/draggable.js:34
(Diff revision 1)
>  
>    onMove(ev) {
>      ev.preventDefault();
>      // Use screen coordinates so, moving mouse over iframes
>      // doesn't mangle (relative) coordinates.
> -    this.props.onMove(ev.screenX, ev.screenY);
> +    this.props.onMove(ev.clientX, ev.clientY);

Please update also the comment.

Comment 6

[Julian Descottes [:jdescottes]]

Patrick mentioned he could also repro the issue on his high dpi monitor.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

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

Comment 8

[(PTO June 19-July 7) Ryan VanderMeulen]

Setting 52 to affected since we're hoping to uplift bug 1315667 there as well.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: splitter-offset.gif

(In reply to Julian Descottes [:jdescottes] from comment #6)
> Patrick mentioned he could also repro the issue on his high dpi monitor.
Yes I can reproduce on windows 10, with an external 4K monitor.
See attached gif.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: splitter-offset-side.gif

Same thing happens in side-docking mode.

Comment 11

[Patrick Brosset <:pbro>]

I just tested with the patch applied, and the issue I reported in comment 9 and comment 10 appears to be fixed. Thanks!

Comment 12

[Julian Descottes [:jdescottes]]

Thanks for testing Patrick! 

Honza: can you update your review based on comment 9 and comment 11 ?

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

https://reviewboard.mozilla.org/r/103670/#review104896

Ok, great thanks!

Honza

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

https://reviewboard.mozilla.org/r/103670/#review104590

> Please update also the comment.

Fixed thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d45b92c983b5
use clientX/Y instead of screenX/Y when moving devtools splitter;r=Honza

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just removing NI flag

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using latest Nightly 53.0a1 2017-01-13 under Mac OS X 10.11.5.

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1315667
[User impact if declined]: resizing devtools sidebar is buggy on retina screens
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: needs to be uplifted right after Bug 1315667
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple js fix, using page coordinates rather than screen coordinates.
[String changes made/needed]:

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8825548 [details]
Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter;

devtools splitter fix for aurora52

Comment 21

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ae2be9972da