1315667 - Offset resizing the inspector sidebar when zoomed in

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

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

Hi,

I can reproduce the bug. I'd like to work on this bug.

Comment 2

[Patrick Brosset <:pbro>]

Thank you Sébastien. I'll ping Honza who should be able to help you get started I think.

Comment 3

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

> 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

Comment 4

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

Attachment: 1315667_1.patch

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.

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 6

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

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

Comment 7

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

(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.

Comment 9

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 10

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

Attachment: 1315667_1.patch

Now, I have some time to work on this.

Comment 11

[Andrew Overholt [:overholt]]

Seems like you're working on this so assigning to you. Thanks!

Comment 12

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

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

Comment 13

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

Attachment: 1315667_1.patch

(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))

Comment 14

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

Attachment: 1315667_1.patch

It's better without the eslint error

Comment 15

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

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

Comment 16

[Jason Laster [:jlast]]

Happy to review tomorrow.

One small nit, ratio can be `ration = win.devicePixelRatio || 1` which is slightly more concise

Comment 17

[Jason Laster [:jlast]]

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.

Comment 18

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

Attachment: 1315667_1.patch

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.

Comment 19

[Jason Laster [:jlast]]

thanks

Comment 20

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

So it's working?

Comment 21

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

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

Comment 22

[Andrew Overholt [:overholt]]

Does this just need to land?

Comment 23

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

Yeap! (sorry for the delay btw)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c4f310d039
Fix resizing inspector sidebar when zooming. r=Honza

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01c4f310d039

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 27

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

Sure, but how do I request Aurora approval?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

On the attached patch, set approval‑mozilla‑aurora to '?' and answer the questions that appear in the comment field.

Comment 29

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

Sorry for the delay. I will do this as soon as bug #1328895 is fixed.

Comment 30

[Sébastien Blin [:sblin] [: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

Comment 31

[Julien Cristau [:jcristau]]

Should I be expecting an uplift request for bug 1328895 then?

Comment 32

[Julian Descottes [:jdescottes]]

(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 33

[Julien Cristau [:jcristau]]

Comment on attachment 8815293 [details] [diff] [review]
1315667_1.patch

devtools splitter fix for aurora52

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db111a952157