Closed Bug 1315667 Opened 4 years ago Closed 4 years ago

Offset resizing the inspector sidebar when zoomed in

Categories

(DevTools :: Inspector, defect, P2)

52 Branch
defect

Tracking

(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: pbro, Assigned: sblin)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

STR:
- open the inspector on any page,
- click somewhere inside the inspector panel so it gets focused
- zoom in a few levels (I did ctrl+ 3 times)
- try to move the splitter between the markup-view and the rule-view

==> There's an offset between the mouse position and the splitter.

This is a recent regression. It works fine for me in 51 and fails in 52 (2016-10-20).

I ran mozregression and got this:

Last good revision: bf78a4c31c1c85b17006883f47282bc6c40fed82
First bad revision: 36373fbb78111d4fe0af105056d5066d1a669739
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=bf78a4c31c1c85b17006883f47282bc6c40fed82&tochange=36373fbb78111d4fe0af105056d5066d1a669739

So this could be due to bug 1262443, bug 1303390 or bug 1260552.
I think bug 1260552 is the most likely candidate.
Version: unspecified → 52 Branch
Hi,

I can reproduce the bug. I'd like to work on this bug.
Thank you Sébastien. I'll ping Honza who should be able to help you get started I think.
Flags: needinfo?(odvarko)
> I'd like to work on this bug.
Great!

The splitter is implemented as a React component here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/splitter/split-box.js

The problem is most likely somewhere in the onMove() method that represents an event handler for  'mousemove' events. These events are generated as the user is dragging/moving the splitter box.
It looks like the coordinates calculation is wrong.

Note that x and y function arguments are set to event.screenX and event.screenY (see draggable.js file in the same directory). This could be somehow involved too.

I am not sure what is your experience with contributing to DevTools, but the following MDN page might be useful for you:

https://developer.mozilla.org/en-US/docs/Tools/Contributing

Feel free to ask for more help!

Honza
Flags: needinfo?(odvarko)
Attached patch 1315667_1.patch (obsolete) — Splinter Review
Hi,

Thanks for the info. I think this is because ev.screenY return a value in "real px" and the size is in css pixels.
Attachment #8809916 - Flags: review?(pbrosset)
Comment on attachment 8809916 [details] [diff] [review]
1315667_1.patch

Review of attachment 8809916 [details] [diff] [review]:
-----------------------------------------------------------------

Tranferring to Honza who's a better reviewer than me on this specific part of the code. Also, we should keep in mind that this file is shared with the debugger.html project on github. So some sync'ing is required I guess.
Attachment #8809916 - Flags: review?(pbrosset) → review?(odvarko)
Comment on attachment 8809916 [details] [diff] [review]
1315667_1.patch

Review of attachment 8809916 [details] [diff] [review]:
-----------------------------------------------------------------

The splitter is now broken for me. I can't change width of the sidebar in the Inspector panel.
Note that I am on Win10

Also, I recall that using screen coordinates was intentional and the reason was iframes. In case where the mouse is hovering onver an iframe (during splitter-dragging), the coordinates where wrong since being relative to the iframe (see also the comment in Draggable.onMove() method. This should be tested too with a new patch.

Honza
Attachment #8809916 - Flags: review?(odvarko) → review-
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Comment on attachment 8809916 [details] [diff] [review]
> 1315667_1.patch
> 
> Review of attachment 8809916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The splitter is now broken for me. I can't change width of the sidebar in
> the Inspector panel.
> Note that I am on Win10
> 
> Also, I recall that using screen coordinates was intentional and the reason
> was iframes. In case where the mouse is hovering onver an iframe (during
> splitter-dragging), the coordinates where wrong since being relative to the
> iframe (see also the comment in Draggable.onMove() method. This should be
> tested too with a new patch.
> 
> Honza

Erf sorry for the delay. I have a lot of work.

Thx for the info. I'll work on this bug tomorrow I hope. By the way it's strange that ev.clientX doesn't work on win10.
Duplicate of this bug: 1318511
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Attached patch 1315667_1.patch (obsolete) — Splinter Review
Now, I have some time to work on this.
Attachment #8809916 - Attachment is obsolete: true
Attachment #8812375 - Flags: review?(odvarko)
Seems like you're working on this so assigning to you. Thanks!
Assignee: nobody → amarok
Comment on attachment 8812375 [details] [diff] [review]
1315667_1.patch

Review of attachment 8812375 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update!

Unfortunately the patch doesn't work for me. Note that I am testing on Win10.

It seems correct to utilize `win.devicePixelRatio`, but the result splitter position is still wrong.
It might be that recalculating only `win.mozInnerScreenX` isn't enough,
what about e.g. `node.offsetWidth`? - its value also changes when zoom is applied.

Honza

Honza
Attachment #8812375 - Flags: review?(odvarko)
Attached patch 1315667_1.patch (obsolete) — Splinter Review
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> It might be that recalculating only `win.mozInnerScreenX` isn't enough,
> what about e.g. `node.offsetWidth`? - its value also changes when zoom is
> applied.

Oh my bad, we need to get the value in css pixel so node.offsetX is correct. We just need to change x and y (in the previous patch I changed mozInnerScreenX but mozInnerScreenX is also correct).

The patch might works now (but I've got an error with eslint (window not defined) but window is a global var)

(I test on linux (Fedora))
Attachment #8812375 - Attachment is obsolete: true
Attachment #8814208 - Flags: review?(odvarko)
Attached patch 1315667_1.patch (obsolete) — Splinter Review
It's better without the eslint error
Attachment #8814208 - Attachment is obsolete: true
Attachment #8814208 - Flags: review?(odvarko)
Attachment #8814210 - Flags: review?(odvarko)
Comment on attachment 8814210 [details] [diff] [review]
1315667_1.patch

Review of attachment 8814210 [details] [diff] [review]:
-----------------------------------------------------------------

Works great to me now, thanks!

@Jason, does this patch work for Debugger.html?

Honza
Attachment #8814210 - Flags: feedback?(jlaster)
Happy to review tomorrow.

One small nit, ratio can be `ration = win.devicePixelRatio || 1` which is slightly more concise
Hmm, strange I applied this patch https://gist.github.com/jasonLaster/0743b5e11ff495b99de3c2f72b22d505 

and saw an issue with how it calculated sizes. http://g.recordit.co/zKPjiln0Oh.gif

This could be an issue with our use of screenX as well.
Attached patch 1315667_1.patchSplinter Review
Thx for the nit.

In your patch screenX() is in css or in "real" pixels ? if screenX() is in real pixels, you should divides screenX by ratio as well to convert in css pixels.
Attachment #8814210 - Attachment is obsolete: true
Attachment #8814210 - Flags: review?(odvarko)
Attachment #8814210 - Flags: feedback?(jlaster)
Flags: needinfo?(jlaster)
thanks
Flags: needinfo?(jlaster)
So it's working?
Attachment #8815293 - Flags: review?(odvarko)
Comment on attachment 8815293 [details] [diff] [review]
1315667_1.patch

Review of attachment 8815293 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me and I've also got positive feedback from Jason.

R+

Thanks for the patch Sébastien!

Honza
Attachment #8815293 - Flags: review?(odvarko) → review+
Does this just need to land?
Flags: needinfo?(amarok)
Yeap! (sorry for the delay btw)
Flags: needinfo?(amarok)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c4f310d039
Fix resizing inspector sidebar when zooming. r=Honza
https://hg.mozilla.org/mozilla-central/rev/01c4f310d039
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1328895
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(amarok)
Sure, but how do I request Aurora approval?
Flags: needinfo?(ryanvm)
On the attached patch, set approval‑mozilla‑aurora to '?' and answer the questions that appear in the comment field.
Flags: needinfo?(ryanvm)
Sorry for the delay. I will do this as soon as bug #1328895 is fixed.
Attachment #8815293 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarok)
The questions didn't appears in the comment section, so:
Approval Request Comment
[Feature/regressing bug #]: Bug 1260552
[User impact if declined]: Offset resizing when zoomed is broken in devtools.
[Risks and why]: Offset resizing will be broken on high DPI monitors. But it's fixed with https://bugzilla.mozilla.org/show_bug.cgi?id=1328895
[String/UUID change made/needed]: none
Should I be expecting an uplift request for bug 1328895 then?
Flags: needinfo?(amarok)
(In reply to Julien Cristau [:jcristau] from comment #31)
> Should I be expecting an uplift request for bug 1328895 then?

Yes! I was not sure how this should be handled here, but I'll request an uplift on bug 1328895 right now. Both bugs should really be uplifted together.
Comment on attachment 8815293 [details] [diff] [review]
1315667_1.patch

devtools splitter fix for aurora52
Attachment #8815293 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(amarok)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.