Closed Bug 1143742 Opened 9 years ago Closed 8 years ago

Please use a multiline editor to edit long CSS values

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: julienw, Assigned: jdescottes)

References

(Depends on 1 open bug)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(5 files)

STR:
1. try to edit a value for the "background" property in the CSS panel in the Inspector tab.

The value is displayed in a multiline way, but when we edit it, it's converted in a one-line input editor which is really painful to use if the value is bigger than the available space.
Yes, we need this! Thanks for filing.
I've recently been playing around with multiple backgrounds (including gradients) and it's really a pain to edit in the rule-view.

Safari somewhat recently switched to using codemirror editors for each and every rule, making it possible to edit text in a more comfortable manner. I guess this bug is one reason to move towards this solution too.
From bug 1223067:

STR:   (Win7_64, Nightly 45, 32bit, ID 20151108030417, new profile)
1. Open new tab
2. Open devtools -> Inspector, select <body> in markup-view, open ruleview tab in sidebar
3. Click "element { }" box to start creating new rule
4. Paste there (Ctrl+V) the following string:
>   background: transparent url("https://mozorg.cdn.mozilla.net/media/img/firefox/firstrun/logo-nightly.cafba4805593.png#its_possible_that_web_pages_use_long_urls_and_dont_forget_about_data_urls") no-repeat scroll 50% 30px;

Result:
 [see screenshot] all rules are moved to the left, UI doesn't look good

Expectations:
 Other rules shouldn't move anywhere = should be stationary
I was recently thinking about this and realized that we could perhaps use contenteditable to support inline editing of selectors, names and values that works with wrapping lines.
I quickly put this together to demonstrate what I had in mind: http://jsbin.com/bisahaqote/edit?html,css,output
Patrick, I think using [contenteditable] is a very bad idea currently. It can cause unexpected behavior (text editing, caret moving, etc) that will stay for a long time.
Core::Editor component has no owner, and there're many bugs related to [contenteditable] which will never(?) be fixed. Even after bug 1153237 is fixed, I found a bug in your testcase w/o any effort.

However, if you think that it will only speed up the work on [contenteditable] bugs, this may still be a good idea. This will definitely improve UX on a number of sites, e.g. Facebook, vk.com, GMail.
We used contenteditable in Gaia::SMS and we fixed some bugs in the process (but yes, there are still bugs, and especially pasting needs careful testing).
(In reply to arni2033 from comment #5)
> Patrick, I think using [contenteditable] is a very bad idea currently. It
> can cause unexpected behavior (text editing, caret moving, etc) that will
> stay for a long time.
> Core::Editor component has no owner, and there're many bugs related to
> [contenteditable] which will never(?) be fixed.
That's a good point. I have noticed some bugs too. But it's just so convenient for a nice multi-line editing widget that I thought I'd give this a try.
> Even after bug 1153237 is
> fixed, I found a bug in your testcase w/o any effort.
Oh, yeah, I'm sure you did. I hacked this together in like 5 minutes. This was merely to visually convey an idea. I'm sure there are many bugs, but it looks to me like even without an owner, we might be able to fix bugs in contenteditable, or work around others.

(In reply to Julien Wajsberg [:julienw] from comment #6)
> We used contenteditable in Gaia::SMS and we fixed some bugs in the process
> (but yes, there are still bugs, and especially pasting needs careful
> testing).
Thanks, then we can leverage this experience if we do decide to use contenteditable too.
Filter on CLIMBING SHOES
OS: Linux → Unspecified
Priority: -- → P2
Hardware: x86_64 → Unspecified
Whiteboard: [btpp-fix-later]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
In preparation for using a multiline editor for property values, tests
need to be updated :
- some tests used the "input" selector, which will no longer work with a
  textarea
- some tests are relying on EventUtils.sendChar to send keys such as
  "VK_RETURN". Doing so also applies the shift key modifier, which has
  a specific behavior with multiline editors

Review commit: https://reviewboard.mozilla.org/r/37881/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37881/
Attachment #8726259 - Flags: review?(gl)
The inplaceEditor now supports a maxWidth configuration option which can either
be a number or a method returning a number. This maxWidth will be applied to the
hidden element used in order to autosize the input.

Review commit: https://reviewboard.mozilla.org/r/37883/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37883/
Attachment #8726260 - Flags: review?(gl)
The default inplace-editor autocomplete behavior is not userfriendly
when combined with a multiline inplace-editor. Navigating up/down might
trigger an autocomplete suggestion.

Also, the autocomplete popup is not displayed at the correct position and
should take the multiline into account.

Review commit: https://reviewboard.mozilla.org/r/37885/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37885/
Attachment #8726261 - Flags: review?(gl)
Comment on attachment 8726259 [details]
MozReview Request: Bug 1143742 - part1: multiline inplace editor: cleanup existing tests;r=gl

https://reviewboard.mozilla.org/r/37881/#review34623
Attachment #8726259 - Flags: review?(gl) → review+
I haven't fully reviewed part 2 and 3, and have been playing around with it. I think the width of the input for single line properties are a bit too wide, and would prefer the width be consistent with what we have now. I will add a screenshot to illustrate. Overall, I like the multiline editor.

Considering the timing of this, I am wondering if we should prioritize landing this after or before the merge. It is real close to the merge and would not provide us the necessary window to fix any bugs that might show up afterwards.
Attached image Screen Shot
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #13)
> I haven't fully reviewed part 2 and 3, and have been playing around with it.
> I think the width of the input for single line properties are a bit too
> wide, and would prefer the width be consistent with what we have now. 

I see what you mean. There is a scary extra 15px added when we use a multiline inplace-editor [1]. I need to figure out if this is really needed.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#378

> Considering the timing of this, I am wondering if we should prioritize
> landing this after or before the merge. It is real close to the merge and
> would not provide us the necessary window to fix any bugs that might show up
> afterwards.

Good point! IMO it would be better to land after the merge. It impacts the inplace-editor, so it could be risky. Uplift is always an option if things go really well.
Comment on attachment 8726259 [details]
MozReview Request: Bug 1143742 - part1: multiline inplace editor: cleanup existing tests;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37881/diff/1-2/
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/1-2/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/1-2/
Blocks: 1155465
Comment on attachment 8726259 [details]
MozReview Request: Bug 1143742 - part1: multiline inplace editor: cleanup existing tests;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37881/diff/2-3/
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/2-3/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/2-3/
(uploaded rebased commits)
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

https://reviewboard.mozilla.org/r/37883/#review38049

::: devtools/client/shared/inplace-editor.js:398
(Diff revision 3)
>    _updateSize: function() {
>      // Replace spaces with non-breaking spaces.  Otherwise setting
>      // the span's textContent will collapse spaces and the measurement
>      // will be wrong.
> -    this._measurement.textContent = this.input.value.replace(/ /g, "\u00a0");
> +    let content = this.input.value;
>  

I don't see any reason to have a blank between content and unbreakableSpace. Let's remove it.

::: devtools/client/shared/inplace-editor.js:406
(Diff revision 3)
> -      // unless there's text content on the line.
> +      content = unbreakableSpace;
> -      width += 15;
> -      this.input.style.height = this._measurement.offsetHeight + "px";
>      }
>  
> -    if (width === 0) {
> +    // If content ends width a new line, add a blank space to force the autosize

Spelling error, I assume width should be with here.

::: devtools/client/shared/inplace-editor.js:429
(Diff revision 3)
> -    }
> +      }
> +      let height = this._measurement.getBoundingClientRect().height;
> +      this.input.style.height = height + "px";
> +    }
> +    this.input.style.overflow = "hidden";
> +    this.input.style.width = width + "px";

So, I am seeing that the toggling into the inplace-editor mode would shrink the existing width and pull in the next character towards the inplace-editor.
For instance, click on the property value and you will see the ";" move inward. Ideally, we would not have that behavior. I think adding 2px to the width might fix this, and try to preserve the existing single line input behaviour.

::: devtools/client/shared/inplace-editor.js:1342
(Diff revision 3)
> -  to.style.fontStyle = style.getPropertyCSSValue("font-style").cssText;
> +  to.style.fontSize = getCssText("font-size");
> +  to.style.fontWeight = getCssText("font-weight");
> +  to.style.fontStyle = getCssText("font-style");
> +  to.style.lineHeight = getCssText("line-height");
> +
> +  if (getCssText("box-sizing") === "border-box") {

Let's avoid calling getCssText("box-sizing") twice by assigning it to a value.
Attachment #8726260 - Flags: review?(gl) → review+
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

https://reviewboard.mozilla.org/r/37885/#review38067

I am noticing a weird behaviour if we simply press space continuously on a new property value editor with autocompletion. At some point after moving to a second line, we have a weird state where we flip between 1 or 2 lined textarea.

::: devtools/client/shared/inplace-editor.js:992
(Diff revision 3)
>          event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RIGHT) {
>        if (this.popup && this.popup.isOpen) {
>          this.popup.hidePopup();
>        }
>      } else if (!cycling && !event.metaKey && !event.altKey && !event.ctrlKey) {
> +      let hasSingleLine =

I prefer this to be named isSingleLine
Attachment #8726261 - Flags: review?(gl)
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/3-4/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/3-4/
Attachment #8726261 - Flags: review?(gl)
When opening the context menu (and displaying the "copy" menu entry)
or when receiving a copy event, we now check if the current target
is a textarea. (multiline inplace editor relies on textarea)

Modified existing test to check this new use case.

Review commit: https://reviewboard.mozilla.org/r/41763/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41763/
Attachment #8733408 - Flags: review?(gl)
https://reviewboard.mozilla.org/r/37883/#review38049

> I don't see any reason to have a blank between content and unbreakableSpace. Let's remove it.

Right! Removed.

> Spelling error, I assume width should be with here.

Good catch, fixed punctuation at the same time.

> So, I am seeing that the toggling into the inplace-editor mode would shrink the existing width and pull in the next character towards the inplace-editor.
> For instance, click on the property value and you will see the ";" move inward. Ideally, we would not have that behavior. I think adding 2px to the width might fix this, and try to preserve the existing single line input behaviour.

You were right, adding 2px to the width allows to maintain the "layout" when opening the input.

> Let's avoid calling getCssText("box-sizing") twice by assigning it to a value.

Thanks!
https://reviewboard.mozilla.org/r/37885/#review38067

I decided to simplify a it the appraoch for multiline+autocomplete. For now, the autocompletion is still activated in multiline. If several lines are displayed, we simply stick the autocomplete popup to the bottom left.

Ultimately, we should also try to skip autocompletion (and maybe increment) when using the keyboard arrows, in order to allow the user to navigate in the field easily. Not sure how much autocompletion should be nerfed for now, so I propose to address this in a follow up bug.
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/4-5/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/4-5/
Comment on attachment 8733408 [details]
MozReview Request: Bug 1143742 - part4: add textarea to valid targets when copying;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41763/diff/1-2/
Addressed the failures on Linux test failures from the previous build.

New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b2d08da355
Still some failures on OSX 10.10, which I can't reproduce locally with OSX 10.11.
Also on OSX10.10, browser_markup_textcontent_edit_02 is now failing.
The failures are coming from the extra 15 pixels that I removed.

I launched several tests on try, and the tests fail unless the input has an extra width equal to the char width. I tried adding a delay between sending the key event and measuring the input size, but this still fails. 

I can't reproduce the issue locally by running the tests on OSX 10.11.
I also used the built artifacts on an old OSX 10.9, and the feature seems to work fine. (the machine is too old/slow to actually build firefox, so I can't run the tests there).

I don't think I have any way to run tests under OSX 10.6 or 10.10 for now.
Attachment #8726261 - Flags: review?(gl) → review+
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

https://reviewboard.mozilla.org/r/37885/#review39703

::: devtools/client/shared/inplace-editor.js:1336
(Diff revisions 2 - 5)
>        this.emit("after-suggest");
>        this._doValidation();
>      }, 0);
> +  },
> +
> +  _isSingleLine: function() {

Add a function comment describing the function
Attachment #8733408 - Flags: review?(gl) → review+
Comment on attachment 8733408 [details]
MozReview Request: Bug 1143742 - part4: add textarea to valid targets when copying;r=gl

https://reviewboard.mozilla.org/r/41763/#review39705

::: devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js:142
(Diff revision 2)
> +  EventUtils.synthesizeMouseAtCenter(editor.input,
> +    {button: 2, type: "contextmenu"}, win);
> +  yield onPopup;
> +
> +  ok(!view._contextmenu.menuitemCopy.hidden,
> +    "Copy menu item is not hidden as expected");

Let's say "Copy menu item is displayed as expected". Let's also add a new line to break up the logic blocks between the hidden test and the checkClipboardData test.
Comment on attachment 8726259 [details]
MozReview Request: Bug 1143742 - part1: multiline inplace editor: cleanup existing tests;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37881/diff/3-4/
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/5-6/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/5-6/
Comment on attachment 8733408 [details]
MozReview Request: Bug 1143742 - part4: add textarea to valid targets when copying;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41763/diff/2-3/
https://reviewboard.mozilla.org/r/37885/#review39703

> Add a function comment describing the function

Done, thanks!
https://reviewboard.mozilla.org/r/41763/#review39705

> Let's say "Copy menu item is displayed as expected". Let's also add a new line to break up the logic blocks between the hidden test and the checkClipboardData test.

Done, added a couple of info as well to be consistent with the two other tests in the same file.
Thanks for the reviews Gabriel, now have to fix my OSX try issues!
Comment on attachment 8726259 [details]
MozReview Request: Bug 1143742 - part1: multiline inplace editor: cleanup existing tests;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37881/diff/4-5/
Comment on attachment 8726260 [details]
MozReview Request: Bug 1143742 - part2: multiline inplace-editor should support a maxWidth option;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37883/diff/6-7/
Comment on attachment 8726261 [details]
MozReview Request: Bug 1143742 - part3: multiline inplace-editor autocomplete behavior;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37885/diff/6-7/
Comment on attachment 8733408 [details]
MozReview Request: Bug 1143742 - part4: add textarea to valid targets when copying;r=gl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41763/diff/3-4/
I think this last version should fix the OSX try issues. By doing step by step screenshots of the test, I could see that the input size was correct but it still has created scrollbars. What most probably happened is that the browser created the scrollbar when inserting the character, before we could resize the input. 

I think the best here is to set overflow:hidden on multiline inputs. We don't allow scrollbars in those inputs anyway.

Try pushes : 
- osx 10.6 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b19402f72db
- others : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ebd6d01ae3b
Depends on: 1261827
I have reproduced this bug with Nightly 39.0a1(2015/03/16) on Windows 10, 64 bit!

The Bug's fix is now verified Beta 48.0b9

Build ID 	20160718142219
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[bugday-20160727]
See Also: → 1326652
Depends on: 1327755
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.