The default bug view has changed. See this FAQ.

Create style panel for web page inspector (reticle) Milestone 0.2

RESOLVED FIXED in Firefox 4.0b2

Status

()

Firefox
Developer Tools
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

Trunk
Firefox 4.0b2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 21 obsolete attachments)

39.88 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Add a style panel to the web page inspector that shows CSS rules and element.style properties in a single panel.

https://wiki.mozilla.org/Firefox/Projects/Inspector#0.2
(Assignee)

Comment 1

7 years ago
Created attachment 440353 [details] [diff] [review]
style-panel-1.patch

patch 1. apply over contents of patch in bug 547453.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
Created attachment 440354 [details] [diff] [review]
style-panel-2.patch

second part. apply over contents of style-panel-1.patch.

includes some toolbar pieces from Milestone 0.4.
(Assignee)

Comment 3

7 years ago
Created attachment 440509 [details] [diff] [review]
style-panel-1b.patch

updated style-panel-1 patch after updates to hg.mozilla.org/users/rcampbell/mozilla-inspector

This should apply cleanly again.
Attachment #440353 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Created attachment 440511 [details] [diff] [review]
style-panel-2b.patch

Updated post changes to hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector.

Patch should apply cleanly again.
Attachment #440354 - Attachment is obsolete: true
(Assignee)

Comment 5

7 years ago
Created attachment 440598 [details] [diff] [review]
style-panel-3.patch

to be applied after the first 2.
(Assignee)

Comment 6

7 years ago
Created attachment 440638 [details] [diff] [review]
style-panel-3b.patch

comments, code cleanup and improved selector display.

apply after patches 1b and 2b.

next up, removing multiple rules.
Attachment #440598 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
Created attachment 440836 [details] [diff] [review]
style-panel-4.patch

includes overriding inherited rules, a short fix to handle frames and iframes, comments and more!
(Assignee)

Comment 8

7 years ago
Created attachment 440902 [details] [diff] [review]
style-panel-4b.patch

couple of minor tweaks.
Attachment #440836 - Attachment is obsolete: true
(Assignee)

Comment 9

7 years ago
Created attachment 440903 [details] [diff] [review]
style-panel-5.patch

Toolbar buttons for Inspect and Style panel toggling now work. (applies to Milestone 0.4)
(Assignee)

Comment 10

7 years ago
Created attachment 441106 [details] [diff] [review]
style-panel-test.patch

Basic style panel test. Simple styling included in style panel as a bonus.

see: http://www.flickr.com/photos/robceemoz/4546455090/ for a screenshot.
(Assignee)

Updated

7 years ago
Blocks: 561782
(Assignee)

Comment 11

7 years ago
Comment on attachment 440509 [details] [diff] [review]
style-panel-1b.patch

pushed to user-repo at 39792:354e13dd2e61
Attachment #440509 - Flags: review?(gsharp)
(Assignee)

Comment 12

7 years ago
Comment on attachment 440511 [details] [diff] [review]
style-panel-2b.patch

pushed to user-repo at: 39793:4972ca144de8
Attachment #440511 - Flags: review?(gsharp)
(Assignee)

Comment 13

7 years ago
Comment on attachment 440638 [details] [diff] [review]
style-panel-3b.patch

pushed to user repo at: 39794:70aeaf80f44c
Attachment #440638 - Flags: review?(gsharp)
(Assignee)

Comment 14

7 years ago
Comment on attachment 440902 [details] [diff] [review]
style-panel-4b.patch

pushed to user repo at: 39795:eca674c59fe7
Attachment #440902 - Flags: review?(gsharp)
(Assignee)

Updated

7 years ago
Attachment #440509 - Flags: review?(gsharp) → review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #440511 - Flags: review?(gsharp) → review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #440638 - Flags: review?(gsharp) → review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #440902 - Flags: review?(gsharp) → review?(gavin.sharp)
(Assignee)

Comment 15

7 years ago
Comment on attachment 440903 [details] [diff] [review]
style-panel-5.patch

pushed to user repo at: 39796:f66696fe00d3
Attachment #440903 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

7 years ago
Comment on attachment 441106 [details] [diff] [review]
style-panel-test.patch

style panel test and some highlighter css.

pushed at: 39797:4900c23a20ee
Attachment #441106 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

7 years ago
Created attachment 444636 [details] [diff] [review]
browser-inspector-stylePanel

moved from bug 547453.
Attachment #444636 - Flags: review?(gavin.sharp)
(Assignee)

Comment 18

7 years ago
Created attachment 444641 [details] [diff] [review]
remove-setTimeout-style-test
Attachment #444641 - Flags: review?(gavin.sharp)
(Assignee)

Comment 19

7 years ago
Created attachment 445359 [details] [diff] [review]
style-panel-rollup-rebased

rebased on moz-central, folded.
Attachment #440509 - Attachment is obsolete: true
Attachment #440511 - Attachment is obsolete: true
Attachment #440638 - Attachment is obsolete: true
Attachment #440902 - Attachment is obsolete: true
Attachment #440903 - Attachment is obsolete: true
Attachment #441106 - Attachment is obsolete: true
Attachment #444636 - Attachment is obsolete: true
Attachment #444641 - Attachment is obsolete: true
Attachment #445359 - Flags: review?(gavin.sharp)
Attachment #440509 - Flags: review?(gavin.sharp)
Attachment #440511 - Flags: review?(gavin.sharp)
Attachment #440638 - Flags: review?(gavin.sharp)
Attachment #440902 - Flags: review?(gavin.sharp)
Attachment #440903 - Flags: review?(gavin.sharp)
Attachment #441106 - Flags: review?(gavin.sharp)
Attachment #444636 - Flags: review?(gavin.sharp)
Attachment #444641 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

7 years ago
Created attachment 445412 [details] [diff] [review]
style-panel-rollup-rebased-2

Retested and cleaned this up a bit more. A couple of errors due to renamed parameters during inspector rebasing and review cleanup. Removed logging. Fixed the browser_inspector_stylePanel test. Sorry for the churn.
Attachment #445359 - Attachment is obsolete: true
Attachment #445412 - Flags: review?(gavin.sharp)
Attachment #445359 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 566088
(Assignee)

Comment 21

7 years ago
Created attachment 445951 [details] [diff] [review]
resize highlight

missed this when rebasing patch. handleEvent was sending rehighlight which got removed during cleanup of previous patches. Changing this to call highlight() now to restore resize functionality.
Attachment #445951 - Flags: review?(gavin.sharp)
(Assignee)

Comment 22

7 years ago
Created attachment 451635 [details] [diff] [review]
style-panel-rebased.patch

rebased to latest (June 16, 2010) mozilla-central.
Attachment #445412 - Attachment is obsolete: true
Attachment #445951 - Attachment is obsolete: true
Attachment #451635 - Flags: review?(gavin.sharp)
Attachment #445412 - Flags: review?(gavin.sharp)
Attachment #445951 - Flags: review?(gavin.sharp)
Attachment #451635 - Flags: review?(gavin.sharp) → review-
Comment on attachment 451635 [details] [diff] [review]
style-panel-rebased.patch

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+listitem.style-selector {
>+  background-color: DarkGray;
>+  color: white;
>+}
>+listitem.style-section {
>+  background-color: LightGray;
>+  color: black;
>+  font-weight: bold;
>+}
>\ No newline at end of file

Should add a newline, but these rules look like they belong in in the themes (winstripe/pinstripe/gnomestripe).

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <toolbar id="inspector-toolbar"

These buttons need localizable labels/accesskeys.

>+    <panel id="inspector-style-panel"

>+           noautohide="true"
>+           level="top"

noautohide means "level" is ignored, so you can omit it (this also applies to the existing inspector-panel, I forgot to check that earlier).

>+           aria-labelledby="inspectStylePanelTitle">

this element doesn't seem to exist...

>+        <toolbar id="inspector-style-toolbar"
>+                 nowindowdrag="true">
>+          <label>Style</label>

l12y needed here too

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>-    for (let i = line.length - 1; i >= 0; i--) {
>+    for (let i = line.length - 1; i >= 0; --i) {

sweet, perf boost! :)

>+  toggleStylePanel: function IUI_toggleStylePanel()

>+    this._showStylePanel = !this._showStylePanel;

Do we need _showStylePanel? Seems like we could just use isStylePanelOpen instead?

With only this patch applied, toggling the "Inspect" menu item off leaves the style panel hanging around, and there seems to be no way to dismiss it again - I didn't get a chance to investigate why that is exactly.

>+  /**   * Is the tree panel open?

nit: stray "*"!

>+  openStylePanel: function IUI_openStylePanel()

>+      this.stylePanel.openPopup(this.browser, "end_after", -100,
>+        - this.win.outerHeight / 2 + 40, false, false);
>+      this.stylePanel.sizeTo(180, this.win.outerHeight / 3);

These numbers are a bit magic-y, maybe add a comment explaining the intended size/position?

>+  dimStylePanel: function IUI_dimStylePanel()

>+  undimStylePanel: function IUI_undimStylePanel()

This might be better to implement as a single dimPanel(aDim) that sets/removes a "dimmed" class or attribute, which is styled with CSS from the theme.

>+  addStyleItem: function IUI_addStyleItem(aLabel, aType, content)

>+    item.className = "style-" + aType;
>+    if (content) {
>+      item.setAttribute("label", aLabel + ": " + content);
>+    } else {
>+      item.setAttribute("label", aLabel);
>+    }

Not sure this kind of concatenation is l10n-friendly - probably want to use a stringbundle with formatStringFromName and positional arguments for this.

>+  createStyleRuleItems: function IUI_createStyleRuleItems(rules)

>+  createStyleItems: function IUI_createStyleItems(rules, sections)

l12y needed for these too.

>+  handleEvent: function IUI_handleEvent(event)

>       case "scroll":
>         this.highlighter.highlight();
>         break;
>+      case "resize":
>+        this.highlighter.highlight();
>+        break;

just merge these two?

>+  isSystemStyleSheet: function IUI_isSystemStyleSheet(aSheet)
>+  {
>+    if (!aSheet) return true;

nit: returns on separate lines are much easier to see

>+    let url = aSheet.href;
>+
>+    if (url.length == 0)

Need a null check on "url" here - I hit a case of this being null on the main wikipedia.org page.

>+   * Parse properties from a given style object.
>+   * Borrowed from Firebug's css.js.

Wouldn't hurt to add the contributor names to the license header if you know them (assuming it's also tri-licensed?).

>+  getInheritedRules: function IUI_getInheritedRules(aNode, sections, usedProps)

>+      if (rules.length) {
>+        sections.splice(0, 0, {element: parent, rules: rules});

sections.unshift() would be clearer (also applies to getElementRules)

>diff --git a/browser/base/content/test/browser_inspector_stylePanel.js b/browser/base/content/test/browser_inspector_stylePanel.js

>+function performTestComparisons(evt)

>+  ok(InspectorUI.styleBox.itemCount > 0, "styleBox has items");

Checking that the styles match the expected <span> would be a nice addition to this test.

r- because of the l10n changes needed and the style panel persistence problem, but I imagine those should be quick to address.
(Assignee)

Comment 24

7 years ago
(In reply to comment #23)
> (From update of attachment 451635 [details] [diff] [review])
> >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css
> 
> >+listitem.style-selector {
> >+  background-color: DarkGray;
> >+  color: white;
> >+}
> >+listitem.style-section {
> >+  background-color: LightGray;
> >+  color: black;
> >+  font-weight: bold;
> >+}
> >\ No newline at end of file
> 
> Should add a newline, but these rules look like they belong in in the themes
> (winstripe/pinstripe/gnomestripe).

added and moved to each of the 'stripes.

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+      <toolbar id="inspector-toolbar"
> 
> These buttons need localizable labels/accesskeys.

gotcha. I'll leave the addition of a keyset until later.

> >+    <panel id="inspector-style-panel"
> 
> >+           noautohide="true"
> >+           level="top"
> 
> noautohide means "level" is ignored, so you can omit it (this also applies to
> the existing inspector-panel, I forgot to check that earlier).

ah. These will be changing slightly when Enn's panel titlebar patches land. Style and DOM panels will be set to "floating".

Removed noautohide attribute.

> >+           aria-labelledby="inspectStylePanelTitle">
> 
> this element doesn't seem to exist...

ewps! Do I need this attribute here? I don't have an element for inspectPagePanelTitle either. Removing them.

> >+        <toolbar id="inspector-style-toolbar"
> >+                 nowindowdrag="true">
> >+          <label>Style</label>
> 
> l12y needed here too

added.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >-    for (let i = line.length - 1; i >= 0; i--) {
> >+    for (let i = line.length - 1; i >= 0; --i) {
> 
> sweet, perf boost! :)

it is so fast now!

> >+  toggleStylePanel: function IUI_toggleStylePanel()
> 
> >+    this._showStylePanel = !this._showStylePanel;
> 
> Do we need _showStylePanel? Seems like we could just use isStylePanelOpen
> instead?

I was using those as placeholders for a pref. Currently the style button on the toolbar toggles that to false. Might be better to have an object containing active and inactive panels so others could add to them for extensions anyway. What do you think? Remove it, leave it in until later?

> With only this patch applied, toggling the "Inspect" menu item off leaves the
> style panel hanging around, and there seems to be no way to dismiss it again -
> I didn't get a chance to investigate why that is exactly.

I missed the section when back-porting these patches that hides the panel on shutdown. I've readded it (see, InspectorUI::closeInspectorUI() for the relevant section).

> >+  /**   * Is the tree panel open?
> 
> nit: stray "*"!

denitted.

> >+  openStylePanel: function IUI_openStylePanel()
> 
> >+      this.stylePanel.openPopup(this.browser, "end_after", -100,
> >+        - this.win.outerHeight / 2 + 40, false, false);
> >+      this.stylePanel.sizeTo(180, this.win.outerHeight / 3);
> 
> These numbers are a bit magic-y, maybe add a comment explaining the intended
> size/position?

actually I meant to tweak those again> I don't want the style panel obscuring the browser at all. Added comments and changed positioning so the panel resizes properly later on.

> >+  dimStylePanel: function IUI_dimStylePanel()
> 
> >+  undimStylePanel: function IUI_undimStylePanel()
> 
> This might be better to implement as a single dimPanel(aDim) that sets/removes
> a "dimmed" class or attribute, which is styled with CSS from the theme.

You're making me touch all the files with this one. :)

done.

> >+  addStyleItem: function IUI_addStyleItem(aLabel, aType, content)
> 
> >+    item.className = "style-" + aType;
> >+    if (content) {
> >+      item.setAttribute("label", aLabel + ": " + content);
> >+    } else {
> >+      item.setAttribute("label", aLabel);
> >+    }
> 
> Not sure this kind of concatenation is l10n-friendly - probably want to use a
> stringbundle with formatStringFromName and positional arguments for this.

These are CSS properties. I don't think they should be localized. Adding a comment to that effect in the method.

> >+  createStyleRuleItems: function IUI_createStyleRuleItems(rules)
> 
> >+  createStyleItems: function IUI_createStyleItems(rules, sections)
> 
> l12y needed for these too.

Yep, there are a couple of strings in here that should be bundleized. Done.

> >+  handleEvent: function IUI_handleEvent(event)
> 
> >       case "scroll":
> >         this.highlighter.highlight();
> >         break;
> >+      case "resize":
> >+        this.highlighter.highlight();
> >+        break;
> 
> just merge these two?

Um. Yes!

> >+  isSystemStyleSheet: function IUI_isSystemStyleSheet(aSheet)
> >+  {
> >+    if (!aSheet) return true;
> 
> nit: returns on separate lines are much easier to see

True.

> >+    let url = aSheet.href;
> >+
> >+    if (url.length == 0)
> 
> Need a null check on "url" here - I hit a case of this being null on the main
> wikipedia.org page.

hunh. That's weird. added.

> >+   * Parse properties from a given style object.
> >+   * Borrowed from Firebug's css.js.
> 
> Wouldn't hurt to add the contributor names to the license header if you know
> them (assuming it's also tri-licensed?).

I don't know them for these particular methods, though I guess I could just add everybody? It's not tri-licensed, it's BSD, which I was told is lenient enough that you can do what you want with it, but probably requires the license to be included.

I'll ask about the license part...

... and after a conversation with our advisors, I'm waiting to hear what our best-practises are in this area. We may not have to separate everything.

> >+  getInheritedRules: function IUI_getInheritedRules(aNode, sections, usedProps)
> 
> >+      if (rules.length) {
> >+        sections.splice(0, 0, {element: parent, rules: rules});
> 
> sections.unshift() would be clearer (also applies to getElementRules)

I seem to remember trying this when I ported these over and had a problem with it. I'll try again.

> >diff --git a/browser/base/content/test/browser_inspector_stylePanel.js b/browser/base/content/test/browser_inspector_stylePanel.js
> 
> >+function performTestComparisons(evt)
> 
> >+  ok(InspectorUI.styleBox.itemCount > 0, "styleBox has items");
> 
> Checking that the styles match the expected <span> would be a nice addition to
> this test.

true, true.

> r- because of the l10n changes needed and the style panel persistence problem,
> but I imagine those should be quick to address.

revised patch coming shortly.
(Assignee)

Comment 25

7 years ago
Created attachment 452086 [details] [diff] [review]
inspector-style-panel-reviewed

a patch to be applied after previous one addressing review comments.

Outstanding bits are still BSD license and attribution, and style panel test addition. We can add to that test in a follow-up bug.
Attachment #452086 - Flags: review?(gavin.sharp)
(Assignee)

Comment 26

7 years ago
Created attachment 452090 [details] [diff] [review]
inspector-style-despliced

missed a couple of commented out lines. This patch removes them.
(Assignee)

Comment 27

7 years ago
Created attachment 452091 [details] [diff] [review]
inspector.properties

added inspector.properties file

Comment 28

7 years ago
>> Wouldn't hurt to add the contributor names to the license header if you know
>> them (assuming it's also tri-licensed?).
> 
> I don't know them for these particular methods, though I guess I could just add
> everybody? It's not tri-licensed, it's BSD, which I was told is lenient enough
> that you can do what you want with it, but probably requires the license to be
> included.

IANAL, but Groklaw says that if you do mix BSD code into a file that is GPLed you have to retain the copyright attributions. Further it appears that it is also best practice to delineate the BSD parts clearly so that in future someone could extract the BSD parts and revert those parts to BSD only. IIRC Mozilla policy is to keep BSD/MIT/public domain code separate in other-licences/

> I'll ask about the license part...

Yes Gerv and/or Mozilla legal should be asked.

> ... and after a conversation with our advisors, I'm waiting to hear what our
> best-practises are in this area. We may not have to separate everything.

OTOH, you could ask John J Barton if the firebug team is willing to re-licence those lines under the tri-licence.
(Assignee)

Comment 29

7 years ago
After talking to Gavin and with Gerv and Luis Villa, I think the easiest way forward is to split out those lines into a separate file. I think the cost of doing that is less than it is to annotate individual sections or functions in the main file.

I should have a patch up today.
(Assignee)

Comment 30

7 years ago
Created attachment 452876 [details] [diff] [review]
inspector-extracted-style-panel

removed imported methods from inspector.js and placed in stylePanel.jsm. Added firebug-license.txt with reference from stylePanel.jsm. Added contributors to stylePanel.jsm.
Attachment #451635 - Attachment is obsolete: true
Attachment #452086 - Attachment is obsolete: true
Attachment #452090 - Attachment is obsolete: true
Attachment #452091 - Attachment is obsolete: true
Attachment #452876 - Flags: review?(gavin.sharp)
Attachment #452086 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 574408
Comment on attachment 452876 [details] [diff] [review]
inspector-extracted-style-panel

>diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc

>+    <stringbundle id="bundle_inspector" src="chrome://browser/locale/inspector.properties"/>

Looks to be unused. (But that's a good thing, since XBL overhead for <stringbundle> elements is sucky... just using the stringBundleService directly as you're doing is best.)

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+    <panel id="inspector-style-panel"

I think we want hidden="true" here like the others, right?

>+           class="dimmable"

Does this serve a useful purpose? Seems like you should be able to just call dimPanel on any panel.

>+        <toolbar id="inspector-style-toolbar"
>+                 nowindowdrag="true">
>+          <label>&inspectStylePanelTitle.label;</label>
>+        </toolbar>

Is the toolbar necessary here?

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

> var InspectorUI = {

>+  dimPanel: function IUI_dimPanel(aDim)

toggleDimForPanel?

>+  initializeStrings: function IUI_initializeStrings()

Just inline this? No real use to having a separate function right?

>+  addStyleItem: function IUI_addStyleItem(aLabel, aType, content)

should be consistent with "a" prefixes if you have them ("aContent") (also applies elsewhere)

>+    if (content) {
>+      item.setAttribute("label", aLabel + ": " + content);
>+    } else {
>+      item.setAttribute("label", aLabel);
>+    }

very nitpicky, but I find this easier to read as:
let label = aLabel;
if (content)
  label += ": " + content;
item.setAttribute("label", label);

>+  createStyleRuleItems: function IUI_createStyleRuleItems(rules)
>+  {
>+    for (let rulesIndex = 0; rulesIndex < rules.length; ++rulesIndex) {

use forEach? Applies to a bunch of other iteration loops in this file and stylePanel.jsm.

>+      let selectorLabel = this.inspectorBundle.GetStringFromName("style.selectorLabel");

Should get this outside of the loop.

>+  createStyleItems: function IUI_createStyleItems(rules, sections)

>+    for (let sectionIndex = 0; sectionIndex < sections.length; ++sectionIndex) {

>+        this.inspectorBundle.formatStringFromName("style.inheritedFrom",
>+          [sectionTitle], 1);

This could avoid the call to formatStringFromName in the loop by doing the replacement manually (using getString() with a "#1" placeholder), similar to e.g. addonsInstalledNeedsRestart in browser.properties.

>diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm

>+/* See firebug-license.txt for terms of usage */

Can it not just be inlined here?

>+  parseCSSProperties: function CSS_parseCSSProps(aStyle)

I thought there'd be a way of doing this with the CSSOM... Worth a followup to investigate cleaner ways of doing the things this file does, to get input from CSSOM experts, perhaps?

>+  getElementRules: function CSS_getElementRules(aNode, rules, usedProps, inherit)

>+    try {
>+      inspectedRules = this.domUtils ? this.domUtils.getCSSStyleRules(aNode)
>+        : null;
>+    } catch (ex) {
>+      Services.console.logStringMessage(msg);
>+    }

msg looks undefined. The code below doesn't null check this.domUtils, so seems pointless to do it here.

>+      if (!this._showUserAgentCSS && isSystemSheet)
>+        continue;

_showUserAgentCSS is always undefined, since it's over in inspector.js

>\ No newline at end of file

add one?

>diff --git a/browser/base/content/test/browser_inspector_stylePanel.js b/browser/base/content/test/browser_inspector_stylePanel.js

>+function setupStyleTests()

>+  ok(spans, "we have the spans");

Please make this: "captain, we have the spans"

>+function performTestComparisons(evt)

Would be good to have a followup on making this check more about the state of the panel (also transitions between states rather than just doing the same tests multiple times for different spans)

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY inspectButton.commandkey     "I">
>+<!ENTITY inspectNextButton.commandkey ".">
>+<!ENTITY inspectPreviousButton.commandkey ",">
>+<!ENTITY inspectStyleButton.commandkey ";">

These are unused.

>diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties

>+style.selectorLabel=Selector
>+style.inheritedFrom=Inherited from: %S

Some LOCALIZATION NOTES for these would be useful, since it's not immediately obvious how they're used (see https://developer.mozilla.org/en/xul_coding_style_guidelines#Localization_Notes )

The undefined variable issues in stylePanel.jsm need fixing, so r-, but it should be quick to re-review once these comments are addressed.
Attachment #452876 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 32

7 years ago
(In reply to comment #31)
> Comment on attachment 452876 [details] [diff] [review]
> inspector-extracted-style-panel
> 
> >diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
> 
> >+    <stringbundle id="bundle_inspector" src="chrome://browser/locale/inspector.properties"/>
> 
> Looks to be unused. (But that's a good thing, since XBL overhead for
> <stringbundle> elements is sucky... just using the stringBundleService directly
> as you're doing is best.)

groovy. Removing.

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+    <panel id="inspector-style-panel"
> 
> I think we want hidden="true" here like the others, right?

yes, probably true.

> >+           class="dimmable"
> 
> Does this serve a useful purpose? Seems like you should be able to just call
> dimPanel on any panel.

only useful for differentiating which panels we could dim. I thought it looked nice in browser.css. I can remove it if you don't think it's useful.

> >+        <toolbar id="inspector-style-toolbar"
> >+                 nowindowdrag="true">
> >+          <label>&inspectStylePanelTitle.label;</label>
> >+        </toolbar>
> 
> Is the toolbar necessary here?

It's useful as a placeholder for the imminently landable titlebars in bug 552982.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> > var InspectorUI = {
> 
> >+  dimPanel: function IUI_dimPanel(aDim)
> 
> toggleDimForPanel?

Yeah, I think so. I took your last review suggestion literaly and just left the name. updating...

> >+  initializeStrings: function IUI_initializeStrings()
> 
> Just inline this? No real use to having a separate function right?

yeah, I thought I was going to have to do more there. Nixed!

> >+  addStyleItem: function IUI_addStyleItem(aLabel, aType, content)
> 
> should be consistent with "a" prefixes if you have them ("aContent") (also
> applies elsewhere)

OK.

> >+    if (content) {
> >+      item.setAttribute("label", aLabel + ": " + content);
> >+    } else {
> >+      item.setAttribute("label", aLabel);
> >+    }
> 
> very nitpicky, but I find this easier to read as:
> let label = aLabel;
> if (content)
>   label += ": " + content;
> item.setAttribute("label", label);

Yeah, you're right.

> >+  createStyleRuleItems: function IUI_createStyleRuleItems(rules)
> >+  {
> >+    for (let rulesIndex = 0; rulesIndex < rules.length; ++rulesIndex) {
> 
> use forEach? Applies to a bunch of other iteration loops in this file and
> stylePanel.jsm.

OK.

> >+      let selectorLabel = this.inspectorBundle.GetStringFromName("style.selectorLabel");
> 
> Should get this outside of the loop.

whups!

> >+  createStyleItems: function IUI_createStyleItems(rules, sections)
> 
> >+    for (let sectionIndex = 0; sectionIndex < sections.length; ++sectionIndex) {
> 
> >+        this.inspectorBundle.formatStringFromName("style.inheritedFrom",
> >+          [sectionTitle], 1);
> 
> This could avoid the call to formatStringFromName in the loop by doing the
> replacement manually (using getString() with a "#1" placeholder), similar to
> e.g. addonsInstalledNeedsRestart in browser.properties.

That's preferable, I guess? OK, .replaced.

> >diff --git a/browser/base/content/stylePanel.jsm b/browser/base/content/stylePanel.jsm
> 
> >+/* See firebug-license.txt for terms of usage */
> 
> Can it not just be inlined here?

Yes, I suppose it could be.

> >+  parseCSSProperties: function CSS_parseCSSProps(aStyle)
> 
> I thought there'd be a way of doing this with the CSSOM... Worth a followup to
> investigate cleaner ways of doing the things this file does, to get input from
> CSSOM experts, perhaps?

absolutely.

> >+  getElementRules: function CSS_getElementRules(aNode, rules, usedProps, inherit)
> 
> >+    try {
> >+      inspectedRules = this.domUtils ? this.domUtils.getCSSStyleRules(aNode)
> >+        : null;
> >+    } catch (ex) {
> >+      Services.console.logStringMessage(msg);
> >+    }
> 
> msg looks undefined. The code below doesn't null check this.domUtils, so seems
> pointless to do it here.

oops. I missed the ex->msg correction I made in a later patch. Removed the null check.

> >+      if (!this._showUserAgentCSS && isSystemSheet)
> >+        continue;
> 
> _showUserAgentCSS is always undefined, since it's over in inspector.js

ah, missed that. Result was the same since I was going to default to off for this.

> 
> >\ No newline at end of file
> 
> add one?

Okay!

> >diff --git a/browser/base/content/test/browser_inspector_stylePanel.js b/browser/base/content/test/browser_inspector_stylePanel.js
> 
> >+function setupStyleTests()
> 
> >+  ok(spans, "we have the spans");
> 
> Please make this: "captain, we have the spans"

Very wel... hey! Now you're just messing with me. (I did it anyway)

> >+function performTestComparisons(evt)
> 
> Would be good to have a followup on making this check more about the state of
> the panel (also transitions between states rather than just doing the same
> tests multiple times for different spans)

Good. I will file.

> >diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd
> 
> >+<!ENTITY inspectButton.commandkey     "I">
> >+<!ENTITY inspectNextButton.commandkey ".">
> >+<!ENTITY inspectPreviousButton.commandkey ",">
> >+<!ENTITY inspectStyleButton.commandkey ";">
> 
> These are unused.

Removed.

> >diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties
> 
> >+style.selectorLabel=Selector
> >+style.inheritedFrom=Inherited from: %S
> 
> Some LOCALIZATION NOTES for these would be useful, since it's not immediately
> obvious how they're used (see
> https://developer.mozilla.org/en/xul_coding_style_guidelines#Localization_Notes
> )

Done.

> The undefined variable issues in stylePanel.jsm need fixing, so r-, but it
> should be quick to re-review once these comments are addressed.

Just going to do a quick build to make sure everything passes then I'll update the attached patch.

Thanks for the review!
(Assignee)

Comment 33

7 years ago
Created attachment 457473 [details] [diff] [review]
revised style panel patch

this should address most of the above comments.
Attachment #452876 - Attachment is obsolete: true
Attachment #457473 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 578885
(Assignee)

Comment 34

7 years ago
one issue, I couldn't call getString from inspectorBundle for some reason. I had to stick with GetStringFromName. Wondering if I initialized it correctly?

Also, filed bug 578885 to augment the style panel test.
Comment on attachment 457473 [details] [diff] [review]
revised style panel patch

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+    <panel id="inspector-style-panel"

>+           class="dimmable"

I think I would prefer removing this.

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

> var InspectorUI = {

>+  _showUserAgentCSS: false,

Unused now...

>+  createStyleRuleItems: function IUI_createStyleRuleItems(aRules)

>+    aRules.forEach(
>+      function(rule) {

Put function on the same line, e.g.:

aRules.forEach(function (rule) {
  //...
});

(applies elsewhere)

>+  addStyleItem: function IUI_addStyleItem(aLabel, aType, aContent)

>+    // Do not localize these strings
>+    let label = aLabel;
>+    item.className = "style-" + aType;
>+    if (aContent)
>+      label += ": " + aContent;

Upon further reflection I'm not sure that this will fly for e.g. RTL languages, or languages where ": " may not be a suitable separator (Japanese?). Any reason not to do the same thing we do for the other string here (replace #1)?

>diff --git a/browser/base/content/test/browser_inspector_initialization.js b/browser/base/content/test/browser_inspector_initialization.js

> function finishInspectorTests(evt)
> {
>-  if (evt.target.id != "inspector-panel")
>+  if (evt.target.id != "inspector-style-panel")

Hmm, why this change? Seems to me like this test should stick to checking for the main inspector panel...

>diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties

>+# LOCALIZATION NOTE  (style.selectorLabel): used in Style panel in 
>+#  inspector. Denotes CSS selector for a set of rules.

I'd use:
"Used in the Inspector style panel to describe a CSS Selector." Though if you take my suggestion from above this will turn into "Selector: #1", so maybe s/describe/label/ and then mention that #1 is "CSS selector + URL"

>+# LOCALIZATION NOTE  (style.inheritedFrom): used in Style panel in
>+#  inspector. Describes which CSS stylesheet and URL (#1) the rules were
>+#  inherited from.
>+style.inheritedFrom=Inherited from: #1

Isn't #1 in the form of "tagname[#id]"?
Attachment #457473 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 36

7 years ago
(In reply to comment #35)
> Comment on attachment 457473 [details] [diff] [review]
> revised style panel patch
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+    <panel id="inspector-style-panel"
> 
> >+           class="dimmable"
> 
> I think I would prefer removing this.

ok, removed and extracted from the *stripes.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> > var InspectorUI = {
> 
> >+  _showUserAgentCSS: false,
> 
> Unused now...

whups. Ripped.

> 
> >+  createStyleRuleItems: function IUI_createStyleRuleItems(aRules)
> 
> >+    aRules.forEach(
> >+      function(rule) {
> 
> Put function on the same line, e.g.:
> 
> aRules.forEach(function (rule) {
>   //...
> });
> 
> (applies elsewhere)

done.

> 
> >+  addStyleItem: function IUI_addStyleItem(aLabel, aType, aContent)
> 
> >+    // Do not localize these strings
> >+    let label = aLabel;
> >+    item.className = "style-" + aType;
> >+    if (aContent)
> >+      label += ": " + aContent;
> 
> Upon further reflection I'm not sure that this will fly for e.g. RTL languages,
> or languages where ": " may not be a suitable separator (Japanese?). Any reason
> not to do the same thing we do for the other string here (replace #1)?

I went with a string that uses the form #1: #2 for label and content. Hopefully that'll work in all cases and let some locales flip them around if that makes sense there.

> >diff --git a/browser/base/content/test/browser_inspector_initialization.js b/browser/base/content/test/browser_inspector_initialization.js
> 
> > function finishInspectorTests(evt)
> > {
> >-  if (evt.target.id != "inspector-panel")
> >+  if (evt.target.id != "inspector-style-panel")
> 
> Hmm, why this change? Seems to me like this test should stick to checking for
> the main inspector panel...

The style panel closes last. If I wait on the inspector-panel, the test hits finishInspectorTests too soon and fails.

I add some notifications in a later version to alert interested observers when the inspector shuts down that eliminates these tricky cases.

> >diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties
> 
> >+# LOCALIZATION NOTE  (style.selectorLabel): used in Style panel in 
> >+#  inspector. Denotes CSS selector for a set of rules.
> 
> I'd use:
> "Used in the Inspector style panel to describe a CSS Selector." Though if you
> take my suggestion from above this will turn into "Selector: #1", so maybe
> s/describe/label/ and then mention that #1 is "CSS selector + URL"

The way I'm constructing these (and at some point, I'll want to elaborate on it so we can attach other properties to these list items), we'll now pass the selectorLabel in as an argument to addStyleItem. It'll do the replacement with two values so we don't need the ": #1" piece in the string.

Make sense?

e.g.,
  addStyleItem: function IUI_addStyleItem(aLabel, aType, aContent)
  {
    let itemLabelString = this.inspectorBundle.GetStringFromName("style.styleItemLabel");
    let item = document.createElement("listitem");

    // Do not localize these strings
    let label = aLabel;
    item.className = "style-" + aType;
    if (aContent) {
      label = itemLabelString.replace("#1", aLabel);
      label = label.replace("#2", aContent);
    }
    item.setAttribute("label", label);

    this.styleBox.appendChild(item);
  },

> >+# LOCALIZATION NOTE  (style.inheritedFrom): used in Style panel in
> >+#  inspector. Describes which CSS stylesheet and URL (#1) the rules were
> >+#  inherited from.
> >+style.inheritedFrom=Inherited from: #1
> 
> Isn't #1 in the form of "tagname[#id]"?

You're right. Fixing.

Just testing this patch locally, and if all goes well I'll check it in.

Thanks for the reviews!
(Assignee)

Comment 37

7 years ago
Created attachment 457844 [details] [diff] [review]
[checked-in] style panel final

changeset:   47791:5575b3579d2f
user:        Rob Campbell <rcampbell@mozilla.com>
date:        Fri Jul 16 11:12:39 2010 -0300
summary:     bug 560692 - Create a style panel for web page inspector. p=me, r=gavin
Attachment #457473 - Attachment is obsolete: true
Attachment #457844 - Flags: review+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: --- → Firefox 4.0b2
(Assignee)

Comment 38

7 years ago
why was this moved to unconfirmed?
I have no idea!
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.