713612 - add closing curly bracket to avoid disrupting the rest of the CSS

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P2)

Description

[Matthew Wein [:mattw]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111226 Firefox/12.0a1
Build ID: 20111226031002

Steps to reproduce:

Opening up a curly bracket when creating a new CSS rule.


Actual results:

the entire page crashes due to the lack of a closing curly bracket


Expected results:

either the Style Editor should add a closing curly bracket, or wait a second or two for me to finishing adding my rules to avoid having the rest of the page crash

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew Wein from comment #0)
> the entire page crashes due to the lack of a closing curly bracket
> 
> Expected results:
> 
> either the Style Editor should add a closing curly bracket, or wait a second
> or two for me to finishing adding my rules to avoid having the rest of the
> page crash

What is meant here is not that the page crashes, but that the styling of the page is sometimes altered to the point of it being unrecognizable until the closing curly bracket is added.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Updated STR:

1. Visit http://www.mozilla.org/projects/firefox/prerelease.html
2. Open the Style Editor
3. In the screen-2010.css file, above the /* {{{ Reset */ comment, add a selector and opening bracket.
4. Wait a second or two for the style to be applied.

Expected results:
Opening the curly bracket should add a matching closing bracket.

Actual results:
Lack of closing bracket causes the rest of the CSS rules to have invalid syntax and as such the styles fail to apply.

Comment 3

[Dave Camp (:dcamp)]

Cedric, didn't the style editor used to automatically close new opening braces?

Comment 4

[Cedric Vivier [:cedricv]]

(In reply to Dave Camp (:dcamp) from comment #3)
> Cedric, didn't the style editor used to automatically close new opening
> braces?

The add-on does. This was especially necessary for automatic transitions.
There is another bug to add this feature in a more generic way in SourceEditor (bug 678980)

Comment 5

[Dave Camp (:dcamp)]

How hard would it be to put in the addon's workaround for now?  I don't think this should depend on a decision about whether closing brackets by default is the right behavior for SourceEditor.

Comment 6

[Cedric Vivier [:cedricv]]

Attachment: StyleEditor-specific bracket completion

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588047 [details] [diff] [review]
StyleEditor-specific bracket completion

+  let pairs = {
+    123/*{*/: {
+      closeString: "}",
+      closeKeyCode: Ci.nsIDOMKeyEvent.DOM_VK_CLOSE_BRACKET
+    },
+    40/*(*/: {
+      closeString: ")",
+      closeKeyCode: Ci.nsIDOMKeyEvent.DOM_VK_0
+    },
+    91/*[*/: {
+      closeString: "]",
+      closeKeyCode: Ci.nsIDOMKeyEvent.DOM_VK_CLOSE_BRACKET
+    },
+  };

Those comments threw me off for a second. The lack of spacing make them difficult to parse at a glance.

Could you move those comments after the { ?

e.g., 123: { // '{'

or similar.

+    //we got a pair to complete, simulate the closing key
       ^ space

I might reword that to:

// We detected an open bracket sending closing character.

actual code looks decent. Does the SourceEditor not provide a convenient mechanism to insert a key or string like this? Might make sense to add this there for reuse.

Could be done in a followup bug.

Comment 8

[Cedric Vivier [:cedricv]]

Attachment: patch v1.1 - address review comments.

Comment 9

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> Does the SourceEditor not provide a convenient
> mechanism to insert a key or string like this? Might make sense to add this
> there for reuse.

Bracket completion is a bit more specific than a plain text insert, needs to be added to the current undo stack as if the user pressed the key himself, and the caret offset needs to be adjusted so that the insertion is not intrusive.

We have bug 678980 open for a more generic implementation of this feature in SourceEditor.

Comment 10

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/006eb238ac90

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/006eb238ac90

Comment 12

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
20120328051619