1243695 - Inspector ruleview inplace-editor takes more space than the text it replaces

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Julian Descottes [:jdescottes]]

Attachment: inplace-editor-layout-shifts.gif

This is a minor annoyance, but in the ruleview, editing a selector, a rulename or rulevalue will shift the elements next to the inserted input a few pixels to the right and to the bottom.

As you can see in the attachement when editing the "margin" rulename, the associated value shifts to the right (~10px), and the padding rule shifts 1 px to the bottom.

It would be much nicer if the input used the same space as the span it replaces.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.wip.patch

Simple fix proposal, applying a negative margin on the inplace-editor input.
Removed the additional 10px added to the input's width.

Added a min width of 10px for empty elements.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: inplace-editor-layout-shifts-fix.gif

How it looks like with the patch applied.

Comment 3

[Julian Descottes [:jdescottes]]

Mike : I think you initially added https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#379 (extra 10 pixels to avoid scrolling when we type).

Do you remember the exact issue ? I can't seem to reproduce it, and removing the 10 extra px makes the input swapping smoother.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.wip.patch

(wrong patch!)

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I never changed it... I just moved the file at one time and this line was already there.

If it works better without the padding then feel free to remove it.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.v1.patch

Thanks for the info! 
Could you review the changeset?

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbeea316a70

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8714313 [details] [diff] [review]
bug1243695.v1.patch

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

Can you look into the test failures before I review this?

Comment 8

[Julian Descottes [:jdescottes]]

Sure! Thanks for pinging me. Interestingly, I don't have the failure locally on OSX. 

Looks like the test is clicking 1px too high, and the expected blur is not triggered anymore.
Just updated the test to click at "height / 2".

new try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=5232c0d163fd

I will flag for review again if try is ok.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

You should use EventUtils.synthesizeMouseAtCenter(editorInput, {}, view.styleWindow); and then you don't even need the rect.

I have logged bug 1245484 to add this as an eslint warning.

Oh, don't forget to attach your patch when it is passing.

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> You should use EventUtils.synthesizeMouseAtCenter(editorInput, {},
> view.styleWindow); and then you don't even need the rect.
> 

Here I actually want to click outside of the input, which is why I use rect.width as horizontal offset.

> Oh, don't forget to attach your patch when it is passing.

Yep, waiting for a green try :)

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.v2.patch (fixed test failure on windows)

Fixed the failing test. Try looks ok.

Comment 12

[Julian Descottes [:jdescottes]]

Thanks for the review!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b1f8d16752e5

Comment 14

[Phil Ringnalda (:philor)]

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

Comment 15

[Julian Descottes [:jdescottes]]

After using this for some time, I found one annoying use case :
- click on a rule view property to edit it
- press right arrow

ER : the caret should be visible blinking.
AR : the caret is hidden.

I would like to add 2 extra pixels here, so that the caret is still visible. The negative margin-right for the editor is increased to compensate for this.

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.part2.v1.patch

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=114a3a5fd99c

Mike: sorry for reopening, hope you don't mind reviewing this additional change.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8718766 [details] [diff] [review]
bug1243695.part2.v1.patch

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

r+ as long as try is green.

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: bug1243695.part2.v2.patch

Carry over r+. Try is green.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a09ce178cd1d

Comment 20

[Carsten Book [:Tomcat]]

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