Last Comment Bug 693887 - Implement a rule-centric style view
: Implement a rule-centric style view
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Dave Camp (:dcamp)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 696180 696181 696182 699213 699215
  Show dependency treegraph
 
Reported: 2011-10-11 18:03 PDT by Dave Camp (:dcamp)
Modified: 2011-11-02 13:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rule View WIP (41.93 KB, patch)
2011-10-11 18:03 PDT, Dave Camp (:dcamp)
rcampbell: feedback+
Details | Diff | Splinter Review
WIP 2 (51.62 KB, patch)
2011-10-18 12:08 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
WIP 3 (54.93 KB, patch)
2011-10-18 13:45 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
WIP 4 (54.68 KB, patch)
2011-10-19 08:57 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
Initial implementation (68.08 KB, patch)
2011-10-20 12:51 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
rule view with fixes (71.61 KB, patch)
2011-10-25 16:20 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
css fixes (71.30 KB, patch)
2011-10-26 11:55 PDT, Dave Camp (:dcamp)
dao+bmo: review+
Details | Diff | Splinter Review
patch as landed (71.25 KB, patch)
2011-11-01 18:37 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-10-11 18:03:44 PDT
Created attachment 566409 [details] [diff] [review]
Rule View WIP

* This isn't exposed via the highlighter yet, you need to inspectrules(node) from a web console.
* I only did css on mac, windows and linux is still todo
* No tests yet.

I plan to leave inherited styles for a followup, shouldn't be too tough.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-10-12 11:44:13 PDT
Comment on attachment 566409 [details] [diff] [review]
Rule View WIP

Nice patch, beautifully-documented.

+    for each (let rule in this.rules) {
+      // Properties within a rule are sorted from less to more
+      // important, walk in reverse order.
+      for (let i = rule.textProps.length - 1; i >= 0; i--) {
+        let textProp = rule.textProps[i];
+
+        let dirty = false;
+
+        for each (let computedProp in textProp.computed) {

Three layers of for looping can be expensive. Also confusing to try to make sense of.

for this:

+      // important, walk in reverse order.
+      for (let i = rule.textProps.length - 1; i >= 0; i--) {
+        let textProp = rule.textProps[i];

you could do:

let reversedProps = rule.textProps.reverse();
for each (let textProp in reversedProps) { ...

maybe slightly more expensive but makes the loop a little easier to understand. Your call there (perf vs. prettiness is a hard argument to make).

I think we can clean this method up a bit more too. Setting | old = null | and then doing an if (old) { ... } else { ... } feels like you could ditch the condition by inlining the else section and adding a break to the inner for loop.

or maybe not... It's a complicated method that seems to be holding and passing around a lot of state. Would the creation of some kind of OverriddenProps object help with that?

...

multiple editor types!

+  /**
+   * Keeps the editor close to the size of its input string.  This is pretty
+   * crappy, suggestions for improvement welcome.
+   */
+  _autosize: function InplaceEditor_autosize()

We struggled with this for the HTML attribute editor too. Didn't come up with a great solution to it, but at least here I think the widths of the fields are going to be reasonably-constrained.

+    this._measurement.textContent = this.input.value.replace(' ', '\u00a0', 'g');
+    this.input.style.width = (this._measurement.offsetWidth + 10) + "px";

that's cheeky!

What about this.elt.getBoundingClientRect(); for your width?

I'd like to play with this in a build before saying yay or nay, but I think, overall, the approach is solid. I'd like to see a little more effort on the markOverridden method and the input field's autosizing before giving an r+ (and of course, appropriate documentation in the editor parts).

Nice!
Comment 2 Dave Camp (:dcamp) 2011-10-18 12:08:07 PDT
Created attachment 567828 [details] [diff] [review]
WIP 2
Comment 3 Dave Camp (:dcamp) 2011-10-18 12:10:05 PDT
Latest WIP reworks the override calculation and adds some tests for the same.
Comment 4 Dave Camp (:dcamp) 2011-10-18 13:45:48 PDT
Created attachment 567863 [details] [diff] [review]
WIP 3

Added some tests for the inplace editor...
Comment 5 Dave Camp (:dcamp) 2011-10-19 08:57:49 PDT
Created attachment 568080 [details] [diff] [review]
WIP 4

Some small modifications from WIP 4.
Comment 6 Dave Camp (:dcamp) 2011-10-20 12:51:29 PDT
Created attachment 568483 [details] [diff] [review]
Initial implementation

I haven't tested the css on windows and linux yet...
Comment 7 Rob Campbell [:rc] (:robcee) 2011-10-20 14:55:37 PDT
Some observations while playing around:

1. Clicking on an open brace in Inline Styles should let you begin editing.
2. Live-updating of values causes CSS errors in the Error Console. Followup bug.
3. Test for property toggling.
4. Clicking inside the rule should begin editing (either the line or create a new line for a new property, not sure which yet)

this is gonna be swell! :D
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-21 07:44:44 PDT
and just to recap what we talked about in person, I think that issue 3 is the only one that this patch needs for this round. The other issues can be tackled in follow-ups.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-10-21 10:00:29 PDT
Comment on attachment 568483 [details] [diff] [review]
Initial implementation

CssRuleView.jsm:

in function ElementStyle():

+  this.dummyElement = doc.createElementNS(this.element.namespaceURI,
+                                          this.element.tagName);

do we need to do createElementNS here? I thought doc.createElement would inherit the namespace from the document. Not that it matters, really.

picky nit:

in markOverridden (at end):

+    for each (let textProp in textProps) {
+      // _updatePropertyOverridden will return true if the
+      // overridden state has changed for the text property.
+      if (this._updatePropertyOverridden(textProp)) {
+        textProp.updateEditor();
+      };
+    };

don't need semi-colons after the for and if blocks.

+   * on all computed properties.
+   *
+   * @return True if the TextProperty's overridden state changed.
+   */
+  _updatePropertyOverridden: function ElementStyle_updatePropertyOverridden(aProp)

should probably have a @param entry in the doc.

in Rule.prototype:

+  /**
+   * Create a new TextProperty to include in the rule.
+   */
+  createProperty: function Rule_createProperty(aName, aValue, aPriority)

nit: should have @params

+   * Renames a property.
+   */
+  setPropertyName: function Rule_setPropertyName(aProperty, aName)

@params (I'll stop bugging you about these from here in)

in _getTextProperties:

+    // These regular expressions are adapted from firebug's css.js.
+    let lines = this.style.cssText.match(/(?:[^;\(]*(?:\([^\)]*?\))?[^;\(]*)*;?/g);
+    let propRE = /\s*([^:\s]*)\s*:\s*(.*?)\s*(?:! (important))?;?$/;

I remember these regexes. I guess you didn't happen to find a cleaner way to express these, did you?

Maybe put those regexes into some consts rather than redeclaring them?

in updateComputed() comment nit:
+    // and see what the computed style looksl ike.
                                              ^ Ctrl-T

in InplaceEditor():

+  this._onBlur = this._onBlur.bind(this);
+  this._onKeyPress = this._onKeyPress.bind(this);
+  this._onInput = this._onInput.bind(this);

do these ever get deleted? Maybe remove them in _clear(), not sure if they'll cause leaks or not (I kind of doubt they will).

in csshtmltree.css:

+.ruleview-rule-source {
+  background-color: #DCE2E8;
+  padding: 2px;

could use background-color: -moz-dialog; if that's what you were shooting for with this color.

+  font-family: monospace;

font: message-box?

I'm not super fussy about the comment nits. I would like to see a green run through try though.
Comment 10 Dave Camp (:dcamp) 2011-10-25 16:17:30 PDT
(In reply to Rob Campbell [:rc] (robcee) from comment #7)

> 1. Clicking on an open brace in Inline Styles should let you begin editing.
> 4. Clicking inside the rule should begin editing (either the line or create
> a new line for a new property, not sure which yet)

Will file followups for these...

> 2. Live-updating of values causes CSS errors in the Error Console. Followup
> bug.
> 3. Test for property toggling.

Fixed these two in the new patch.

(In reply to Rob Campbell [:rc] (robcee) from comment #9)

> +  this.dummyElement = doc.createElementNS(this.element.namespaceURI,
> +                                          this.element.tagName);
> 
> do we need to do createElementNS here? I thought doc.createElement would
> inherit the namespace from the document. Not that it matters, really.

That would fail if inspecting a xul document while the CssRuleView is hosted in an html document.

> don't need semi-colons after the for and if blocks.

Weird, fixed.

> should probably have a @param entry in the doc.
> nit: should have @params
> @params (I'll stop bugging you about these from here in)

Added quite a few @param entries.

> +    // These regular expressions are adapted from firebug's css.js.
> +    let lines =
> this.style.cssText.match(/(?:[^;\(]*(?:\([^\)]*?\))?[^;\(]*)*;?/g);
> +    let propRE = /\s*([^:\s]*)\s*:\s*(.*?)\s*(?:! (important))?;?$/;
> 
> I remember these regexes. I guess you didn't happen to find a cleaner way to
> express these, did you?

No, I didn't.  I bet someone could.

> Maybe put those regexes into some consts rather than redeclaring them?

I did do that.

> in updateComputed() comment nit:
> +    // and see what the computed style looksl ike.

Oops.

> +  this._onBlur = this._onBlur.bind(this);
> +  this._onKeyPress = this._onKeyPress.bind(this);
> +  this._onInput = this._onInput.bind(this);
> 
> do these ever get deleted? Maybe remove them in _clear(), not sure if
> they'll cause leaks or not (I kind of doubt they will).

They're removed as event handlers, at which point the only thing keeping them alive is |this|, and GC should take care of that just fine.

> +.ruleview-rule-source {
> +  background-color: #DCE2E8;
> +  padding: 2px;
> 
> could use background-color: -moz-dialog; if that's what you were shooting
> for with this color.

I did that for now.

> +  font-family: monospace;
> 
> font: message-box?

I just pulled out the font-family entirely and it looks pretty nice in the default font.

> I'm not super fussy about the comment nits. I would like to see a green run
> through try though.

I'll put this newest patch up on the devtools branch.
Comment 11 Dave Camp (:dcamp) 2011-10-25 16:20:42 PDT
Created attachment 569537 [details] [diff] [review]
rule view with fixes

On top of the review comments, this patch adds:

* styleinspector.css for application-logic css.
* i18n for the "element" string.
Comment 12 Dave Camp (:dcamp) 2011-10-25 16:22:24 PDT
Comment on attachment 569537 [details] [diff] [review]
rule view with fixes

Dao, do you want to look at the css here?  It's loaded in an iframe, shouldn't affect browser xul.
Comment 13 Dão Gottwald [:dao] 2011-10-26 07:44:34 PDT
Comment on attachment 569537 [details] [diff] [review]
rule view with fixes

>--- /dev/null
>+++ b/browser/devtools/styleinspector/styleinspector.css

>+.ruleview-expander {
>+  visibility: hidden;
>+}

Is this always hidden? If yes, why does it exist? If no, what causes this to be shown again?

>+.ruleview-computedlist {
>+  display: none;
>+}
>+
>+.ruleview-computedlist.styleinspector-open {
>+  display: block;
>+}

.ruleview-computedlist:not(.styleinspector-open) {
  display: none;
}

>--- a/browser/themes/winstripe/browser/devtools/csshtmltree.css
>+++ b/browser/themes/winstripe/browser/devtools/csshtmltree.css

>+.ruleview {
>+  background-color: #FFF;
>+  position: relative;
>+  outline: 0;
>+}

Is focusable with the Tab key? It should get the focus ring in that case.

>+.ruleview-propertyname {
>+  display: inline-block;
>+  padding: 1px 0 1px 0;

padding: 1px 0;
Comment 14 Dave Camp (:dcamp) 2011-10-26 11:55:33 PDT
Created attachment 569750 [details] [diff] [review]
css fixes
Comment 15 Dave Camp (:dcamp) 2011-10-28 10:12:03 PDT
(In reply to Dão Gottwald [:dao] from comment #13)
> >+.ruleview-expander {
> >+  visibility: hidden;
> >+}
> 
> Is this always hidden? If yes, why does it exist? If no, what causes this to
> be shown again?

There's code that manually tweaks the visibility.  I got rid of this rule, it's not needed.

> 
> >+.ruleview-computedlist {
> >+  display: none;
> >+}
> >+
> >+.ruleview-computedlist.styleinspector-open {
> >+  display: block;
> >+}
> 
> .ruleview-computedlist:not(.styleinspector-open) {
>   display: none;
> }

Done.

> >--- a/browser/themes/winstripe/browser/devtools/csshtmltree.css
> >+++ b/browser/themes/winstripe/browser/devtools/csshtmltree.css
> 
> >+.ruleview {
> >+  background-color: #FFF;
> >+  position: relative;
> >+  outline: 0;
> >+}
> 
> Is focusable with the Tab key? It should get the focus ring in that case.

Removed outline: 0.

> >+.ruleview-propertyname {
> >+  display: inline-block;
> >+  padding: 1px 0 1px 0;
> 
> padding: 1px 0;

Done.
Comment 16 Dão Gottwald [:dao] 2011-10-29 11:06:02 PDT
Comment on attachment 569750 [details] [diff] [review]
css fixes

>+.ruleview {
>+  background-color: #FFF;
>+  position: relative;
>+}

Can you add a comment explaining what position:relative is doing here?
Comment 17 Dave Camp (:dcamp) 2011-11-01 18:37:01 PDT
Created attachment 571214 [details] [diff] [review]
patch as landed

https://hg.mozilla.org/integration/fx-team/rev/23daeec7cfa2

I removed the position: relative on .ruleview - That was leftover from when I didn't always put the view in an iframe.  This view is always in an iframe, and being relative to that iframe was just fine.
Comment 18 Tim Taubert [:ttaubert] 2011-11-02 03:01:29 PDT
https://hg.mozilla.org/mozilla-central/rev/23daeec7cfa2

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