672748 - Style inspector UI refresh

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: mockup

Limi created a mockup some time back and the design is far better than the one we currently use. We are no longer using categories but the rest of the mockup is still relevant.

Comment 2

[Rob Campbell [:rc] (:robcee)]

adding silly status keyword

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update Style Inspector UI

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 1: All categories collapsed

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 2: Background category expanded

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 3: Text, Fonts & Color Category expanded

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 4: Text, Fonts & Color Category expanded along with rules

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update Style Inspector UI

Updated patch according to feedback

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 1: All categories collapsed

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 2: Background category expanded

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Text, Fonts & Color Category expanded

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot 4: Text, Fonts & Color Category expanded along with rules

Comment 13

[Rob Campbell [:rc] (:robcee)]

giving this a ridiculous worst-case scenario in case we get stuck in a rathole redesigning this thing.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug 672743 removes categories so any problems related to category styles can be ignored. A screenshot of the style inspector without categories is included in that bug.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update Style Inspector UI

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot - note that categories have been removed since Limi's mockup was created

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

^ Rebased

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 548726 [details]
Screenshot - note that categories have been removed since Limi's mockup was created

this looks a ton better, imo. Nice.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

shorlander: Are you okay with the design in https://bugzilla.mozilla.org/attachment.cgi?id=548726 ?

Comment 21

[Stephen Horlander [:shorlander]]

(In reply to comment #20)
> shorlander: Are you okay with the design in
> https://bugzilla.mozilla.org/attachment.cgi?id=548726 ?

I haven't had a chance to use it yet. Is there a build with this floating around? If not I can just build it locally.

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Not yet but I will hopefully be creating a try build tomorrow if that would be of any help.

Comment 23

[Rob Campbell [:rc] (:robcee)]

creating an integration build with this included.

patch is currently failing to apply. fixing...

Comment 24

[Rob Campbell [:rc] (:robcee)]

this fails in gnomestripe/.../csshtmltree.css pretty hard. Looks like you're diffing against a different version of this file.

also, afaik, linux doesn't ship with a version of Arial. Use sansserif.

Comment 25

[Rob Campbell [:rc] (:robcee)]

of course it works much better if you apply the patches in order. disregard my previous comment about this failing to apply.

(but do take note of the reference to Arial in gnomestripe!)

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch

Rebased

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 551800 [details] [diff] [review]
Update style inspector UI patch

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

Patch looks fine, but I suggest you ask for review from Däo as well, since he's the expert on CSS.

Also, please ask shorlander about the updated UI.

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch

Update style inspector UI patch

Comment 29

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch

Rebased

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 2

Rebased

Comment 31

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: shorlanders mockup

shorlander's new mockup.

Comment 32

[Stephen Horlander [:shorlander]]

Comment on attachment 555708 [details] [diff] [review]
Update style inspector UI patch 2

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

I have been using this pretty extensively for a week or so and I really like where it is headed!

Some things that could be improved are scanability/readability and finding relevant properties.

I talked with Kevin and some things that are slated that will help this; only showing modified properties by default and adding a filter bar. Which will address most of my issues with actually finding what you are after.

In addition to that there are some visual and layout changes that could make things better:


*Visual/Layout*

• "Selected Element" header is redundant since that information is tied to the Highlighter/breadcrumb
  - It doesn't exactly hurt to have it, but it isn't necessarily needed. If we do keep it we should style it differently to set it off from the rest.

• Collapsed Items should be one line instead of two :
  
  > (property, value, applied rules)
  
  instead of

  (property, value)
    > (applied rules)

  - This will make the list less cumbersome and more streamlined
  - Also it creates a direct relationship between the property you are looking for and expanding that property for attached rules.

• Element Names should expand items not link to MDC
  - Secondary arrow (or perhaps help icon?) should link to MDC
  - Links to MDC should open in tabs instead of new windows
    - Related: It should not open new instances of an already open page
  - The link arrow could only be shown on hover (don't tell limi)

• Removing "Unmatched Rules"
  - This is clearly contentious but I think this adds more noise and complexity than usefulness.

• Color:
  - Different colors for property name and property value
    - Bold handles this effectively right now but dropping the bold would have nice horizontal space savings
    - Color the rule name and the value differently (see mockup)

• Zebra Striping
  - This can help breakup up lines in long lists


*Bugs*

I found some things that don't appear to be working. Not sure if they are directly related to this patch or if they are bugs in dependent pieces.

• Stylesheet links don't actually go to the line number
• The property link flickers the URL overlay and link-text underline when hovered
• The word "One" is listed instead of the number when there is only one Unmatched rule
• You can't expand the Unmatched rules with the disclosure triangle when they are under the matched rules
• You can't collapse the Unmatched rules if you expanded them under the Matched rules
• Expanding the Rules tree switches the cursor from hand to text-select

Comment 33

[Stephen Horlander [:shorlander]]

Attachment: shorlanders mockup - i02

Comment 34

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The new mockup gives some extra insight but:

> • Removing "Unmatched Rules"
>   - This is clearly contentious but I think this adds more noise and complexity than usefulness.

Until we have the style doctor this is the only way to see these rules. Obviously removing this is still under debate.

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We also have Bug 674485 - Style inspector is missing a color legend for it's rule colors. I guess that this should probably be done as part of the UI refresh as at the moment the meaning of the different colors is not discoverable.

[#000] Best match
strikethrough Overridden Match
[#666] Parent match
[brown] Unmatched

I would also say that the "selected element" header and the "legend" footer should be fixed.

It seems like we are going to keep unmatched rules, at least until the CSS Doctor is ready. It is important to know that the closest unmatched selectors are at the top of the list. I am guessing that grey & green number plates will work best in this case.

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 3

Now it looks nice

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 564223 [details] [diff] [review]
Update style inspector UI patch 3

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

> .styleInspector {
>   min-width: 350px;
>+  overflow: none;
> }

overflow has no such value.

>+  // Toggle for zebra striping
>+  _darkStripe: false,

:nth-child doesn't work here?

>+    icon.style.top = (target.getBoundingClientRect().top + 5) + "px";

This looks quirky.

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 4

(In reply to Dão Gottwald [:dao] from comment #37)
> Comment on attachment 564223 [details] [diff] [review] [diff] [details] [review]
> Update style inspector UI patch 3
> 
> >--- a/browser/base/content/browser.css
> >+++ b/browser/base/content/browser.css
> 
> > .styleInspector {
> >   min-width: 350px;
> >+  overflow: none;
> > }
> 
> overflow has no such value.
> 

/me almost dies of embarrassment
I obviously meant hidden but this means it is not needed ... removed.

> >+  // Toggle for zebra striping
> >+  _darkStripe: false,
> 
> :nth-child doesn't work here?
> 

I wish it would. Unfortunately if rows are hidden (via the style filter or "Only user styles" checkbox then :nth-child will no longer work.

> >+    icon.style.top = (target.getBoundingClientRect().top + 5) + "px";
> 
> This looks quirky.

I have now changed this to vertically center the icon in the current row and removed the magic number. I would have used a css background image and an attribute to toggle display of the icon but unfortunately the icon needs to be able to receive a click event. Having a single icon with a single event that we move over each row on mouseover is more efficient that adding 300 of them and slowing down the UI.

Thanks for the review

Comment 39

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 5

Removed unused property

Comment 40

[Mihai Sucan [:msucan]]

Comment on attachment 564515 [details] [diff] [review]
Update style inspector UI patch 5

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

General comments:

- this UI refresh looks awesome! WOW! Great work!
- the cursor:pointer is over-used whenever I move the mouse. See my comments below.
- please remove all trailing spaces in this patch.
- there are some performance concerns, but these need to be addressed when the patch for bug 685902 is ready. Basically, I have in mind how to minimize the calls to hasUnmatchedSelectors() and coalesce them into one faster call.
- the arrow icon for the MDN link should perhaps be a "?", not an arrow. This is confirmed by shorlander (pinged him on IRC), but the latest mockup we have in this bug does indeed show an arrow...

More comments below.

Looking forward for the updated patch. Thank you!

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +221,5 @@
>              this.win.setTimeout(displayProperties.bind(this), 50);
>            } else {
>              this.htmlComplete = true;
> +            this.gotoMdnIcon.addEventListener("click", function() {
> +              this.win.open(PropertyView.link, "MDNWindow");

This solution is not ideal. Please see my comments for csshtmltree.xhtml.

@@ +482,5 @@
>  
>    /**
> +   * Add zebra striping to the element
> +   */
> +  get stripe()

This is not an ideal solution. In the className getter you should add a class for the stripe.

@@ +525,5 @@
> +        this.unmatchedSelectorContainer.innerHTML = "";
> +        this.unmatchedExpander.removeAttribute("open");
> +      }
> +
> +      this.matchedSelectorsContainer.innerHTML = "";

These bits of code are confusing.

I would expect:

- make the property expandable if there are matched or unmatched selectors. Do not call hasUnmatchedSelectors() if hasMatchedSelectors() returns true - to keep execution fast.
- show the matched selectors table when there are such selectors, after the user clicks the property name.
- show the "unmatched selectors" title if there are unmatched selectors. Until this point here the code should never call hasUnmatchedSelectors() - unless hasMatchedSelectors() returned false in the first step (as explained above).
- show the unmatched selectors table when the user expands the unmatched selectors view.

The code confusion with the two containers for unmatched selectors is unneeded. I would suggest that you use only one container for unmatched selectors and hide the "unmatched selectors" title when there are no matched selectors - to make it look like now.

@@ +549,5 @@
> +      this.matchedExpander.style.visibility = "";
> +      this.element.style.cursor = "pointer";
> +    } else {
> +      this.matchedExpander.style.visibility = "hidden";
> +      this.element.style.cursor = "";

The this.element.style.cursor = "pointer" line causes the mouse to be a pointer all over the place. You want only the .property-header to have cursor:pointer.

I would suggest to use a className "expandable" for this.element. From CSS you can change the expander visibility and the .property-header cursor.

@@ +566,5 @@
>  
>    /**
>     * Refresh the panel unmatched rules.
>     */
> +  refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors() {

The code here is also confusing because of the unmatched selectors containers...

@@ +636,5 @@
>  
>    /**
>     * The action when a user expands matched selectors.
>     */
>    matchedSelectorsClick: function PropertyView_matchedSelectorsClick(aEvent)

This should be renamed to propertyHeaderClick.

@@ +641,5 @@
>    {
>      this.matchedExpanded = !this.matchedExpanded;
>      this.refreshMatchedSelectors();
> +
> +    if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {

Is this needed?

There's some confusion:
- when PropertyView instances are constructed the refreshMatchedSelectors() is invoked.
- when PropertyView.refresh() is called both refreshMatchedSelectors() and refreshUnmatchedSelectors() are invoked.
- when the user clicks the property header the refreshMatchedSelectors() method is invoked, and optionally refreshUnmatchedSelectors().
- when refreshMatchedSelectors() executes, if there are any matched selectors, refreshUnmatchedSelectors() is also invoked.

Please make these things clearer. Thank you!

@@ +671,5 @@
> +    
> +    // Having a single icon with a single event that we move over each row on
> +    // mouseover is more efficient that adding 300 of them and slowing the UI
> +    icon.style.top = rect.top + rect.height / 2 - icon.clientHeight / 2
> +                     - MDN_ICON_HEIGHT / 2 + "px";

This code somehow makes the MDN icon jump around a few pixels when I move the mouse from the left to right (and back).

Please use the solution suggested in the comments for csshtmltree.xhtml. Thanks!

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +69,5 @@
>      let ns = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>      let panel = win.document.createElementNS(ns, "panel");
>  
> +    function SI_addToLegend(aContainer, aClass, aText) {
> +      let span = win.document.createElement("span");

Please use a XUL description element.

The span you create is in the XUL namespace, which does not have any span element defined (afaik).

@@ +70,5 @@
>      let panel = win.document.createElementNS(ns, "panel");
>  
> +    function SI_addToLegend(aContainer, aClass, aText) {
> +      let span = win.document.createElement("span");
> +      span.setAttribute("class", "styleinspector-legendKey " + aClass);

Please check if you can use element.className instead of setAttribute() here.

@@ +105,1 @@
>      vbox.appendChild(hbox);

There's some confusion here with variable names.

Please rename "vbox" to something like "mainContainer" and "hbox" to "resizerContainer". Also rename the .styleinspector-resizerbox class name accordingly.

@@ +105,3 @@
>      vbox.appendChild(hbox);
>  
> +    let legend = win.document.createElement("div");

Please use a XUL hbox element.

Afaik there is no div element defined in the XUL namespace.

@@ +110,2 @@
>      let spacer = win.document.createElement("spacer");
>      spacer.setAttribute("flex", "1");

Please use spacer.flex = 1.

(same below)

@@ +117,5 @@
> +        StyleInspector.l10n("rule.status.MATCHED"));
> +    SI_addToLegend(legend, "styleinspector-parentmatch",
> +        StyleInspector.l10n("rule.status.PARENT_MATCH"));
> +    SI_addToLegend(legend, "styleinspector-unmatched",
> +        StyleInspector.l10n("rule.status.UNMATCHED"));

You could put the l10n() call in SI_addToLegend(). You can also put the "rule.status." and "styleinspector-" prefixes there.

@@ +119,5 @@
> +        StyleInspector.l10n("rule.status.PARENT_MATCH"));
> +    SI_addToLegend(legend, "styleinspector-unmatched",
> +        StyleInspector.l10n("rule.status.UNMATCHED"));
> +
> +    let spacer = win.document.createElement("spacer");

There's another let spacer = ... above. Please remove the repeated "let" here.

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +128,5 @@
>      property: ... // PropertyView from CssHtmlTree.jsm
>    }
>    -->
>    <div id="templateProperty">
> +    <div class="${className}" save="${element}" stripe="${stripe}" dir="${getRTLAttr}">

The ${stripe} can be merged into the ${className} code.

@@ +137,2 @@
>          <div save="${valueNode}" class="property-value" dir="ltr">${value}</div>
>        </div>

I would prefer a new element inside div.property-header for the MDN link. So you don't need onmouseover/out, you can do it with CSS.

The new element can be something like <div class="property-doc-link"><a href="${mdnLink}" target="_blank" title="&helpLinkTitle;">&helpLinkTitle;</a></div>. So you don't need an onclick event handler either.

::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
@@ +217,5 @@
> +  font-weight: bold;
> +}
> +
> +.property-view[stripe="dark"] {
> +   background-color: #EEE;

This color is too dark, please make it lighter, as in the mockup Stephen has attached to the bug.

::: browser/themes/pinstripe/browser/devtools/csshtmltree.css
@@ +200,5 @@
> +
> +#header {
> +  background-color: #DCE2E8;
> +  padding: 5px 5px 0 5px;
> +  height: 70px;

Height is fixed here which breaks the layout when the selected element path is very long. Try it on ubuntu.com or github.com when the Style Panel width is default. You need to allow the header height to grow when the path is multiple lines due to wrapping.

@@ +206,5 @@
> +
> +#propertyContainer {
> +  position: absolute;
> +  overflow-y: auto;
> +  top: 75px;

Same as above.

(you should not need position:absolute)

Comment 41

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 6

(In reply to Mihai Sucan [:msucan] from comment #40)
> Comment on attachment 564515 [details] [diff] [review] [diff] [details] [review]
> Update style inspector UI patch 5
> 
> Review of attachment 564515 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - this UI refresh looks awesome! WOW! Great work!
> - the cursor:pointer is over-used whenever I move the mouse. See my comments
> below.
> - please remove all trailing spaces in this patch.

Done

> - there are some performance concerns, but these need to be addressed when
> the patch for bug 685902 is ready. Basically, I have in mind how to minimize
> the calls to hasUnmatchedSelectors() and coalesce them into one faster call.

Yes, I am expecting that.

> - the arrow icon for the MDN link should perhaps be a "?", not an arrow.
> This is confirmed by shorlander (pinged him on IRC), but the latest mockup
> we have in this bug does indeed show an arrow...
> 

Changed

> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +221,5 @@
> >              this.win.setTimeout(displayProperties.bind(this), 50);
> >            } else {
> >              this.htmlComplete = true;
> > +            this.gotoMdnIcon.addEventListener("click", function() {
> > +              this.win.open(PropertyView.link, "MDNWindow");
> 
> This solution is not ideal. Please see my comments for csshtmltree.xhtml.
> 

Fixed

> @@ +482,5 @@
> >  
> >    /**
> > +   * Add zebra striping to the element
> > +   */
> > +  get stripe()
> 
> This is not an ideal solution. In the className getter you should add a
> class for the stripe.
> 

Done

> @@ +525,5 @@
> > +        this.unmatchedSelectorContainer.innerHTML = "";
> > +        this.unmatchedExpander.removeAttribute("open");
> > +      }
> > +
> > +      this.matchedSelectorsContainer.innerHTML = "";
> 
> These bits of code are confusing.
> 
> I would expect:
> 
> - make the property expandable if there are matched or unmatched selectors.
> Do not call hasUnmatchedSelectors() if hasMatchedSelectors() returns true -
> to keep execution fast.
> - show the matched selectors table when there are such selectors, after the
> user clicks the property name.
> - show the "unmatched selectors" title if there are unmatched selectors.
> Until this point here the code should never call hasUnmatchedSelectors() -
> unless hasMatchedSelectors() returned false in the first step (as explained
> above).
> - show the unmatched selectors table when the user expands the unmatched
> selectors view.
> 

hasUnmatchedSelectors() is needed to decide whether to decide whether to display the unmatched selectors block (or at least the title / matched selector arrow) so never calling hasMatchedSelectors() is not currently possible as far as I can see. This means that whenever refreshMatchedSelectors is called we now also need to call refreshUnmatchedSelectors (hence the refreshAllSelectors method).

> The code confusion with the two containers for unmatched selectors is
> unneeded. I would suggest that you use only one container for unmatched
> selectors and hide the "unmatched selectors" title when there are no matched
> selectors - to make it look like now.
> 

Done

> @@ +549,5 @@
> > +      this.matchedExpander.style.visibility = "";
> > +      this.element.style.cursor = "pointer";
> > +    } else {
> > +      this.matchedExpander.style.visibility = "hidden";
> > +      this.element.style.cursor = "";
> 
> The this.element.style.cursor = "pointer" line causes the mouse to be a
> pointer all over the place. You want only the .property-header to have
> cursor:pointer.
> 
> I would suggest to use a className "expandable" for this.element. From CSS
> you can change the expander visibility and the .property-header cursor.
> 

Done

> @@ +566,5 @@
> >  
> >    /**
> >     * Refresh the panel unmatched rules.
> >     */
> > +  refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors() {
> 
> The code here is also confusing because of the unmatched selectors
> containers...
> 

We only use one container now but I am not sure that it is any easier to read.

> @@ +636,5 @@
> >  
> >    /**
> >     * The action when a user expands matched selectors.
> >     */
> >    matchedSelectorsClick: function PropertyView_matchedSelectorsClick(aEvent)
> 
> This should be renamed to propertyHeaderClick.
> 

Done

> @@ +641,5 @@
> >    {
> >      this.matchedExpanded = !this.matchedExpanded;
> >      this.refreshMatchedSelectors();
> > +
> > +    if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {
> 
> Is this needed?
> 

Yes it is. When the property header is clicked it either expands matched or unmatched selectors. The condition to check this is !this.hasMatchedSelectors && this.hasUnmatchedSelectors.

> There's some confusion:
> - when PropertyView instances are constructed the refreshMatchedSelectors()
> is invoked.
> - when PropertyView.refresh() is called both refreshMatchedSelectors() and
> refreshUnmatchedSelectors() are invoked.
> - when the user clicks the property header the refreshMatchedSelectors()
> method is invoked, and optionally refreshUnmatchedSelectors().
> - when refreshMatchedSelectors() executes, if there are any matched
> selectors, refreshUnmatchedSelectors() is also invoked.
> 
> Please make these things clearer. Thank you!
> 

Done

> @@ +671,5 @@
> > +    
> > +    // Having a single icon with a single event that we move over each row on
> > +    // mouseover is more efficient that adding 300 of them and slowing the UI
> > +    icon.style.top = rect.top + rect.height / 2 - icon.clientHeight / 2
> > +                     - MDN_ICON_HEIGHT / 2 + "px";
> 
> This code somehow makes the MDN icon jump around a few pixels when I move
> the mouse from the left to right (and back).
> 
> Please use the solution suggested in the comments for csshtmltree.xhtml.
> Thanks!
> 

I couldn't reproduce this so obviously Fedora is better than Ubuntu ;o)

Done ... we now use the CSS solution and a little click hacketry.

> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +69,5 @@
> >      let ns = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >      let panel = win.document.createElementNS(ns, "panel");
> >  
> > +    function SI_addToLegend(aContainer, aClass, aText) {
> > +      let span = win.document.createElement("span");
> 
> Please use a XUL description element.
> 
> The span you create is in the XUL namespace, which does not have any span
> element defined (afaik).
> 

Done

> @@ +70,5 @@
> >      let panel = win.document.createElementNS(ns, "panel");
> >  
> > +    function SI_addToLegend(aContainer, aClass, aText) {
> > +      let span = win.document.createElement("span");
> > +      span.setAttribute("class", "styleinspector-legendKey " + aClass);
> 
> Please check if you can use element.className instead of setAttribute() here.
> 

Done

> @@ +105,1 @@
> >      vbox.appendChild(hbox);
> 
> There's some confusion here with variable names.
> 
> Please rename "vbox" to something like "mainContainer" and "hbox" to
> "resizerContainer". Also rename the .styleinspector-resizerbox class name
> accordingly.
> 

Done

> @@ +105,3 @@
> >      vbox.appendChild(hbox);
> >  
> > +    let legend = win.document.createElement("div");
> 
> Please use a XUL hbox element.
> 
> Afaik there is no div element defined in the XUL namespace.
> 

Fixed

> @@ +110,2 @@
> >      let spacer = win.document.createElement("spacer");
> >      spacer.setAttribute("flex", "1");
> 
> Please use spacer.flex = 1.
> 
> (same below)
> 

Changed (I changed all of them in this file).

> @@ +117,5 @@
> > +        StyleInspector.l10n("rule.status.MATCHED"));
> > +    SI_addToLegend(legend, "styleinspector-parentmatch",
> > +        StyleInspector.l10n("rule.status.PARENT_MATCH"));
> > +    SI_addToLegend(legend, "styleinspector-unmatched",
> > +        StyleInspector.l10n("rule.status.UNMATCHED"));
> 
> You could put the l10n() call in SI_addToLegend().

Done

> You can also put the "rule.status." and "styleinspector-" prefixes there.

Done

> 
> @@ +119,5 @@
> > +        StyleInspector.l10n("rule.status.PARENT_MATCH"));
> > +    SI_addToLegend(legend, "styleinspector-unmatched",
> > +        StyleInspector.l10n("rule.status.UNMATCHED"));
> > +
> > +    let spacer = win.document.createElement("spacer");
> 
> There's another let spacer = ... above. Please remove the repeated "let"
> here.
> 

Done

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +128,5 @@
> >      property: ... // PropertyView from CssHtmlTree.jsm
> >    }
> >    -->
> >    <div id="templateProperty">
> > +    <div class="${className}" save="${element}" stripe="${stripe}" dir="${getRTLAttr}">
> 
> The ${stripe} can be merged into the ${className} code.
> 

Done

> @@ +137,2 @@
> >          <div save="${valueNode}" class="property-value" dir="ltr">${value}</div>
> >        </div>
> 
> I would prefer a new element inside div.property-header for the MDN link. So
> you don't need onmouseover/out, you can do it with CSS.
> 
> The new element can be something like <div class="property-doc-link"><a
> href="${mdnLink}" target="_blank"
> title="&helpLinkTitle;">&helpLinkTitle;</a></div>. So you don't need an
> onclick event handler either.
> 

Done

> ::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
> @@ +217,5 @@
> > +  font-weight: bold;
> > +}
> > +
> > +.property-view[stripe="dark"] {
> > +   background-color: #EEE;
> 
> This color is too dark, please make it lighter, as in the mockup Stephen has
> attached to the bug.
> 

Done

> ::: browser/themes/pinstripe/browser/devtools/csshtmltree.css
> @@ +200,5 @@
> > +
> > +#header {
> > +  background-color: #DCE2E8;
> > +  padding: 5px 5px 0 5px;
> > +  height: 70px;
> 
> Height is fixed here which breaks the layout when the selected element path
> is very long. Try it on ubuntu.com or github.com when the Style Panel width
> is default. You need to allow the header height to grow when the path is
> multiple lines due to wrapping.
> 
> @@ +206,5 @@
> > +
> > +#propertyContainer {
> > +  position: absolute;
> > +  overflow-y: auto;
> > +  top: 75px;
> 
> Same as above.
> 
> (you should not need position:absolute)

Fixed using CSS3 Flexbox.

Comment 42

[Mihai Sucan [:msucan]]

Comment on attachment 565170 [details] [diff] [review]
Update style inspector UI patch 6

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

Thanks for your patch update! Much improved.

General comments:

- Please ask Dão for CSS review as well.
- The "unmatched selectors" title stays visible even when there are no unmatched selectors. (this might be the container staying visible, actually, when it shouldn't, see comments below)
- Looking at Stephen's comment 32: please make the MDN link open in a new tab, instead of a new window.

Giving r- for the remaining comments to be addressed. Very close to where we want this to be! Looking forward for the updated patch. Thank you!

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +480,5 @@
> +      this.tree._darkStripe = !this.tree._darkStripe;
> +      return "property-view" + darkValue;
> +    } else {
> +      return "property-view-hidden";
> +    }

I expected you'd add a class name, not have "property-view-dark".

Nit: Mozilla's code style suggests:

"Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level."

@@ +486,5 @@
> +
> +  /**
> +   * Returns the MDN link for the property
> +   */
> +  get mdnLink()

This is not needed. I forgot in my previous comment we had .link. You can directly use .link in csshtmltree.xhtml.

(the previous comment mainly pointed out a solution rather requiring specific property names.)

@@ +508,5 @@
> +   * @param aNode The node to which the open attribute is to be added
> +   */
> +  closeNode: function PropertyView__openElement(aNode)
> +  {
> +    aNode.removeAttribute("open");

openNode() and closeNode() are not needed.

@@ +553,5 @@
> +      this.matchedExpander.style.visibility = "";
> +      this.propertyHeader.classList.add("expandable");
> +    } else {
> +      this.matchedExpander.style.visibility = "hidden";
> +      this.propertyHeader.classList.remove("expandable");

From CSS you can change the .matchedExpander visibility when the .propertyHeader element has the .expandable class as well.

@@ +571,5 @@
>     * Refresh the panel unmatched rules.
>     */
>    refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors()
>    {
>      let hasUnmatchedSelectors = this.hasUnmatchedSelectors;

Please don't call this here, for performance reasons.

@@ +577,5 @@
>  
> +    this.unmatchedSelectorTable.hidden = !this.unmatchedExpanded;
> +
> +    if (hasMatchedSelectors) {
> +      this.unmatchedSelectorsContainer.hidden = hasUnmatchedSelectors && !this.matchedExpanded;

Shouldn't this be:

this.unmatchedSelectorsContainer.hidden = !this.matchedExpanded || !this.hasUnmatchedSelectors;

?

(Hopefully) it's less confusing: when you have matched selectors, unmatched selectors are hidden only if matchedExpanded is false, or when there are no unmatched selectors.

@@ +580,5 @@
> +    if (hasMatchedSelectors) {
> +      this.unmatchedSelectorsContainer.hidden = hasUnmatchedSelectors && !this.matchedExpanded;
> +    } else {
> +      this.unmatchedSelectorsContainer.hidden = !this.unmatchedExpanded;
> +      this.unmatchedTitleBlock.hidden = true;

I think you also need to add to the if (hasMatchedSelectors) { block above } ... the line:

this.unmatchedTitleBlock.hidden = false;

@@ +583,5 @@
> +      this.unmatchedSelectorsContainer.hidden = !this.unmatchedExpanded;
> +      this.unmatchedTitleBlock.hidden = true;
> +    }
> +
> +    if (hasUnmatchedSelectors && this.unmatchedExpanded) {

Please change to:

if (this.unmatchedExpanded && this.hasUnmatchedSelectors) {

@@ +588,2 @@
>        CssHtmlTree.processTemplate(this.templateUnmatchedSelectors,
> +      this.unmatchedSelectorTable, this);

Nit: add two spaces for indentation.

@@ +603,4 @@
>      }
>    },
>  
> +  refreshAllSelectors: function PropertyView_refreshAllSelectors()

Please add a comment.

@@ +652,2 @@
>    {
> +    if (aEvent.rangeParent.className != "helplink") {

Why rangeParent? Shouldn't target work?

https://developer.mozilla.org/en/DOM/event

@@ +654,5 @@
> +      this.matchedExpanded = !this.matchedExpanded;
> +      this.refreshAllSelectors();
> +
> +      if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {
> +        this.unmatchedSelectorsClick(aEvent);

This will call refreshUnmatchedSelectors() twice.

I suggest the following code for the propertyHeaderClick() method:

propertyHeaderClick: function PropertyView_propertyHeaderClick(aEvent)
{
  if (aEvent.rangeParent.className != "helplink") {
    this.matchedExpanded = !this.matchedExpanded;
    if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {
      this.unmatchedExpanded = !this.unmatchedExpanded;
    }
    this.refreshAllSelectors();
    aEvent.preventDefault();
  }
}

Thoughts?

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +74,5 @@
> +      description.className = "styleinspector-legendKey styleinspector-" + aClass;
> +      aContainer.appendChild(description);
> +
> +      let text = StyleInspector.l10n("rule.status." + aText);
> +      let textNode =  win.document.createTextNode(text);

Nit: double space.

@@ +90,5 @@
>      panel.setAttribute("width", 350);
>      panel.setAttribute("height", win.screen.height / 2);
>  
> +    let mainContainer = win.document.createElement("vbox");
> +    mainContainer.setAttribute("flex", "1");

mainContainer.flex = 1;

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +129,5 @@
> +        <div class="property-name" dir="${getRTLAttr}">${name}</div>
> +        <div class="property-doc-link">
> +          <a href="${mdnLink}" class="helplink" target="_blank">
> +            <img src="chrome://browser/skin/devtools/goto-mdn.png"
> +                 alt="&helpLinkTitle;" title="&helpLinkTitle;"></img>

Don't use an <img> here. Just put &helpLinkTitle; in the anchor. You can style the anchor from CSS to render the icon.

::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
@@ +158,5 @@
> +  display: inline-block;
> +  visibility: hidden;
> +}
> +
> +.property-header:hover .property-doc-link {

.property-header:hover > .property-doc-link {

Comment 43

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 7

(In reply to Mihai Sucan [:msucan] from comment #42)
> Comment on attachment 565170 [details] [diff] [review] [diff] [details] [review]
> Update style inspector UI patch 6
> 
> Review of attachment 565170 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your patch update! Much improved.
> 
> General comments:
> 
> - Please ask Dão for CSS review as well.

Will do

> - The "unmatched selectors" title stays visible even when there are no
> unmatched selectors. (this might be the container staying visible, actually,
> when it shouldn't, see comments below)

I couldn't reproduce this but maybe addressing your comments has fixed it.

> - Looking at Stephen's comment 32: please make the MDN link open in a new
> tab, instead of a new window.
> 

Done ... it now opens in a new tab unless it is already open (in which case the tab is focused). Actually, I like this a lot.

> Giving r- for the remaining comments to be addressed. Very close to where we
> want this to be! Looking forward for the updated patch. Thank you!
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +480,5 @@
> > +      this.tree._darkStripe = !this.tree._darkStripe;
> > +      return "property-view" + darkValue;
> > +    } else {
> > +      return "property-view-hidden";
> > +    }
> 
> I expected you'd add a class name, not have "property-view-dark".
> 

Done, also prevented any concats to save a cycle or two.

> Nit: Mozilla's code style suggests:
> 
> "Don't put an else right after a return. Delete the else, it's unnecessary
> and increases indentation level."
> 

Fixed

> @@ +486,5 @@
> > +
> > +  /**
> > +   * Returns the MDN link for the property
> > +   */
> > +  get mdnLink()
> 
> This is not needed. I forgot in my previous comment we had .link. You can
> directly use .link in csshtmltree.xhtml.
> 
> (the previous comment mainly pointed out a solution rather requiring
> specific property names.)
> 

hehe ... of course. Fixed.

> @@ +508,5 @@
> > +   * @param aNode The node to which the open attribute is to be added
> > +   */
> > +  closeNode: function PropertyView__openElement(aNode)
> > +  {
> > +    aNode.removeAttribute("open");
> 
> openNode() and closeNode() are not needed.
> 

In my opinion these methods made things easier to read but ... removed.

> @@ +553,5 @@
> > +      this.matchedExpander.style.visibility = "";
> > +      this.propertyHeader.classList.add("expandable");
> > +    } else {
> > +      this.matchedExpander.style.visibility = "hidden";
> > +      this.propertyHeader.classList.remove("expandable");
> 
> From CSS you can change the .matchedExpander visibility when the
> .propertyHeader element has the .expandable class as well.
> 

Of course, done.

> @@ +571,5 @@
> >     * Refresh the panel unmatched rules.
> >     */
> >    refreshUnmatchedSelectors: function PropertyView_refreshUnmatchedSelectors()
> >    {
> >      let hasUnmatchedSelectors = this.hasUnmatchedSelectors;
> 
> Please don't call this here, for performance reasons.
> 

Done

> @@ +577,5 @@
> >  
> > +    this.unmatchedSelectorTable.hidden = !this.unmatchedExpanded;
> > +
> > +    if (hasMatchedSelectors) {
> > +      this.unmatchedSelectorsContainer.hidden = hasUnmatchedSelectors && !this.matchedExpanded;
> 
> Shouldn't this be:
> 
> this.unmatchedSelectorsContainer.hidden = !this.matchedExpanded ||
> !this.hasUnmatchedSelectors;
> 
> ?
> 
> (Hopefully) it's less confusing: when you have matched selectors, unmatched
> selectors are hidden only if matchedExpanded is false, or when there are no
> unmatched selectors.
> 

Done

> @@ +580,5 @@
> > +    if (hasMatchedSelectors) {
> > +      this.unmatchedSelectorsContainer.hidden = hasUnmatchedSelectors && !this.matchedExpanded;
> > +    } else {
> > +      this.unmatchedSelectorsContainer.hidden = !this.unmatchedExpanded;
> > +      this.unmatchedTitleBlock.hidden = true;
> 
> I think you also need to add to the if (hasMatchedSelectors) { block above }
> ... the line:
> 
> this.unmatchedTitleBlock.hidden = false;
> 

Done

> @@ +583,5 @@
> > +      this.unmatchedSelectorsContainer.hidden = !this.unmatchedExpanded;
> > +      this.unmatchedTitleBlock.hidden = true;
> > +    }
> > +
> > +    if (hasUnmatchedSelectors && this.unmatchedExpanded) {
> 
> Please change to:
> 
> if (this.unmatchedExpanded && this.hasUnmatchedSelectors) {
> 

Can't believe I missed that, done.

> @@ +588,2 @@
> >        CssHtmlTree.processTemplate(this.templateUnmatchedSelectors,
> > +      this.unmatchedSelectorTable, this);
> 
> Nit: add two spaces for indentation.
> 

Done

> @@ +603,4 @@
> >      }
> >    },
> >  
> > +  refreshAllSelectors: function PropertyView_refreshAllSelectors()
> 
> Please add a comment.
> 

Done

> @@ +652,2 @@
> >    {
> > +    if (aEvent.rangeParent.className != "helplink") {
> 
> Why rangeParent? Shouldn't target work?
> 
> https://developer.mozilla.org/en/DOM/event
> 

After moving the image into CSS this makes more sense ... done

> @@ +654,5 @@
> > +      this.matchedExpanded = !this.matchedExpanded;
> > +      this.refreshAllSelectors();
> > +
> > +      if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {
> > +        this.unmatchedSelectorsClick(aEvent);
> 
> This will call refreshUnmatchedSelectors() twice.
> 
> I suggest the following code for the propertyHeaderClick() method:
> 
> propertyHeaderClick: function PropertyView_propertyHeaderClick(aEvent)
> {
>   if (aEvent.rangeParent.className != "helplink") {
>     this.matchedExpanded = !this.matchedExpanded;
>     if (!this.hasMatchedSelectors && this.hasUnmatchedSelectors) {
>       this.unmatchedExpanded = !this.unmatchedExpanded;
>     }
>     this.refreshAllSelectors();
>     aEvent.preventDefault();
>   }
> }
> 
> Thoughts?
> 

Yes, that makes sense. Done.

> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +74,5 @@
> > +      description.className = "styleinspector-legendKey styleinspector-" + aClass;
> > +      aContainer.appendChild(description);
> > +
> > +      let text = StyleInspector.l10n("rule.status." + aText);
> > +      let textNode =  win.document.createTextNode(text);
> 
> Nit: double space.
> 

Ew ... fixed.

> @@ +90,5 @@
> >      panel.setAttribute("width", 350);
> >      panel.setAttribute("height", win.screen.height / 2);
> >  
> > +    let mainContainer = win.document.createElement("vbox");
> > +    mainContainer.setAttribute("flex", "1");
> 
> mainContainer.flex = 1;
> 

Done

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +129,5 @@
> > +        <div class="property-name" dir="${getRTLAttr}">${name}</div>
> > +        <div class="property-doc-link">
> > +          <a href="${mdnLink}" class="helplink" target="_blank">
> > +            <img src="chrome://browser/skin/devtools/goto-mdn.png"
> > +                 alt="&helpLinkTitle;" title="&helpLinkTitle;"></img>
> 
> Don't use an <img> here. Just put &helpLinkTitle; in the anchor. You can
> style the anchor from CSS to render the icon.
> 

Done

> ::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
> @@ +158,5 @@
> > +  display: inline-block;
> > +  visibility: hidden;
> > +}
> > +
> > +.property-header:hover .property-doc-link {
> 
> .property-header:hover > .property-doc-link {

Done

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 566798 [details] [diff] [review]
Update style inspector UI patch 7

>+  mdnLinkClick: function PropertyView_mdnLinkClick(aEvent)
>+  {
>+    let browserEnumerator = Services.wm.getEnumerator("navigator:browser");
>+
>+    let found = false;
>+    while (!found && browserEnumerator.hasMoreElements()) {
>+      let browserWin = browserEnumerator.getNext();
>+      let tabbrowser = browserWin.gBrowser;
>+
>+      let numTabs = tabbrowser.browsers.length;
>+      for (let index = 0; index < numTabs; index++) {
>+        let currentBrowser = tabbrowser.getBrowserAtIndex(index);
>+        if (this.link == currentBrowser.currentURI.spec) {
>+          tabbrowser.selectedTab = tabbrowser.tabContainer.childNodes[index];
>+          browserWin.focus();
>+          found = true;

Please don't do this. It will fail unexpectedly when the target page happens to redirect to a slightly different URI. We actually have a utility function for this (switchToTabHavingURIs), but we only use it for pages under the client's control (e.g. about:addons).

>+      this.tree.win.delayedOpenTab(this.link, null, null, null, null);

Use openUILinkIn instead of delayedOpenTab.

Comment 45

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 8

Addressed Dão's comments.

Comment 46

[Mihai Sucan [:msucan]]

Comment on attachment 567050 [details] [diff] [review]
Update style inspector UI patch 8

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

Thanks for the patch update! Patch looks fine for me now. All tests pass. Giving it r+ with the comments below properly addressed.

(also thank you Dão for your comments on the patch!)

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +649,5 @@
>  
>    /**
>     * The action when a user expands matched selectors.
>     */
> +  propertyHeaderClick: function PropertyView_propertyHeaderClick(aEvent)

Please add @param info.

@@ +673,5 @@
>    },
> +
> +  /**
> +     * The action when a user clicks on the MDN help link for a property.
> +     */

Nit: wrong indentation. Also please add a @param.

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +128,5 @@
> +        <div save="${matchedExpander}" class="match expander"></div>
> +        <div class="property-name" dir="${getRTLAttr}">${name}</div>
> +        <div class="helplink" alt="&helpLinkTitle;" title="&helpLinkTitle;"
> +             onclick="${mdnLinkClick}">
> +          <a href="${link}" class="helplink"></a>

DIV elements do not have the |alt| attribute, move the title to the anchor, change the anchor to hold &helpLinkTitle and remove the .helplink class from the anchor, then update the stylesheets accordingly.

::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
@@ +105,1 @@
>  }

Apply this to the anchor, not to the DIV. Since I asked you to put text content into the anchor you need to do height: 0; overflow: hidden; padding-top: 14px.

::: browser/themes/winstripe/browser/jar.mn
@@ +230,5 @@
>          skin/classic/aero/browser/tabview/tabview.png                (tabview/tabview.png)
>          skin/classic/aero/browser/tabview/tabview-inverted.png       (tabview/tabview-inverted.png)
>          skin/classic/aero/browser/tabview/tabview.css                (tabview/tabview.css)
>          skin/classic/aero/browser/devtools/arrows.png                (devtools/arrows.png)
> +        skin/classic/aero/devtools/goto-mdn.png                      (devtools/goto-mdn.png)

This is most likely wrong. Shouldn't that be skin/classic/aero/browser/devtools/goto-mdn.png? (like above)

Comment 47

[Mihai Sucan [:msucan]]

Comment on attachment 567050 [details] [diff] [review]
Update style inspector UI patch 8

Dão: can you please review the CSS bits of this patch?

Thank you!

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 567050 [details] [diff] [review]
Update style inspector UI patch 8

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

> .property-header {
>+  padding: 4px 5px 4px 0;

This is wrong for RTL, isn't it?

>+.link,
>+.link:visited {
>+  color: #0091ff;
> }

>-a.link:visited {
>+a.link,
>+a.helplink,
>+a.link:visited,
>+a.helplink:visited {
>   text-decoration: none;
> }

Is there a reason for including the node name ("a") in these selectors?

> .rule-unmatched {
>-  padding: 2px 0;
>+  padding: 2px 0 2px 12px;

RTL again

> .property-name {
>+  width: 220px;
> }

Magic number? :(

>+        skin/classic/aero/devtools/goto-mdn.png                      (devtools/goto-mdn.png)

Yep, this looks broken.

Comment 49

[Mihai Sucan [:msucan]]

Attachment: flexible width for properties

Mike: this is a patch that applies cleanly on top of your patch 8 (attachment 567050 [details] [diff] [review]). As discussed this is one of the possible solutions to make the width of the properties flexible. No JS changes were needed, apart from minimal changes needed to deal with the markup.

Please note this is a WIP patch that shows the solution works (patch is ~70% done). Please clean it up and fold it into your main patch. Minimal work is needed: make some changes to fix a bit of style breakage (I did most of the work) and test it for RTL users. I did the changes in the gnomestripe theme - you need to also make the changes in pinstripe and winstripe.

When done, ask Dão for review. Thank you!

Comment 50

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Shorlanders screenshot

Shorlanders feedback:
1. The appearance of search filters should match the search filters in the bookmark section (platform specific).
2. Expandos should be platform specific.
3. Values should stay in the right-hand column when wrapped.

Comment 51

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Thanks for finding a way Mihai ... I will put all of this together along with Shorlanders feedback tomorrow and then ask Dão for review.

Comment 52

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 9 WIP

Shorlanders feedback:
1. The appearance of search filters should match the search filters in the bookmark section (platform specific).

Done

2. Expandos should be platform specific.

Done

3. Values should stay in the right-hand column when wrapped.

Done

========================================

Patch mostly working but tests appear to leave the DOM in a strange state and then time out.

Before merging Mihai's patch that changes this to use tables all tests ran without issues. Since the conversion to tables everything runs fine in real life but tests totally break.

It seems like something is getting called recursively when running tests ... platform bug maybe?

Comment 53

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #52)

> Patch mostly working but tests appear to leave the DOM in a strange state
> and then time out.
> 
> Before merging Mihai's patch that changes this to use tables all tests ran
> without issues. Since the conversion to tables everything runs fine in real
> life but tests totally break.

All tests pass here with the exception of browser_styleinspector_bug_672746_default_styles.js which has a minor failure.  The following fix is needed in propertyVisible():

     if (propView.name == aName) {
-      return propView.className != "property-view-hidden";
+      return propView.visible;
     }

After this change, all tests pass. (Linux here)


> It seems like something is getting called recursively when running tests ...
> platform bug maybe?

On what OS do the tests fail? It's surprising for the tests to fail since the UI changes you/we did are minimal. The switch to a fluid layout that shouldn't have any impact on the tests...

Btw, I really like how this looks. Awesome work!

Comment 54

[Mihai Sucan [:msucan]]

Comment on attachment 569053 [details] [diff] [review]
Update style inspector UI patch 9 WIP

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

Just a quick comment below:

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +146,5 @@
> +    userStylesCheckbox.id = "onlyUserStylesCheckbox";
> +    userStylesCheckbox.setAttribute("label",
> +      this.l10n("style.userStylesLabel"));
> +    userStylesCheckbox.setAttribute("checked", "true");
> +    userStylesCheckbox.className = ".styleinspector-onlyuserstyles";

Please remove the dot in the className. ;)

@@ +155,5 @@
> +    userStylesFilter.setAttribute("placeholder",
> +      this.l10n("style.userStylesSearch"));
> +    userStylesFilter.setAttribute("type", "search");
> +    userStylesFilter.setAttribute("compact", "true");
> +    userStylesFilter.className = ".styleinspector-searchfield";

Same as above.

Comment 55

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Update style inspector UI patch 10

(In reply to Mihai Sucan [:msucan] from comment #53)
> (In reply to Michael Ratcliffe from comment #52)
> 
> > Patch mostly working but tests appear to leave the DOM in a strange state
> > and then time out.
> > 
> > Before merging Mihai's patch that changes this to use tables all tests ran
> > without issues. Since the conversion to tables everything runs fine in real
> > life but tests totally break.
> 
> All tests pass here with the exception of
> browser_styleinspector_bug_672746_default_styles.js which has a minor
> failure.  The following fix is needed in propertyVisible():
> 
>      if (propView.name == aName) {
> -      return propView.className != "property-view-hidden";
> +      return propView.visible;
>      }
> 
> After this change, all tests pass. (Linux here)
> 
> 
> > It seems like something is getting called recursively when running tests ...
> > platform bug maybe?
> 
> On what OS do the tests fail? It's surprising for the tests to fail since
> the UI changes you/we did are minimal. The switch to a fluid layout that
> shouldn't have any impact on the tests...
> 

Because of Dão's comment about using a magic number (width: 220px;) we switched to using a table for layout instead of divs. Unfortunately, in debug mode the table reflows triggered Bug 460637 which threw hundreds of assertions during testing and causing the test to time out. This means that we are unable to use tables to tabulate our data until the bug is fixed. Using a div with a fixed width as our first column appears to be our only choice for the moment.

> Shorlanders feedback:
> ...
> 3. Values should stay in the right-hand column when wrapped.
> 
> Done

This also depends on the previous bug being fixed.

> Btw, I really like how this looks. Awesome work!
> 

/me is strutting

> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +146,5 @@
> > +    userStylesCheckbox.id = "onlyUserStylesCheckbox";
> > +    userStylesCheckbox.setAttribute("label",
> > +      this.l10n("style.userStylesLabel"));
> > +    userStylesCheckbox.setAttribute("checked", "true");
> > +    userStylesCheckbox.className = ".styleinspector-onlyuserstyles";
> 
> Please remove the dot in the className. ;)
> 
> @@ +155,5 @@
> > +    userStylesFilter.setAttribute("placeholder",
> > +      this.l10n("style.userStylesSearch"));
> > +    userStylesFilter.setAttribute("type", "search");
> > +    userStylesFilter.setAttribute("compact", "true");
> > +    userStylesFilter.className = ".styleinspector-searchfield";
> 
> Same as above.

Um ... done, it was a long day :o/

I have logged new bugs for:
1. Style inspector property names & values should be tabulated using a table (bug 697398)
2. Style inspector expandos (treetwisties) should be styled for XP & OSX (bug 697399)

Dão ... can you have a quick look over the css in this patch?

Comment 56

[Mihai Sucan [:msucan]]

Comment on attachment 569637 [details] [diff] [review]
Update style inspector UI patch 10

Patch looks fine! r+! Thanks for the updates!


One comment (not binding to the review): I am not sure that the move of the "only user styles" checkbox and of the search filter outside of the iframe is ideal. It really depends how the sidebar panel patch will work and how the rules view (from dcamp) all fit together. This doesn't need to change now - we'll see then if anything needs to change.

Comment 57

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 569637 [details] [diff] [review]
Update style inspector UI patch 10

+  // Only user styles checkbox
+  this.onlyUserStylesCheckbox =
+    this.panel.querySelector("#onlyUserStylesCheckbox");
+  this.onlyUserStylesCheckbox.addEventListener("command",
+    this.onlyUserStylesChanged.bind(this));

This isn't going to work. I just spent a bunch of time ripping stuff out of the panel so we can retarget the style inspector in other venues. Can we find a way for these widgets to exist in the iframe or restyle the xhtml widgets to look more like system widgets?

Comment 58

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector now 100% contained in iframe

(In reply to Rob Campbell [:rc] (robcee) from comment #57)
> Comment on attachment 569637 [details] [diff] [review] [diff] [details] [review]
> Update style inspector UI patch 10
> 
> +  // Only user styles checkbox
> +  this.onlyUserStylesCheckbox =
> +    this.panel.querySelector("#onlyUserStylesCheckbox");
> +  this.onlyUserStylesCheckbox.addEventListener("command",
> +    this.onlyUserStylesChanged.bind(this));
> 
> This isn't going to work. I just spent a bunch of time ripping stuff out of
> the panel so we can retarget the style inspector in other venues. Can we
> find a way for these widgets to exist in the iframe or restyle the xhtml
> widgets to look more like system widgets?

Now everything (including the search filter etc.) and legend live in the iframe.

Comment 59

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Dão ... can you take a look at the css for this bug?

Comment 60

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector now 100% contained in iframe patch 2

Rebased

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 570212 [details] [diff] [review]
Style Inspector now 100% contained in iframe patch 2

>+    <table class="header">
>+      <tr>
>+        <td>
>+          <label class="userStylesLabel" dir="${getRTLAttr}">
>+            &userStylesLabel;
>+            <input class="onlyuserstyles" save="${onlyUserStylesCheckbox}"
>+                   type="checkbox" onchange="${onlyUserStylesChanged}" checked=""/>

Why is the checkbox after the label?

>+          </label>
>+        </td>
>+        <td class="searchFieldContainer">
>+          <xul:textbox class="searchfield" type="search" save="${searchField}"
>+                       placeholder="&userStylesSearch;"
>+                       oncommand="${filterChanged}"/>
>+        </td>
>+      </tr>
>+    </table>

Why is this a table?

>+++ b/browser/themes/gnomestripe/browser/devtools/csshtmltree.css

>+#path {
>   font-size: 11px;
>   word-spacing: -1px;
>+  margin-bottom: 0;
>+  background-color: #DCE2E8;

This color looks alien on my ubuntu desktop. Can you use -moz-dialog? You're also hardcoding strange color values elsewhere where you should probably use native colors (à la listbox.css).

> .expander {
>-  width: 8px;
>-  height: 8px;
>+  -moz-appearance: treetwisty;
>+  width: 9px;
>+  height: 9px;
>   float: left;
>-  -moz-margin-start: 15px;
>+  -moz-margin-start: 5px;
>   -moz-margin-end: 5px;
>-  margin-top: 3px;
>-  background: url("chrome://browser/skin/devtools/arrows.png");
>-  background-position: 24px 0;
>+}
>+.expander[open] {
>+  -moz-appearance: treetwistyopen;
> }

The twisty is misaligned and cut off over here.

Comment 62

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector now 100% contained in iframe patch 3

(In reply to Dão Gottwald [:dao] from comment #61)
> Comment on attachment 570212 [details] [diff] [review] [diff] [details] [review]
> Style Inspector now 100% contained in iframe patch 2
> 
> >+    <table class="header">
> >+      <tr>
> >+        <td>
> >+          <label class="userStylesLabel" dir="${getRTLAttr}">
> >+            &userStylesLabel;
> >+            <input class="onlyuserstyles" save="${onlyUserStylesCheckbox}"
> >+                   type="checkbox" onchange="${onlyUserStylesChanged}" checked=""/>
> 
> Why is the checkbox after the label?
> 

Fixed

> >+          </label>
> >+        </td>
> >+        <td class="searchFieldContainer">
> >+          <xul:textbox class="searchfield" type="search" save="${searchField}"
> >+                       placeholder="&userStylesSearch;"
> >+                       oncommand="${filterChanged}"/>
> >+        </td>
> >+      </tr>
> >+    </table>
> 
> Why is this a table?
> 

Because we want the xul:textbox to autofit the remaining space in the panel and this is easily done with tables. Both -moz-box-flex and flex="1" fail when html & xul are mixed in a single block. It is not possible to use a xul:checkbox because xul tags in html documents are rendered incorrectly when there is an html alternative and it is not possible to use an input field instead of a xul:textbox because we want the native search look and feel. As far as I can see using a table is our only choice here.

> >+++ b/browser/themes/gnomestripe/browser/devtools/csshtmltree.css
> 
> >+#path {
> >   font-size: 11px;
> >   word-spacing: -1px;
> >+  margin-bottom: 0;
> >+  background-color: #DCE2E8;
> 
> This color looks alien on my ubuntu desktop. Can you use -moz-dialog? You're
> also hardcoding strange color values elsewhere where you should probably use
> native colors (à la listbox.css).
> 

The color scheme is shorlander's design and I used the colors from his mockups. Still, I have now used -moz-colors for more things because this is what you have requested. I have not changed the current colors for:
1. iframe bg color
2. path.lastchild
3. links
4. the bg color of dark rows

Changing any of these colors would be akin to totally disregarding shorlander's design.

> > .expander {
> >-  width: 8px;
> >-  height: 8px;
> >+  -moz-appearance: treetwisty;
> >+  width: 9px;
> >+  height: 9px;
> >   float: left;
> >-  -moz-margin-start: 15px;
> >+  -moz-margin-start: 5px;
> >   -moz-margin-end: 5px;
> >-  margin-top: 3px;
> >-  background: url("chrome://browser/skin/devtools/arrows.png");
> >-  background-position: 24px 0;
> >+}
> >+.expander[open] {
> >+  -moz-appearance: treetwistyopen;
> > }
> 
> The twisty is misaligned and cut off over here.

Interesting ... on Ubuntu 10.04, Fedora 13 & Fedora 14 they are fine. If you add your version, theme etc. to bug 697399 we can look into it then maybe somebody with a similar setup to you could fix it.

Comment 63

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector now 100% contained in iframe patch 4

Dão ... I have now been over the colors and used system colors where possible without departing from shorlander's design.

We use custom colors for:
1. path.lastchild
2. links
3. the items in the legend

We manage to zebra stripe using background: -moz-oddtreerow and overlaying it with a semi-transparent white image to fade them (we don't want the really bold colors used by the tree). Fading the colors this way means that they will always fit the browser theme and still be subtle.

We hope that this is sufficient for you because this is holding up a bunch of other patches and we are very short of time.

Comment 64

[Dão Gottwald [:dao]]

Attachment: broken twisties (ubuntu 11.10)

I'm on Ubuntu 11.10, using the default ambiance theme. Ubuntu 10.4 still ships Firefox 3.6, I think, so it's not really relevant for us.

Comment 65

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #62)
> It is not possible to use a
> xul:checkbox because xul tags in html documents are rendered incorrectly
> when there is an html alternative

I don't understand what this means; as stated this seems like a false statement to me. What can fail is mixing -moz-box with other display types. Is that what you mean?

Comment 66

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Dão ... it was Mihai using the latest devtools build. I don't have access to Ubuntu 11.10 but I will add it to bug 697399.

(In reply to Dão Gottwald [:dao] from comment #65)
> (In reply to Michael Ratcliffe from comment #62)
> > It is not possible to use a
> > xul:checkbox because xul tags in html documents are rendered incorrectly
> > when there is an html alternative
> 
> I don't understand what this means; as stated this seems like a false
> statement to me. What can fail is mixing -moz-box with other display types.
> Is that what you mean?

I mean that if you mix xul & html elements within the same block then flex does not work.

Comment 67

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Also, if I try to use a xul:checkbox in the document it is rendered like a textbox. The table is simply a workaround for some xul quirks.

Comment 68

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #66)
> Dão ... it was Mihai using the latest devtools build. I don't have access to
> Ubuntu 11.10 but I will add it to bug 697399.

Please fix it here? You're adding this code which doesn't quite work and nobody's working on bug 697399 either.

> > > It is not possible to use a
> > > xul:checkbox because xul tags in html documents are rendered incorrectly
> > > when there is an html alternative
> > 
> > I don't understand what this means; as stated this seems like a false
> > statement to me. What can fail is mixing -moz-box with other display types.
> > Is that what you mean?
> 
> I mean that if you mix xul & html elements within the same block then flex
> does not work.

Presumably the container needs display:-moz-box.

Comment 69

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Dão Gottwald [:dao] from comment #68)
> (In reply to Michael Ratcliffe from comment #66)
> > Dão ... it was Mihai using the latest devtools build. I don't have access to
> > Ubuntu 11.10 but I will add it to bug 697399.
> 
> Please fix it here? You're adding this code which doesn't quite work and
> nobody's working on bug 697399 either.
> 

I guess I will need to install Ubuntu 11.10 then.

> > > > It is not possible to use a
> > > > xul:checkbox because xul tags in html documents are rendered incorrectly
> > > > when there is an html alternative
> > > 
> > > I don't understand what this means; as stated this seems like a false
> > > statement to me. What can fail is mixing -moz-box with other display types.
> > > Is that what you mean?
> > 
> > I mean that if you mix xul & html elements within the same block then flex
> > does not work.
> 
> Presumably the container needs display:-moz-box.

Nope, that makes no difference. The moment I switch the checkbox with a xul checkbox flex starts to work but the xul:checkbox looks like a text field with a label.

Comment 70

[Paul Rouget [:paul]]

I tested this patch with my Ubuntu 11.10. I have the same bug.
> >+  -moz-appearance: treetwisty;
> >+  width: 9px;
> >+  height: 9px;

I think the Firefox built-in twisty image is 9px, but the OS' twisty is 12px.

width: 12px;
height: 12px;

Fixes the bug.

Comment 71

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #69)
> Nope, that makes no difference. The moment I switch the checkbox with a xul
> checkbox flex starts to work but the xul:checkbox looks like a text field
> with a label.

Can you upload this patch for me to take a look at it?

Comment 72

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector now 100% contained in iframe patch 5

(In reply to Paul Rouget [:paul] from comment #70)
> I tested this patch with my Ubuntu 11.10. I have the same bug.
> > >+  -moz-appearance: treetwisty;
> > >+  width: 9px;
> > >+  height: 9px;
> 
> I think the Firefox built-in twisty image is 9px, but the OS' twisty is 12px.
> 
> width: 12px;
> height: 12px;
> 
> Fixes the bug.

Thanks Paul ... done

Comment 73

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have also managed to remove the table that Dão disliked ... the issue was purely a missing -moz-box style on the checkbox label.

Comment 74

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Dão Gottwald [:dao] from comment #71)
> (In reply to Michael Ratcliffe from comment #69)
> > Nope, that makes no difference. The moment I switch the checkbox with a xul
> > checkbox flex starts to work but the xul:checkbox looks like a text field
> > with a label.
> 
> Can you upload this patch for me to take a look at it?

I have created bug 699002 about this issue.

Comment 75

[Dão Gottwald [:dao]]

Comment on attachment 571278 [details] [diff] [review]
Style Inspector now 100% contained in iframe patch 5

>+    <xul:hbox class="header" flex="1">

header is unique, so it should be an id rather than a class, right?

>+.darkrow {
>+  background-color: -moz-oddtreerow;
>+  background-image: url("chrome://browser/skin/devtools/transparent-white.png");
>+}

This doesn't really make sense. I suggest you use something like background-color: rgba(0,0,0,.1); if you want to avoid -moz-oddtreerow, which I find slightly strange.

I couldn't apply this patch on mozilla-central tip.

Comment 76

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector with review comments fully addressed

(In reply to Dão Gottwald [:dao] from comment #75)
> Comment on attachment 571278 [details] [diff] [review] [diff] [details] [review]
> Style Inspector now 100% contained in iframe patch 5
> 
> >+    <xul:hbox class="header" flex="1">
> 
> header is unique, so it should be an id rather than a class, right?
> 

Nope, although it is clear why you would think that. Parts of the xhtml file are cloned and used higher up in the file and we need to avoid duplicate ids, hence using a class in this instance.

> >+.darkrow {
> >+  background-color: -moz-oddtreerow;
> >+  background-image: url("chrome://browser/skin/devtools/transparent-white.png");
> >+}
> 
> This doesn't really make sense. I suggest you use something like
> background-color: rgba(0,0,0,.1); if you want to avoid -moz-oddtreerow,
> which I find slightly strange.
> 

We need something very pale so I have opted for rgba(0,0,0,.022). Done.

> I couldn't apply this patch on mozilla-central tip.

Apologies ... now rebased (fx-team)

Comment 77

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Style Inspector with review comments fully addressed

Comment 78

[Dão Gottwald [:dao]]

Comment on attachment 571357 [details] [diff] [review]
Style Inspector with review comments fully addressed

>+.expander {
>+  -moz-appearance: treetwisty;
>+  width: 12px;
>+  height: 12px;
>+  float: left;

This is the wrong side for RTL.

The twisties look as broken as ever over here, by the way...

Comment 79

[Rob Campbell [:rc] (:robcee)]

Attachment: mac screenshot

Comment 80

[Rob Campbell [:rc] (:robcee)]

Attachment: Style Inspector, mac jar.mn breakage fixed

Comment 81

[Rob Campbell [:rc] (:robcee)]

Attachment: linux screenshot (ubuntu 10.04)

twisties look fine on my ubuntu VM.

Comment 82

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #81)
> Created attachment 571836 [details]
> linux screenshot (ubuntu 10.04)
> 
> twisties look fine on my ubuntu VM.

These are entirely different from how the twisties look in Ubuntu 11.10; see attachment 571022 [details].

Comment 83

[Dão Gottwald [:dao]]

Comment on attachment 571784 [details] [diff] [review]
Style Inspector, mac jar.mn breakage fixed

>+.expander {
>+  -moz-appearance: treetwisty;
>+  width: 12px;
>+  height: 12px;
>+  float: left;

This is still positioned on the wrong side for RTL.

Comment 84

[Dave Camp (:dcamp)]

Yeah, twisties don't look right on my ubuntu vm.

They're broken in the html panel too, but not the bookmark or history side panels.

Comment 85

[Dave Camp (:dcamp)]

This is a pretty significant UI improvement without the twisty changes, would it be reasonable to back the twisty changes out of this patch, use the ones that were there before, and handle those separately in a followup?

Comment 86

[Dave Camp (:dcamp)]

Or if someone that knows how -moz-appearance: treetwisty works can step in and help out, that would be a huge help.

Comment 87

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Removed treetwisties

(In reply to Dão Gottwald [:dao] from comment #83)
> Comment on attachment 571784 [details] [diff] [review] [diff] [details] [review]
> Style Inspector, mac jar.mn breakage fixed
> 
> >+.expander {
> >+  -moz-appearance: treetwisty;
> >+  width: 12px;
> >+  height: 12px;
> >+  float: left;
> 
> This is still positioned on the wrong side for RTL.

Fixed

(In reply to Dave Camp (:dcamp) from comment #85)
> This is a pretty significant UI improvement without the twisty changes,
> would it be reasonable to back the twisty changes out of this patch, use the
> ones that were there before, and handle those separately in a followup?

This bug along with the optimizations makes the style inspector many times better so I have backed out the treetwisty changes as we know that the arrow images that we used before work across platforms. The new treetwisty bug is bug 699762.

Dão, is this okay for you now?

Comment 88

[Dão Gottwald [:dao]]

Why did you undo the twisty changes for Windows and Mac? Were these problematic?

You should file a bug on the specific problem with the Linux twisties. Centralizing several separate issues in one followup bug will just make it less likely that somebody will work on that bug.

Comment 89

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Removed treetwisties only for Linux

I will log the treetwisty bug after submitting this patch.

Dão, is this now r+?

Comment 90

[Dão Gottwald [:dao]]

Comment on attachment 571963 [details] [diff] [review]
Removed treetwisties only for Linux

>+.expander {
>+ width: 8px;
>+ height: 8px;
>+ float: left;
>+ -moz-margin-start: 15px;
>+ -moz-margin-end: 5px;
>+ margin-top: 3px;
>+ background: url("chrome://browser/skin/devtools/arrows.png") 48px 0;

Indentation is off.

You forgot to remove arrows.png from pinstripe and winstripe.

Comment 91

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Hopefully the last change

(In reply to Dão Gottwald [:dao] from comment #90)
> Comment on attachment 571963 [details] [diff] [review] [diff] [details] [review]
> Removed treetwisties only for Linux
> 
> >+.expander {
> >+ width: 8px;
> >+ height: 8px;
> >+ float: left;
> >+ -moz-margin-start: 15px;
> >+ -moz-margin-end: 5px;
> >+ margin-top: 3px;
> >+ background: url("chrome://browser/skin/devtools/arrows.png") 48px 0;
> 
> Indentation is off.
> 

Fixed

> You forgot to remove arrows.png from pinstripe and winstripe.

Nope, dcamp uses them as part of the Rule View.

Comment 92

[Dão Gottwald [:dao]]

> > You forgot to remove arrows.png from pinstripe and winstripe.
> 
> Nope, dcamp uses them as part of the Rule View.

Where? mxr doesn't find it. Is this a pending patch? I'm pretty sure that image shouldn't be used going forward, for the same reason you stopped using it in this bug.

Comment 93

[Dave Camp (:dcamp)]

It landed in bug 693887.  

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/devtools/csshtmltree.css#226

for one use of it.

I haven't been able to get treetwisty to work right in the rule view yet.

Comment 94

[Dão Gottwald [:dao]]

(In reply to Dave Camp (:dcamp) from comment #93)
> It landed in bug 693887.  
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/
> browser/devtools/csshtmltree.css#226
> 
> for one use of it.
> 
> I haven't been able to get treetwisty to work right in the rule view yet.

Is there a bug filed on this?

Comment 95

[Dão Gottwald [:dao]]

Comment on attachment 571972 [details] [diff] [review]
Hopefully the last change

> .expander[dir="rtl"] {
>-  background-position: 16px 0;
>+  float: right;
>+  background-position: 40px 0;
> }

This doesn't work, since there's no dir attribute on the expander. You can use :-moz-locale-dir here, can't you?

Comment 96

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Hopefully the last change

(In reply to Dão Gottwald [:dao] from comment #95)
> Comment on attachment 571972 [details] [diff] [review] [diff] [details] [review]
> Hopefully the last change
> 
> > .expander[dir="rtl"] {
> >-  background-position: 16px 0;
> >+  float: right;
> >+  background-position: 40px 0;
> > }
> 
> This doesn't work, since there's no dir attribute on the expander. You can
> use :-moz-locale-dir here, can't you?

Unfortunately that selector does not work properly in html docs (https://developer.mozilla.org/en/CSS/%3A-moz-locale-dir%28ltr%29). We had a bunch of issues with RTL in iframes and, after working through them with Ehsan, we turned out adding dir attributes where necessary instead.

I have now also done so with our expanders.

Comment 97

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Oh, Dão ... the treetwisty issue is bug 699832

Comment 98

[Dave Camp (:dcamp)]

Filed bug 699840 on the rule view tree twisties.

Comment 99

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #96)
> Unfortunately that selector does not work properly in html docs
> (https://developer.mozilla.org/en/CSS/%3A-moz-locale-dir%28ltr%29). We had a
> bunch of issues with RTL in iframes and, after working through them with
> Ehsan, we turned out adding dir attributes where necessary instead.

So, please file a bug on turning this into a xul document (see bug 583041 for a good example). You can still use html nodes in such a document. The root node being html has no advantage, as far as I can tell.

Comment 100

[Dão Gottwald [:dao]]

Comment on attachment 572012 [details] [diff] [review]
Hopefully the last change

>+    <label class="selectedElementLabel" dir="${getRTLAttr}">
>+      &selectedElementLabel;
>+    </label>

This should be no <label>. Label elements are for labeling form fields. What you have here should just be a <span> (or <h*>).

>+    <ol>
>+      <li foreach="item in ${pathElements}" dir="${getRTLAttr}">
>+        <a href="#" onclick="${pathClick}" __pathElement="${item.element}">
>+          ${__element.pathElement = item.element; item.display}
>+        </a>
>+      </li>
>+    </ol>

This seems to be busted in RTL mode. Please file a bug.

Comment 101

[Rob Campbell [:rc] (:robcee)]

(In reply to Dão Gottwald [:dao] from comment #100)
> Comment on attachment 572012 [details] [diff] [review] [diff] [details] [review]
> Hopefully the last change
> 
> >+    <label class="selectedElementLabel" dir="${getRTLAttr}">
> >+      &selectedElementLabel;
> >+    </label>
> 
> This should be no <label>. Label elements are for labeling form fields. What
> you have here should just be a <span> (or <h*>).
> 
> >+    <ol>
> >+      <li foreach="item in ${pathElements}" dir="${getRTLAttr}">
> >+        <a href="#" onclick="${pathClick}" __pathElement="${item.element}">
> >+          ${__element.pathElement = item.element; item.display}
> >+        </a>
> >+      </li>
> >+    </ol>
> 
> This seems to be busted in RTL mode. Please file a bug.

filed bug 699900 for this.

Comment 102

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/49b98a76f55b

Comment 103

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/49b98a76f55b

Comment 104

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #99)
> (In reply to Michael Ratcliffe from comment #96)
> > Unfortunately that selector does not work properly in html docs
> > (https://developer.mozilla.org/en/CSS/%3A-moz-locale-dir%28ltr%29). We had a
> > bunch of issues with RTL in iframes and, after working through them with
> > Ehsan, we turned out adding dir attributes where necessary instead.
> 
> So, please file a bug on turning this into a xul document (see bug 583041
> for a good example). You can still use html nodes in such a document. The
> root node being html has no advantage, as far as I can tell.

Has this bug been filed?

Comment 105

[Dave Camp (:dcamp)]

(In reply to Dão Gottwald [:dao] from comment #104)
 
> > So, please file a bug on turning this into a xul document (see bug 583041
> > for a good example). You can still use html nodes in such a document. The
> > root node being html has no advantage, as far as I can tell.
> 
> Has this bug been filed?

Bug 700036.