Closed Bug 1243695 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect)

46 Branch
defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(3 files, 5 obsolete files)

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.
Attached patch bug1243695.wip.patch (obsolete) — Splinter Review
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.
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)
Attached patch bug1243695.wip.patch (obsolete) — Splinter Review
(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
Attached patch bug1243695.v1.patch (obsolete) — Splinter Review
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 :)
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
https://hg.mozilla.org/mozilla-central/rev/b1f8d16752e5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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 → ---
Attached patch bug1243695.part2.v1.patch (obsolete) — Splinter Review
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+
Carry over r+. Try is green.
Attachment #8718766 - Attachment is obsolete: true
Attachment #8719336 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a09ce178cd1d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: