1502003 - [markup-view] Make transition to styleinspector-propertyeditor less jumpy

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Florens Verschelde :fvsch]

When clicking an element name, attribute or attribute value to edit it, the textarea that pops up takes more space than the original display, making content jump around a few pixels:

- The line height increases, making every row below shift down.
- The content on the right shifts to the right a few pixels, and in some cases some content might shift to the next line.

For long attribute values that already span several lines this is hard to prevent, but for simpler case it should feel more transparent. I tried a fix briefly in CSS and it seems manageable.

Places where this issue is managed nicely:
- The Rules view.
- Chrome DevTools's markup view.

Comment 1

[Florens Verschelde :fvsch]

Attachment: bug-1502003-markup-view-styleinspector-propertyeditor-width.patch

Small patch to start discussing this.

This is based on top of the work in bug 1501997 for vertical alignment; the way it manages width is independent.

So it looks like the same JS code is used to give a width to the inline editor in the markup view and in the Rules view (maybe in the Layout>Box Model part too?). We're hardcoding a +2px width for some reason, and I'd be curious to know why, because:

- In the markup view, it results in extra empty space on the right of the editor (which makes content jump around when we insert the editor)
- In the Rules view, we have to cancel those 2px with a negative margin (margin: -1px -3px -1px -1px; where the first 1px is for the border, because this stylesheet uses box-sizing:border-box).

It looks like removing the hardcoded +2px in JS works well.
If we want more flexibility we could avoid setting width/max-width in JS, and set this value as a custom property (--original-width) that we can then use in CSS as we see fit (e.g. `width: calc(var(--original-width) + 4px);`), but right now I haven’t seen the need for this kind of refactor.

Remaining work that I'm aware of:
1. This more integrated style (with negative margins in the markup view) is fine for most cases, but looks bad for the "Add an attribute" editor. I filed another bug on this topic, which we could merge with this one: bug 1501959.
2. Since we’re working on the inline editor styles, do we want to evolve its style for the dark theme? White text, dark/black background, blue border?

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 9020702 [details] [diff] [review]
bug-1502003-markup-view-styleinspector-propertyeditor-width.patch

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

The +2px seems to come from this change: https://hg.mozilla.org/mozilla-central/diff/02c981b1fb55/devtools/client/shared/inplace-editor.js
Or, at least the code for this was modified then. Now there seems to have been a case where +2px was added before too, and the comment for it said "Add 2 pixels to ensure the caret will be visible".
I'm going to F? Julian who worked on this at the time, maybe he can help more than I can.
Now, your code changes do look ok to me though.

Comment 3

[Florens Verschelde :fvsch]

That's interesting, I'll have to check how my patch behaves without this extra space.

I'm thinking maybe we can reserve 2px of content space on the right, and 2px of padding-left, to get a visually balanced look.
I could try moving the width to a custom property so we can adjust that on the CSS side.

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 9020702 [details] [diff] [review]
bug-1502003-markup-view-styleinspector-propertyeditor-width.patch

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

The initial bug that introduced this was a first attempt at making the whole inplace editing less jumpy.

The bug was https://bugzilla.mozilla.org/show_bug.cgi?id=1243695
Initially I didn't have the +2px but reopened the bug and added a changeset to add it because I couldn't see the caret:
https://hg.mozilla.org/mozilla-central/rev/a09ce178cd1d

I added a negative margin to compensate for it, but I was really focused on the rule-view, and not on the markup-view.
I didn't realize this was not working for the markup-view correctly.

If you look at the GIFs from the initial bug you can see that the style of the inputs was different. 
The caret could be completely hidden when if you moved it completely to the right. 
Now it looks like the style of the inputs have changed. The border is dotted, and you can still the caret when it is on the right with your patch.

So the change seems fine to me, and I am really happy to have seamless transitions!
Thanks for working on this.

Maybe :pbro can confirm this is not an issue on windows either?

Comment 5

[Julian Descottes [:jdescottes]]

See question above.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: rule-view-jumpy.gif

Quick test on windows, with the rule-view.
I can see some slight shifting down when cycling through the properties in the above rule.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: markup-view-jumpy.gif

The markup-view seems to behave perfectly. No jumping around. I can also see the caret to the right of the field.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #6)
> Created attachment 9022850 [details]
> rule-view-jumpy.gif
> 
> Quick test on windows, with the rule-view.
> I can see some slight shifting down when cycling through the properties in
> the above rule.

I assume the shift already occurs without the patch?
(note that the big shift to the left occurs for all properties with toggle icon)

Comment 9

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.