930680 - Cannot type a semicolon inside a CSS property value

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Zack Weinberg (:zwol)]

If you type a semicolon while editing a CSS property value in the devtools, even if you are syntactically inside a string, unquoted url(), or comment, or immediately after a backslash, the editor forcibly ends the current property-value and starts a new one.

In particular this makes it impossible to type

    background-image:url("data:image/png;base64,

which is a thing one might reasonably want to be able to do.

[I *think* the above are the only situations where a semicolon doesn't signal the end of a property value, but don't quote me on that.]

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This issue appears in the Inspector's Rule View, but the Style Editor (at least the CodeMirror version) appears to be fine.

Comment 2

[Zack Weinberg (:zwol)]

I was unaware that the "Style Editor" was a separate thing from the Inspector capability to modify styles!  Amazing what one can fail to notice.

Comment 3

[Zack Weinberg (:zwol)]

Note also that if you *paste in* a semicolon, this does not happen, which was nice yesterday because it gave me a workaround -- but it just occurred to me to check, and if you paste a string that *really is* several property-value pairs, e.g. 

    color:red;font-family:sans-serif;font-style:italic

it doesn't get split up appropriately.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Zack Weinberg (:zwol) from comment #3)
> if you paste a string that *really is* several
> property-value pairs, e.g. 
> 
>     color:red;font-family:sans-serif;font-style:italic
> 
> it doesn't get split up appropriately.
This part now works since bug 913630.

However, typing a semicolon while in a string value still doesn't work.

Comment 5

[Tom Tromey :tromey]

The bug here is that rule-view.js makes in inplace-editor with ";" as the
"advance character", and this "advance" functionality doesn't let the caller
decide whether or not to actually advance.

In additions to strings (and URLs, which are treated specially depenending
on which version of the CSS spec you read), a ";" can also appear in a CSS comment.

Comment 6

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #5)

> In additions to strings (and URLs, which are treated specially depenending
> on which version of the CSS spec you read), a ";" can also appear in a CSS
> comment.

... which was pointed out already, sorry about that.

Comment 7

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

Here's the patch.  No tests yet.

This relies on the css lexer patch.

Comment 8

[Zack Weinberg (:zwol)]

Typo: "then we all the termination" should probably read "then the semicolon terminates the value".

(I do not know enough about this code to comment on anything else.)

Comment 9

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

Fixed my goofy comment, thanks Zack.

This also makes a change Patrick asked for, namely making the
validator function not be a method of TextPropertyEditor.

Comment 10

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

Updated for changes to the webidl.
Note that this still needs a test case; I will write it soon.

Comment 11

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

I've updated the appropriate inplace-editor comment and added a test.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8601689 [details] [diff] [review]
allow inserting ";" in values in style inspector

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

LGTM.

::: browser/devtools/shared/inplace-editor.js
@@ +76,5 @@
> + *       When an "advance character" is typed, if advanceValidate is
> + *       set, it will be called with the current text and the
> + *       insertion point.  If it returns true, then the focus
> + *       advance takes place.  If it returns false, then the character
> + *       is inserted instead.

I like the approach of providing a validator callback here. One thought: we could make advanceChars be either a string of characters OR a callback.
This would make it possible to only have 1 option instead of 2. Of course if you did that you'd need the callback to also be given the character.

Up to you to change this or not, I'm not going to stop this from landing if you don't, but it'd be nice to not add too many options to this already crowded list.

@@ +940,5 @@
>        prevent = false;
> +    } else if ((aEvent.charCode in this._advanceCharCodes
> +                && (!this.advanceValidate ||
> +                    this.advanceValidate(this.input.value,
> +                                         this.input.selectionStart)))

_onKeyPress is like 200 lines of really complex logic, many branches, and complex conditions to read. Could you factor this condition out in a helper method?

...if (this.isAdvanceCharacter(aEvent.charCode)...

with:

isAdvanceCharacter: function(charCode) {
  let isValid = !this.advanceValidate ||
                this.advanceValidate(this.input.value, this.input.selectionStart);
  return charCode in this.__advanceCharCodes && isValid;
}

::: browser/devtools/shared/test/unit/test_advanceValidate.js
@@ +21,5 @@
> +        "testing _advanceValidate at " + where);
> +}
> +
> +function run_test() {
> +  // Inside a symbol.

Can you change this comment into a print statement so that it's easy to know exactly where the test failed, if it fails (especially when reading logs on a remove server).
do_print("Inside a symbol");
Same thing for the other comments below.

::: browser/devtools/styleinspector/rule-view.js
@@ +3415,5 @@
>  
> +/**
> + * Called when a ";" is typed in a value editor.  This decides
> + * whether to advance or not, by lexing the input and seeing
> + * whether the ";" would be a terminator at this point.

Can you add:
@param {String} aValue The current text editor value.
@param {Number} aInsertionPoint The index where ";" is inserted.
@return {Boolean}

Comment 13

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

This addresses the review comments and changes advanceChars to be
either a string or a function, as suggested.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8604113 [details] [diff] [review]
allow inserting ";" in values in style inspector

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

Thanks for that. Just one thing though: you forgot to pass the text and insertion point to this._advanceChars.
The rest of the interdiff looks fine to me otherwise.

::: browser/devtools/shared/inplace-editor.js
@@ +225,5 @@
> +    let advanceChars = aOptions.advanceChars || '';
> +    for (let i = 0; i < advanceChars.length; i++) {
> +      advanceCharcodes[advanceChars.charCodeAt(i)] = true;
> +    }
> +    this._advanceChars = (aCharCode) => aCharCode in advanceCharcodes;

nit: drop the parens around aCharCode.

@@ +941,5 @@
>      if (this.multiline &&
>          aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RETURN &&
>          aEvent.shiftKey) {
>        prevent = false;
> +    } else if (this._advanceChars(aEvent.charCode)

You should also be passing the text and insertion point as arguments here.

Comment 15

[Tom Tromey :tromey]

Attachment: allow inserting ";" in values in style inspector

Ugh, sorry about that.  This version tests that the parameter is
passed, at least a bit.

I tested it interactively as well.

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49de6490646d

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/6460ad9acd52

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6460ad9acd52

Comment 19

[Saheda Reza [:Antora]]

Buggy : 
User Agent : Mozilla/5.0 (Windows NT 6.3; rv:27.0) Gecko/20100101 Firefox/27.0
Build ID :  20131024030204

Fixed on today's Firefox nightly build :
User Agent : Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID : 20150629134017

Suggesting STATUS -> VERIFIED FIXED