1290086 - Width of Inline Property Editor should be not shrink to tiny size

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jens]

Attachment: 2016-07-28 15_00_54-CodePen - firefox devtools_ ux Try editing the css class name - Firefox Develope.png

STR:

I want to edit or select a css class name that a bit longer than usual:

1. Go to the example page that I prepared (this is a stripped down minimal page from a real world ecommerce system): http://codepen.io/nachtigall/full/yJEwZp/ 
2. Search for class="woocommerce-MyAccount-navigation-link woocommerce-MyAccount-navigation-link--dashboard is-active"
3. Double click on this

AR:
The input box shrinks although there are no space constrains. This changes the context and I need to find where the string I was looking for went. Seems like the <input class="styleinspector-propertyeditor"> has a fixed width of 237px here, way to small value. 

ER:
Have the edit input box the same size as when not edited. That is, big enough to view the whole class names.

See screenshot.

Comment 1

[Julian Descottes [:jdescottes]]

Probably a regression from Bug 1143742. 

The autocomplete uses a hidden element to measure the size of its input. When using a single-line inplace editor, the measurement element is positioned in absolute and can be constrained by its container.

We should either measure the size before hiding the reference element, or use a multiline inplace editor.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1290086 - use multiline inplace editor for markupview autocompletes;

Review commit: https://reviewboard.mozilla.org/r/69270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69270/

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8777804 [details]
Bug 1290086 - use multiline inplace editor for markupview autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69270/diff/1-2/

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8777804 [details]
Bug 1290086 - use multiline inplace editor for markupview autocompletes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69270/diff/2-3/

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8777804 [details]
Bug 1290086 - use multiline inplace editor for markupview autocompletes;

https://reviewboard.mozilla.org/r/69270/#review67040

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_long-classname.js:17
(Diff revision 3)
> +const TEST_URL = `data:text/html;charset=utf8, <div class="${classname}"></div>`;
> +
> +add_task(function* () {
> +  let {inspector} = yield openInspectorForURL(TEST_URL);
> +
> +  info("Select node and make sure it is focused");

Redundant info() considering it precedes selectNode()

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the review! Try is green : https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc84dcf5afaa. Landing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/46dede080ba1
use multiline inplace editor for markupview autocompletes;r=gl

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46dede080ba1

Comment 9

[Jens]

Tested on Nightly 51. Verified fixed. Great. Thank you!

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Julian, maybe we want to uplift that to aurora?

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: bug1290086.aurora.patch

Sure! Here is a rebased version that can be applied on top of aurora. 

(no actual code change, except that the patch didn't apply cleanly, carrying over r+)

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8783986 [details] [diff] [review]
bug1290086.aurora.patch

Approval Request Comment
[Feature/regressing bug #]:Bug 1143742
[User impact if declined]:bad user experience when editing long classnames in devtools
[Describe test coverage new/current, TreeHerder]: new mochitest covering the regression : devtools/client/inspector/markup/test/browser_markup_tag_edit_long-classname.js, try https://treeherder.mozilla.org/#/jobs?repo=try&revision=b53124ece64c 
[Risks and why]: low risk. Has been on central for some time, covered by tests. The new code is updating the configuration of 3 autocomplete widgets to use a feature that landed in 48.
[String/UUID change made/needed]:N/A

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8783986 [details] [diff] [review]
bug1290086.aurora.patch

Fix was verified on Nightly, Aurora50+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40076409f9b5

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/40076409f9b5

There's a markup.js.rej in mozilla-beta right now from this cset.

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: bug1290086.beta.followup.patch

My bad, I didn't see the .rej was part of the changeset, sorry :( 
Here is a patch to remove it in case you want to cleanup beta.

Approval Request Comment
[Feature/regressing bug #]:N/A (follow up)
[User impact if declined]: no user impact
[Describe test coverage new/current, TreeHerder]:N/A
[Risks and why]: removing a .rej committed by mistake.
[String/UUID change made/needed]: N/A

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8797150 [details] [diff] [review]
bug1290086.beta.followup.patch

I'll land this a=NPOTB next time I'm pushing there.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6331567bfa91