Implement a rule-centric style view

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #566409 - Flags: feedback?(rcampbell)
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!
Attachment #566409 - Flags: feedback?(rcampbell) → feedback+
(Assignee)

Comment 2

6 years ago
Created attachment 567828 [details] [diff] [review]
WIP 2
Attachment #566409 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Latest WIP reworks the override calculation and adds some tests for the same.
(Assignee)

Comment 4

6 years ago
Created attachment 567863 [details] [diff] [review]
WIP 3

Added some tests for the inplace editor...
Attachment #567828 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 568080 [details] [diff] [review]
WIP 4

Some small modifications from WIP 4.
Attachment #567863 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 696180
(Assignee)

Updated

6 years ago
Blocks: 696181
(Assignee)

Updated

6 years ago
Blocks: 696182
(Assignee)

Comment 6

6 years ago
Created attachment 568483 [details] [diff] [review]
Initial implementation

I haven't tested the css on windows and linux yet...
Attachment #568080 - Attachment is obsolete: true
Attachment #568483 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #568483 - Flags: review? → review?(rcampbell)
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
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.
Status: NEW → ASSIGNED
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.
Attachment #568483 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
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.
Attachment #568483 - Attachment is obsolete: true
Attachment #569537 - Flags: review?(rcampbell)
(Assignee)

Comment 12

6 years ago
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.
Attachment #569537 - Flags: review?(dao)
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;

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 9 Branch → Trunk
(Assignee)

Comment 14

6 years ago
Created attachment 569750 [details] [diff] [review]
css fixes
Attachment #569537 - Attachment is obsolete: true
Attachment #569537 - Flags: review?(rcampbell)
Attachment #569537 - Flags: review?(dao)
Attachment #569750 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 15

6 years ago
(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 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?
Attachment #569750 - Flags: review?(dao) → review+
(Assignee)

Comment 17

6 years ago
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.
Attachment #569750 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/23daeec7cfa2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
(Assignee)

Updated

6 years ago
Blocks: 699213
(Assignee)

Updated

6 years ago
Blocks: 699215
You need to log in before you can comment on or make changes to this bug.