Closed
Bug 1328895
Opened 7 years ago
Closed 7 years ago
high DPI : Issues when resizing the Inspector sidebar
Categories
(DevTools :: Inspector, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 53
People
(Reporter: phorea, Assigned: jdescottes)
References
Details
(Keywords: regression)
Attachments
(4 files)
[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
Assignee | ||
Comment 1•7 years ago
|
||
Happens on all platforms with a high DPI monitor.
OS: Mac OS X → All
Summary: [OSX] Issues when resizing the Inspector sidebar → high DPI : Issues when resizing the Inspector sidebar
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8825548 -
Flags: review?(odvarko)
Assignee | ||
Comment 6•7 years ago
|
||
Patrick mentioned he could also repro the issue on his high dpi monitor.
Flags: needinfo?(pbrosset)
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
Setting 52 to affected since we're hoping to uplift bug 1315667 there as well.
Comment 9•7 years ago
|
||
(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.
Flags: needinfo?(pbrosset)
Comment 10•7 years ago
|
||
Same thing happens in side-docking mode.
Comment 11•7 years ago
|
||
I just tested with the patch applied, and the issue I reported in comment 9 and comment 10 appears to be fixed. Thanks!
Assignee | ||
Comment 12•7 years ago
|
||
Thanks for testing Patrick! Honza: can you update your review based on comment 9 and comment 11 ?
Flags: needinfo?(odvarko)
Comment 13•7 years ago
|
||
mozreview-review |
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
Attachment #8825548 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d45b92c983b5 use clientX/Y instead of screenX/Y when moving devtools splitter;r=Honza
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d45b92c983b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Reporter | ||
Comment 18•7 years ago
|
||
Verified as fixed using latest Nightly 53.0a1 2017-01-13 under Mac OS X 10.11.5.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 19•7 years ago
|
||
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]:
Attachment #8825548 -
Flags: approval-mozilla-aurora?
Comment 20•7 years ago
|
||
Comment on attachment 8825548 [details] Bug 1328895 - use clientX/Y instead of screenX/Y when moving devtools splitter; devtools splitter fix for aurora52
Attachment #8825548 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ae2be9972da
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
status-firefox50:
unaffected → ---
status-firefox51:
unaffected → ---
status-firefox52:
fixed → ---
status-firefox53:
verified → ---
Flags: in-qa-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•