Last Comment Bug 740603 - "Copy Rule" in the rule view includes expanded shorthand properties
: "Copy Rule" in the rule view includes expanded shorthand properties
Status: RESOLVED FIXED
[ruleview][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 10 Branch
: x86 Mac OS X
: P1 normal (vote)
: Firefox 14
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on:
Blocks: 736014 740606
  Show dependency treegraph
 
Reported: 2012-03-29 14:25 PDT by Dave Camp (:dcamp)
Modified: 2012-04-17 10:49 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Made "Copy Rule" more robust (4.88 KB, patch)
2012-03-30 03:29 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
dcamp: review-
Details | Diff | Review
Added test (12.42 KB, patch)
2012-04-04 15:34 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch now includes wait for editor focus and blur (20.38 KB, patch)
2012-04-10 05:17 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Now green on try (20.57 KB, patch)
2012-04-11 14:09 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
dcamp: review+
Details | Diff | Review
Addressed reviewers comments (25.08 KB, patch)
2012-04-16 04:23 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Dave Camp (:dcamp) 2012-03-29 14:25:34 PDT
* Point the inspector at an element with a shorthand property
* Use "Copy Rule" from the context menu
* Paste the resulting text

All the expanded shorthand properties are included.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2012-03-30 03:29:21 PDT
Created attachment 610843 [details] [diff] [review]
Made "Copy Rule" more robust

Seems like this must be an OSX issue only as it works fine on Linux & Windows.

I have made the copy rule & declaration methods a little more robust so it should hopefully fix the problem.
Comment 2 Dave Camp (:dcamp) 2012-04-02 10:01:30 PDT
Comment on attachment 610843 [details] [diff] [review]
Made "Copy Rule" more robust

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

Needs tests for the things that were failing.
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2012-04-04 15:34:14 PDT
Created attachment 612371 [details] [diff] [review]
Added test

Dave, I am asking for feedback from you as discussed.

See:
https://tbpl.mozilla.org/?tree=Try&rev=1c403040a6db

There are timing issues relating to the rule view ... it seems like the DOM is not fully initialized at the time the rule view sends out it's ready notifier.
Comment 4 Dave Camp (:dcamp) 2012-04-04 15:42:21 PDT
Comment on attachment 612371 [details] [diff] [review]
Added test

OK, between synthesizing the mouse click on the property name and working on its editor, you need to wait for the editor to be created.  The browser_ruleview_ui.js test has a method that will wait for an editor to be created, you should be able to use something like that.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2012-04-10 05:17:39 PDT
Created attachment 613558 [details] [diff] [review]
Patch now includes wait for editor focus and blur

This was held up by a hard to find leak (fixed by somebody else somewhere).

Patch works fine locally but need to wait for try:
https://tbpl.mozilla.org/?tree=Try&rev=0f011861fc57
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-04-11 08:14:21 PDT
dcamp: Seems like my latest patch is already attached
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-04-11 14:09:00 PDT
Created attachment 614163 [details] [diff] [review]
Now green on try

Fixed oranges and timing issues. If we can get this into Aurora then we would not need to disable the copy menus.
Comment 8 Dave Camp (:dcamp) 2012-04-13 14:01:47 PDT
Comment on attachment 614163 [details] [diff] [review]
Now green on try

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

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +1021,1 @@
>  

Is it ever possible for node to be null here?  If so, please protect against that.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2012-04-16 04:23:20 PDT
Created attachment 615300 [details] [diff] [review]
Addressed reviewers comments

(In reply to Dave Camp (:dcamp) from comment #8)
> Comment on attachment 614163 [details] [diff] [review]
> Now green on try
> 
> Review of attachment 614163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +1021,1 @@
> >  
> 
> Is it ever possible for node to be null here?  If so, please protect against
> that.
Comment 10 Panos Astithas [:past] 2012-04-17 03:44:59 PDT
https://hg.mozilla.org/integration/fx-team/rev/f1f9579b3eea
Comment 11 Rob Campbell [:rc] (:robcee) 2012-04-17 10:49:12 PDT
https://hg.mozilla.org/mozilla-central/rev/f1f9579b3eea

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