Last Comment Bug 713612 - add closing curly bracket to avoid disrupting the rest of the CSS
: add closing curly bracket to avoid disrupting the rest of the CSS
Status: VERIFIED FIXED
[styleeditor][qa!]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 12
Assigned To: Cedric Vivier [:cedricv]
:
Mentors:
http://www.mozilla.org/projects/firef...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-26 23:57 PST by Matthew Wein [:K-9, :mattw]
Modified: 2012-04-02 06:37 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
StyleEditor-specific bracket completion (4.85 KB, patch)
2012-01-12 08:42 PST, Cedric Vivier [:cedricv]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.1 - address review comments. (4.79 KB, patch)
2012-01-25 03:04 PST, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review

Description Matthew Wein [:K-9, :mattw] 2011-12-26 23:57:09 PST
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) 2011-12-27 00:02:14 PST
(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) 2011-12-27 00:12:15 PST
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) 2012-01-04 15:05:27 PST
Cedric, didn't the style editor used to automatically close new opening braces?
Comment 4 Cedric Vivier [:cedricv] 2012-01-09 00:38:59 PST
(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) 2012-01-10 08:55:26 PST
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] 2012-01-12 08:42:55 PST
Created attachment 588047 [details] [diff] [review]
StyleEditor-specific bracket completion
Comment 7 Rob Campbell [:rc] (:robcee) 2012-01-23 07:07:20 PST
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] 2012-01-25 03:04:10 PST
Created attachment 591401 [details] [diff] [review]
patch v1.1 - address review comments.
Comment 9 Cedric Vivier [:cedricv] 2012-01-25 03:07:22 PST
(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 11 Tim Taubert [:ttaubert] 2012-01-25 09:04:39 PST
https://hg.mozilla.org/mozilla-central/rev/006eb238ac90
Comment 12 Ioana (away) 2012-04-02 06:37:17 PDT
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

Note You need to log in before you can comment on or make changes to this bug.