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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

46 Branch
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Created attachment 8713120 [details]
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.
Created attachment 8713121 [details] [diff] [review]
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.
Created attachment 8713122 [details]
inplace-editor-layout-shifts-fix.gif

How it looks like with the patch applied.
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.
Flags: needinfo?(mratcliffe)
Created attachment 8713125 [details] [diff] [review]
bug1243695.wip.patch

(wrong patch!)
Attachment #8713121 - Attachment is obsolete: true
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.
Flags: needinfo?(mratcliffe)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Created attachment 8714313 [details] [diff] [review]
bug1243695.v1.patch

Thanks for the info! 
Could you review the changeset?

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbeea316a70
Attachment #8713125 - Attachment is obsolete: true
Attachment #8714313 - Flags: review?(mratcliffe)
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?
Attachment #8714313 - Flags: review?(mratcliffe)
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.
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.
(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 :)
Created attachment 8715792 [details] [diff] [review]
bug1243695.v2.patch (fixed test failure on windows)

Fixed the failing test. Try looks ok.
Attachment #8714313 - Attachment is obsolete: true
Attachment #8715792 - Flags: review?(mratcliffe)
Attachment #8715792 - Flags: review?(mratcliffe) → review+
Thanks for the review!
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/b1f8d16752e5
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1f8d16752e5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8718766 [details] [diff] [review]
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.
Attachment #8715792 - Attachment is obsolete: true
Attachment #8718766 - Flags: review?(mratcliffe)
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.
Attachment #8718766 - Flags: review?(mratcliffe) → review+
Created attachment 8719336 [details] [diff] [review]
bug1243695.part2.v2.patch

Carry over r+. Try is green.
Attachment #8718766 - Attachment is obsolete: true
Attachment #8719336 - Flags: review+
Keywords: checkin-needed

Comment 19

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a09ce178cd1d
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a09ce178cd1d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.