Last Comment Bug 712469 - Inspector Rule View selectors can be more visually scannable
: Inspector Rule View selectors can be more visually scannable
Status: RESOLVED FIXED
[ruleview][firefox14-wanted]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 17
Assigned To: Heather Arthur [:harth]
:
Mentors:
: 734672 734889 762076 (view as bug list)
Depends on: 781974
Blocks: 708257 716516 719831
  Show dependency treegraph
 
Reported: 2011-12-20 15:22 PST by Jason Grlicky [:grlicky]
Modified: 2012-08-10 17:55 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mockup: computed view + exposé mode (1.50 MB, image/png)
2012-02-09 14:36 PST, Paul Rouget [:paul]
no flags Details
tentative - screenshot (420.04 KB, image/png)
2012-03-05 20:05 PST, Paul Rouget [:paul]
no flags Details
tentative - patch (15.04 KB, patch)
2012-03-05 20:06 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
tentative v0.2 - patch (16.29 KB, patch)
2012-03-05 20:23 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
tentative v0.2 - screenshot (264.97 KB, image/png)
2012-03-05 20:28 PST, Paul Rouget [:paul]
no flags Details
mockup: feedback + suggestions for tentative 0.2 screenshot (227.19 KB, image/png)
2012-03-08 15:06 PST, Jason Grlicky [:grlicky]
mratcliffe: feedback+
fayearthur: feedback+
Details
patch v0.1 (22.08 KB, patch)
2012-03-14 01:19 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v0.1.1 (22.38 KB, patch)
2012-03-14 02:56 PDT, Paul Rouget [:paul]
mratcliffe: feedback+
Details | Diff | Review
patch v0.1.2 (22.36 KB, patch)
2012-03-14 03:05 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
screenshot v0.1.2 (418.91 KB, image/png)
2012-03-14 08:14 PDT, Paul Rouget [:paul]
no flags Details
rebased patch (15.03 KB, patch)
2012-06-28 09:48 PDT, Heather Arthur [:harth]
no flags Details | Diff | Review
rebased patch with fixed test (17.61 KB, patch)
2012-07-05 09:04 PDT, Heather Arthur [:harth]
dcamp: review+
Details | Diff | Review
rebased patch with fix for comments (17.03 KB, patch)
2012-07-11 09:08 PDT, Heather Arthur [:harth]
dcamp: review+
shorlander: ui‑review+
Details | Diff | Review

Description Jason Grlicky [:grlicky] 2011-12-20 15:22:59 PST
After using the style editor more day-to-day, a use case that I find myself doing quite frequently which I think could be better served is that of visually scanning downwards through a list of styles to find a particular selector.

As an example, I found myself inspecting the twitter bootstrap examples to see how they're put together ( http://cl.ly/012t2B3d0n432V3c0W2B ). My current goal was to find out how the "pull-left" class was affecting the form element I was inspecting. My immediate task then is to scan down the left-hand-side of the style editor to find the .pull-left selector.

As you can see from the screenshot, the font and indentation along the left-hand side of the style editor are all uniform. This, coupled with the fact that the filenames, selectors, checkboxes, and dropdowns are all given similar visual weights and colors, makes the visual hierarchy very flat and difficult to scan for anything in particular.

I believe that we can make this totally awesome though, with just a few tweaks! Some proposals:
 - Decrease the prominence of the checkboxes and dropdown indicators, so that the indentation of style properties is more profound. This could mean moving them to the right hand side, or just reducing their visual weight.
 - Reduce the visual prominence given to the file name / line number, as it is in general not as important as the style selector. Then increase the visual prominence of the style selector. This can be done by using colors or at least varying shades of grey to further differentiate levels in the information hierarchy, or simply moving the position of these objects.

I'd love to provide you with some mockups that illustrate potential solutions for these visual problems if that would be helpful. I'd need to take into account your plans for current functionality and any unimplemented design work you've already got, so if this is interesting to you, just let me know and we can talk more!
 
For point of contrast, in the Firebug style editor design makes this use-case really really clear, by having the selectors be a different color than the properties, values, and file names ( http://cl.ly/2F1p151z3n0Z0G3K050D ). The indentation also makes very clear what is a selector and what isn't. Similarly, the Chrome style editor uses color to differentiate elements, and also gives the file name/line number less prominence ( http://cl.ly/260G3x2h1p330q0f0J2G ). Though not as easy to scan for selectors as firebug (due to the "Matched CSS Rules" bar and the dropdowns/alert icons), it is still easier to scan for selectors than our style editor. We can do better! :D :D
Comment 1 Paul Rouget [:paul] 2011-12-21 04:46:53 PST
First, this is a very clear and fair feedback. And I agree for most of it. Thank you for that.

Second, afaik, we are not planning to update the style of this tool soon. But I believe that we really need to provide a much better user experience for the Rule View. So yes, please, mockups would be very much appreciated.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-12-22 07:26:57 PST
thanks for this bug, Jason. It is indeed good stuff.

I'm rewording the summary to reflect the feature name. We have something of a nomenclature issue with some of these different tools now. The Style Editor is the new, full text editor for working with CSS in Firefox 11 (currently in Aurora).

The Inspector Rule View is the feature you're referring to here and is part of the Inspector.
Comment 3 Jason Grlicky [:grlicky] 2012-01-06 10:13:30 PST
Thanks for the info, Rob! Inspector Rule View it is :D

I'm going to be working on some mockups for this soon - next week is an Apps work week, so I won't be able to start then, but shortly thereafter.
Comment 4 Paul Rouget [:paul] 2012-01-09 05:59:15 PST
Also, see bug 716516.
Comment 5 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-01-09 12:29:00 PST
The 5 points relating to the rule view from bug 716516 are:
1. CSS selectors (along with braces) should be color 1
2. Names and values should be colors 2 and 3
3. Names and values should be indented where appropriate to make them easier to read
4. CSS rule checkboxes should only be visible only when the appropriate property is moused over ... this will greatly tidy up the visual space.
5. In order to free up even more visual space in the rule view the file and line number should be on the right-hand side of the CSS selector.

I will make bug 716516 dependant on this one so that we know which colors and styles we should use with other tools.
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-10 08:45:15 PST
Jason, can you assign yourself to this bug if you're working on it? Thanks!

filter on pegasus
Comment 7 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-01-11 09:00:07 PST
filter on pegasus
Comment 8 Jason Grlicky [:grlicky] 2012-01-26 18:10:19 PST
I've got a question for these mocks - are longhand properties in their expanded form going to be editable?

http://cl.ly/0A2W350C3b3s3u0g3p1S

I ask because right now, they look editable, but are not. So we should fix that, one way or the other ;)
Comment 9 Paul Rouget [:paul] 2012-01-27 00:37:51 PST
Indeed, right now, they are not editable.

I am not sure what that would mean if they could be editable. It would create a new declaration, and that would be weird. I think they should not be editable, and should be seen as a hint/help.

Also, how useful are these longhand properties if they are not editable? I think they make the UI of a line quite wonky. The twisty should not be that "visible".
Comment 10 Jason Grlicky [:grlicky] 2012-01-27 13:52:42 PST
Ok, cool - I agree that making them editable would be weird, and that this leaves their purpose as mainly being educational. Yeah, I totally agree that the twisty should be less prominent because of this!
Comment 11 Paul Rouget [:paul] 2012-02-08 23:28:23 PST
We are now showing the checkbox only on hover.
Comment 12 Jason Grlicky [:grlicky] 2012-02-09 14:15:18 PST
YESSSSSSSS~
Comment 13 Paul Rouget [:paul] 2012-02-09 14:36:37 PST
Created attachment 595880 [details]
mockup: computed view + exposé mode

To give you some more context, here is a mockup of how we would like to show the tools in the future. This is not 100% we will implement that, but I thought that could help (and the mockups shows the property view, not the rule view).
Comment 14 Paul Rouget [:paul] 2012-02-13 11:22:13 PST
Jason, any update on the mockup?
Comment 15 Jason Grlicky [:grlicky] 2012-02-14 10:26:39 PST
Hey Paul - I've been in a bit of a crunch recently with the apps work. I'm not sure when it will let up, and I definitely don't want to hold you guys up. Since I'm at an offsite this week, I know that I will at least not be able to work on this any more for the remainder of the week, but am hoping to pick it up again afterwards.
Comment 16 Paul Rouget [:paul] 2012-03-05 20:05:08 PST
Created attachment 603146 [details]
tentative - screenshot
Comment 17 Paul Rouget [:paul] 2012-03-05 20:06:12 PST
Created attachment 603147 [details] [diff] [review]
tentative - patch
Comment 18 Paul Rouget [:paul] 2012-03-05 20:09:26 PST
Here, I am experimenting with reproducing the look of the Source Editor (same colors). I also indent the code and moved the file reference to the right (float:right).

Screenshot: attachment 603146 [details]
Code: attachment 603147 [details] [diff] [review]

Might be a good temporary solution.
Comment 19 Paul Rouget [:paul] 2012-03-05 20:23:37 PST
Created attachment 603149 [details] [diff] [review]
tentative v0.2 - patch

fixed the alignment
Comment 20 Paul Rouget [:paul] 2012-03-05 20:25:49 PST
(haven't tested with Mac and Windows yet)
Comment 21 Paul Rouget [:paul] 2012-03-05 20:28:50 PST
Created attachment 603150 [details]
tentative v0.2 - screenshot
Comment 22 Paul Rouget [:paul] 2012-03-05 20:32:23 PST
Also - we need to `overflow:ellipsis` the selectors, the file reference and the declarations. This is not very easy.
Comment 23 Heather Arthur [:harth] 2012-03-05 23:20:52 PST
If the style sheet and line number is going to be a link to the Style Editor then I don't think it makes sense to make it look like a CSS comment. It should look like a link, but I love how it's off to the right side so you can focus more on the rule itself.
Comment 24 Paul Rouget [:paul] 2012-03-05 23:36:02 PST
> I don't think it makes sense to make it look like a CSS comment

Making it look like a CSS comment helps the user to scan the content of the sidebar. I understand that it makes the feature less discoverable, but I think it's a good tradeoff. I also think that the user will eventually over the comment and then see that underline and the "hand", and he will then understand that he can click there.

I can also underline the reference by default: http://i.imgur.com/UoaQ1.png

I think we need some Shorlander magic here.
Comment 25 Jason Grlicky [:grlicky] 2012-03-08 15:06:08 PST
Created attachment 604219 [details]
mockup: feedback + suggestions for tentative 0.2 screenshot

Hey Paul - awesome work on this! I totally agree about the user eventually mousing over the style sheet/line number link and discovering it, and I think using the comment color there is a totally good call. I agree with Heather that the /* */ is probably overkill, though.

I'd still like to try out your patch to get a feel for how the expanded styles work, but in the mean time I was too excited, so I whipped something up really quickly to demonstrate a couple concepts. :D

Main differences:

- Selectors are grouped by inheritance, and the "Inherited from ____"statements are lumped into one title for each group.  The group of selectors that directly match this element (the one at the very top of the sidebar) doesn't have a group title. Please let me know if there are any problems with this.

- Though I kept the CSS comment color on the style sheet/line number, I removed the /* */ to keep the visual noise of the sidebar at a minimum.

- The style sheet/line number is underlined on hover.

Thanks, I'd love to hear what you think!
Comment 26 Marten Schilstra 2012-03-08 23:32:47 PST
I agree that the /* */ would only clutter the inspector. Rest of the mockup looks promising.
Comment 27 Paul Rouget [:paul] 2012-03-09 00:18:18 PST
I would also find a solution to make obvious what is editable (property names, property values, and curly brackets) and what is not.

I was thinking about using a dashed border-bottom to do that (see this screenshot: http://i.imgur.com/UoaQ1.png).

If you guys have a better idea…
Comment 28 Paul Rouget [:paul] 2012-03-12 09:40:09 PDT
*** Bug 734889 has been marked as a duplicate of this bug. ***
Comment 29 sys.sgx 2012-03-12 09:44:58 PDT
buttons do have more margin :)
i also agree with this idea.
Comment 30 Marten Schilstra 2012-03-12 10:08:54 PDT
The light dashed underline and maybe a tooltip should be enough to tell the user that the property is editable.  Maybe only the underline on hover? The underline is pretty much on the edge of cluttering the view.

Maybe also drop the semi-colons? They don't mean anything in this context.
Comment 31 sys.sgx 2012-03-12 10:13:38 PDT
may I suggest adding a way to easily comment an entire selector (instead of having to click each element inside it)?
thanks
Comment 32 Paul Rouget [:paul] 2012-03-12 10:37:58 PDT
(In reply to sys.sgx from comment #31)
> may I suggest adding a way to easily comment an entire selector (instead of
> having to click each element inside it)?
> thanks

Commenting a selector?

You mean folding a rule set?
Comment 33 Paul Rouget [:paul] 2012-03-12 10:38:28 PDT
Ho, disabling a rule set.
Comment 34 Paul Rouget [:paul] 2012-03-12 10:38:58 PDT
Please file a bug for that.
Comment 35 sys.sgx 2012-03-12 10:42:47 PDT
done: https://bugzilla.mozilla.org/show_bug.cgi?id=734934
topic moved there.
Comment 36 sys.sgx 2012-03-12 11:51:46 PDT
we can relate this topic with this one: https://bugzilla.mozilla.org/show_bug.cgi?id=734965
Comment 37 sys.sgx 2012-03-12 12:14:40 PDT
*** Bug 734672 has been marked as a duplicate of this bug. ***
Comment 38 Heather Arthur [:harth] 2012-03-13 22:17:51 PDT
Comment on attachment 604219 [details]
mockup: feedback + suggestions for tentative 0.2 screenshot

I really like splitting out the inherited rules. I've heard a web dev express that at first they just want to see the rules that match the element, and this would make that much easier. I also like the style editor links to the side.

Aside from the color scheme which we'll update soon, I like this rule view quite a bit.
Comment 39 Paul Rouget [:paul] 2012-03-14 01:19:55 PDT
Created attachment 605677 [details] [diff] [review]
patch v0.1
Comment 40 Paul Rouget [:paul] 2012-03-14 01:21:41 PDT
Comment on attachment 605677 [details] [diff] [review]
patch v0.1

Mike, can you take a look at how I add the inherited source?
Comment 41 Paul Rouget [:paul] 2012-03-14 02:56:25 PDT
Created attachment 605701 [details] [diff] [review]
patch v0.1.1

removed the border for the computed style
Comment 42 Paul Rouget [:paul] 2012-03-14 03:05:20 PDT
Created attachment 605702 [details] [diff] [review]
patch v0.1.2
Comment 43 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-03-14 03:52:04 PDT
Comment on attachment 604219 [details]
mockup: feedback + suggestions for tentative 0.2 screenshot

I like it. Obviously we should be using the same name / value throughout all tools (object inspector etc.) but I guess that can come later.
Comment 44 Paul Rouget [:paul] 2012-03-14 04:02:27 PDT
(In reply to Michael Ratcliffe from comment #43)
> Comment on attachment 604219 [details]
> mockup: feedback + suggestions for tentative 0.2 screenshot
> 
> I like it.

Cool!

> Obviously we should be using the same name / value throughout all
> tools (object inspector etc.) but I guess that can come later.

bug 715472
Comment 45 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-03-14 04:07:13 PDT
Comment on attachment 605701 [details] [diff] [review]
patch v0.1.1

> .ruleview-property > span > .ruleview-propertyname,
> .ruleview-property > .ruleview-propertyvalue {
>   border-bottom: 1px dashed #cddae5;
> }

Can you not use the following ... it would be much more efficient:
.ruleview-propertyname,
.ruleview-propertyvalue {
  border-bottom: 1px dashed #cddae5;
}

With that change f+ ... I especially like the grouping of inherited rules.
Comment 46 Paul Rouget [:paul] 2012-03-14 04:08:57 PDT
(In reply to Michael Ratcliffe from comment #45)
> Comment on attachment 605701 [details] [diff] [review]
> patch v0.1.1
> 
> > .ruleview-property > span > .ruleview-propertyname,
> > .ruleview-property > .ruleview-propertyvalue {
> >   border-bottom: 1px dashed #cddae5;
> > }
> 
> Can you not use the following ... it would be much more efficient:
> .ruleview-propertyname,
> .ruleview-propertyvalue {
>   border-bottom: 1px dashed #cddae5;
> }
> 
> With that change f+ ... I especially like the grouping of inherited rules.

Nope, because I don't want to use a border for the expanded values.
Comment 47 Paul Rouget [:paul] 2012-03-14 04:09:18 PDT
We need to do the same design update for the Computed View: bug 735629
Comment 48 Paul Rouget [:paul] 2012-03-14 04:19:16 PDT
(In reply to Paul Rouget [:paul] from comment #46)
> (In reply to Michael Ratcliffe from comment #45)
> > Comment on attachment 605701 [details] [diff] [review]
> > patch v0.1.1
> > 
> > > .ruleview-property > span > .ruleview-propertyname,
> > > .ruleview-property > .ruleview-propertyvalue {
> > >   border-bottom: 1px dashed #cddae5;
> > > }
> > 
> > Can you not use the following ... it would be much more efficient:
> > .ruleview-propertyname,
> > .ruleview-propertyvalue {
> >   border-bottom: 1px dashed #cddae5;
> > }
> > 
> > With that change f+ ... I especially like the grouping of inherited rules.
> 
> Nope, because I don't want to use a border for the expanded values.

Expanded values are not editable. And the border is supposed to mean "editable".
Comment 49 Paul Rouget [:paul] 2012-03-14 04:21:45 PDT
Comment on attachment 605702 [details] [diff] [review]
patch v0.1.2

Try-builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/prouget@mozilla.com-023b9dafb24d/

Note that in this try build, even the non-editable properties (once a declaration is expanded) have the dashed border.

This is fixed in patcvh v0.1.2.
Comment 50 sys.sgx 2012-03-14 06:20:47 PDT
suggestion: because some rules might be sequentially overrided by other css styles, we could add a "link" to each overrided rule in the Style panel so that upon clicking on it, it will "highlight" (or something similar) the style that is overriding it. Thus, the you will be able to tell the hierarchy of the rules.
Comment 51 Paul Rouget [:paul] 2012-03-14 08:14:56 PDT
Created attachment 605758 [details]
screenshot v0.1.2
Comment 52 sys.sgx 2012-03-14 08:20:06 PDT
The web console also plans to have a "dark" color theme. would it be possible to be in sync with that and also offer such a dark theme (matching the one of the console)? screenshot v0.1.2 is great.
Comment 53 Rob Campbell [:rc] (:robcee) 2012-03-14 08:40:57 PDT
(In reply to sys.sgx from comment #52)
> The web console also plans to have a "dark" color theme. would it be
> possible to be in sync with that and also offer such a dark theme (matching
> the one of the console)? screenshot v0.1.2 is great.

sys.sgx: Could you please refrain from advocating for and commenting on these bugs with your suggestions? The place for this kind of discussion is not bugzilla, but we do follow the news/google group: mozilla.dev.apps.firefox.

Comments of this type or not really helpful here, though we do appreciate feedback in the appropriate forum.

thanks.
Comment 54 sys.sgx 2012-03-14 09:04:28 PDT
rc: i think giving in sync suggestions for current specific bugs was part of the bugzilla process. so, the right way was to add a topic to the newsgroup (talking about pretty much everything mozilla related) suggesting to this specific bug to be in sync with another bug currently on its way to land to the next version?.. well, ok, i respect that.
Comment 55 Paul Rouget [:paul] 2012-03-14 09:11:58 PDT
triage, filter on centaur
Comment 56 Kevin Dangoor 2012-03-16 06:52:25 PDT
I've had a request to make the line height a bit tighter to get more information into the view. I just wanted to pass that along for consideration as you're styling this view.
Comment 57 Paul Rouget [:paul] 2012-04-03 02:00:42 PDT
@jgrlicky ui-review ping?
Also, I need help to make sure that the editable values look editable.
Comment 58 Jason Grlicky [:grlicky] 2012-05-02 17:25:25 PDT
Hey Paul,

Thank you so much for your patience on this bug. I do think the dotted-underlined values look editable!

In addition to this, it might be with it to change the cursor to a pointer when hovering over the editable boxes to indicate that the item is actionable. I realize this is not the convention with text boxes, but these don't look like typical text-boxes, so some additional cues that activity is available seems appropriate to me.

After using it, I've also got some other ideas about how to make it more polished. Split any of these out into other bugs as you see fit, of course!

Can be addressed without a mockup:
	
	- Hovering behavior on the properties seems to show the checkboxes only after a delay. It would make the system more understandable and appear less sluggish to show the checkbox immediately (the current nightly behavior). 
	- It seems like clicking on the whitespace to add a rule is broken in the patch.
	- The next lines of rules should wrap to their indentation level, like the expanded rules do. This http://cl.ly/042B393x1t0a0e1W0S2z stops the rules from being scannable. There might be even more we could do here, but this improvement is the low-hanging-fruit.
	- Expanded rules should not appear editable (via the mouse cursor or underlining), since they are not.

May need a mockup:
	- The colors are not right on the blue "Inherited from" line dividers. The important thing is that the text in the dividers be more prominent than the borders. Some colors you could try would be text: #57819E, border: #B4C4D1.
	- Expanders are too prominent. I would make them the same visual prominence as the blue "Inherited from" dividers. A color to try would be #CDDAE5.
	- Indentation on expanded rules can be reduced.

Thanks for the great work you've been doing for web developers everywhere, Paul. If you need clarification on anything, feel free to ask!
Comment 59 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-05-03 09:55:43 PDT
> - It seems like clicking on the whitespace to add a rule is broken in the patch.

If clicking on whitespace allowed you to add a property then it would not be possible to copy text.
Comment 60 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-05-03 10:20:41 PDT
Ignore comment 59, I was wrong.
Comment 61 Paul Rouget [:paul] 2012-05-25 11:08:58 PDT
Note for later: the expanded properties (from shorthand properties) should have a different look (even if subtle) to reflect the fact they are not editable.
Comment 62 Paul Rouget [:paul] 2012-06-08 06:24:32 PDT
For the record, here is a recent mockup for the Rule View: https://bug715472.bugzilla.mozilla.org/attachment.cgi?id=622375
And a mockup (a little older) for the Computed View: https://bug749628.bugzilla.mozilla.org/attachment.cgi?id=619038
Comment 63 Heather Arthur [:harth] 2012-06-28 09:48:08 PDT
Created attachment 637554 [details] [diff] [review]
rebased patch

rebased patch v0.1.2, with some style fixes.
Comment 64 Heather Arthur [:harth] 2012-07-05 09:04:37 PDT
Created attachment 639363 [details] [diff] [review]
rebased patch with fixed test

Rebased patch with and all tests passing.

Style is a bit different than Paul's original patch, mainly I kept the background white to match the computed view for the time being.
Comment 65 Paul Rouget [:paul] 2012-07-09 06:05:49 PDT
Comment on attachment 639363 [details] [diff] [review]
rebased patch with fixed test

Thanks for rebasing all this. Since your work is based on my work, it's possible that I'm the one to blame for some of these remarks :)

- the pseudo indentation doesn't work. And for some reason, I don't see the "ruleview-property-name-and-colon" span in the DOM.
- for hsl colors, don't use spaces hsl(121,42%,43%)
- can you explain why we need `_clearRules()` in nodeChanged now?
- can we make the filename:linenumber block unselectable?
- for transitions, don't use the -moz- prefix
Comment 66 Dave Camp (:dcamp) 2012-07-09 11:08:45 PDT
Comment on attachment 639363 [details] [diff] [review]
rebased patch with fixed test

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

Looks great.
Comment 67 Heather Arthur [:harth] 2012-07-11 09:08:49 PDT
Created attachment 641080 [details] [diff] [review]
rebased patch with fix for comments

Updated patch to Paul's comments:

(In reply to Paul Rouget [:paul] from comment #65)
> - the pseudo indentation doesn't work. And for some reason, I don't see the
> "ruleview-property-name-and-colon" span in the DOM.

Discussed this on IRC. I think it's easier to read without aligning the property values. Aligning the property values makes it easier to compare values, but they're unrelated and don't really need to be compared. At the same time it makes it a bit slower to associate a property name with it's value, which is important.

> - can you explain why we need `_clearRules()` in nodeChanged now?

Discussed with dcamp. A recent change was made to save unnecessary UI updates on node changes, but this was too complicated with the new structure of laying out the rules. So we're reverting back to clearing and rebuilding the rule list on a node change.

> - can we make the filename:linenumber block unselectable?

Done. For the "inherited from" headers also.
Comment 68 Heather Arthur [:harth] 2012-07-11 10:59:02 PDT
Comment on attachment 641080 [details] [diff] [review]
rebased patch with fix for comments

Steven, can you take a look at this new layout for the rule view? Try builds here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/fayearthur@gmail.com-d051ad30bfe5/
Comment 69 Dave Camp (:dcamp) 2012-07-11 16:20:28 PDT
Comment on attachment 641080 [details] [diff] [review]
rebased patch with fix for comments

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

I'm happy with the code if UX is happy with the behavior/look.
Comment 70 Stephen Horlander [:shorlander] 2012-07-12 17:54:03 PDT
Comment on attachment 641080 [details] [diff] [review]
rebased patch with fix for comments

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

This looks good!

The only thing I noticed is that selecting the property changes the height of the section causing everything to shift vertically. This doesn't happen with the values. r+ with that fixed.
Comment 71 Heather Arthur [:harth] 2012-07-16 16:50:23 PDT
http://hg.mozilla.org/integration/fx-team/rev/2149897d05a0

(In reply to Stephen Horlander from comment #70)
> The only thing I noticed is that selecting the property changes the height
> of the section causing everything to shift vertically. This doesn't happen
> with the values. r+ with that fixed.

fixed. nice catch!
Comment 72 Tim Taubert [:ttaubert] 2012-07-18 03:36:51 PDT
https://hg.mozilla.org/mozilla-central/rev/2149897d05a0
Comment 73 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2012-07-19 04:24:08 PDT
-# e.g "Inherited from body#bodyID (styles.css:20)"
-rule.inheritedSource=Inherited from %S (%S)
+# e.g "Inherited from body#bodyID"
+rule.inheritedSource=Inherited from %S

I know I should probably open a new bug for this, but:

a) don't change a string like this (removing a variable) without changing its name as well
b) you should update its l10n comment (last line)

# LOCALIZATION NOTE (rule.inheritedSource): Shown for CSS rules
# that were inherited from a parent node.  Will be passed a node
# identifier and a source location.
Comment 74 Paul Rouget [:paul] 2012-07-19 12:20:48 PDT
(In reply to Francesco Lodolo [:flod] from comment #73)
> -# e.g "Inherited from body#bodyID (styles.css:20)"
> -rule.inheritedSource=Inherited from %S (%S)
> +# e.g "Inherited from body#bodyID"
> +rule.inheritedSource=Inherited from %S
> 
> I know I should probably open a new bug for this, but:
> 
> a) don't change a string like this (removing a variable) without changing
> its name as well
> b) you should update its l10n comment (last line)
> 
> # LOCALIZATION NOTE (rule.inheritedSource): Shown for CSS rules
> # that were inherited from a parent node.  Will be passed a node
> # identifier and a source location.

You're very right. Sorry, we missed that. Filed bug 775692.
Comment 75 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2012-07-19 21:00:32 PDT
(In reply to Paul Rouget [:paul] from comment #74)
> You're very right. Sorry, we missed that. Filed bug 775692.

Thanks Paul. Next I'll be less lazy and I'll open the bug myself ;-)
Comment 76 Paul Rouget [:paul] 2012-07-20 09:06:02 PDT
*** Bug 762076 has been marked as a duplicate of this bug. ***

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