723890 - Improve "<inline style sheet #n>" label

Status: NEW

(DevTools :: Style Editor, enhancement, P3)

Description

[Cedric Vivier [:cedricv]]

The sequential order (ie. #n) is not very useful, the order in the list already gives that information.

It would be more useful to show the line number of the owner style element in the document (when possible).

Possible representations (that I think of) :

A) <inline style sheet:42>
B) <inline style sheet at line 42>
C) <style:42>
D) <style> at line 42

Any preference?

Comment 1

[Kevin Dangoor]

(In reply to Cedric Vivier [:cedricv] from comment #0)
> Possible representations (that I think of) :
> 
> A) <inline style sheet:42>
> B) <inline style sheet at line 42>
> C) <style:42>
> D) <style> at line 42
> 
> Any preference?

I like D because it seems easiest to interpret.

Any thoughts on style sheets that are created via the New button?

<style> created by Style Editor

?

Comment 2

[Cedric Vivier [:cedricv]]

(In reply to Kevin Dangoor from comment #1)
> (In reply to Cedric Vivier [:cedricv] from comment #0)
> > D) <style> at line 42
> > 
> > Any preference?
> 
> I like D because it seems easiest to interpret.

D it is then.


> Any thoughts on style sheets that are created via the New button?
> <style> created by Style Editor
> ?

Isn't "New style sheet" as it is currently straightforward enough?

Comment 3

[Kevin Dangoor]

(In reply to Cedric Vivier [:cedricv] from comment #2)
> Isn't "New style sheet" as it is currently straightforward enough?

Err, yeah. I was thinking that New created more "inline style sheet"s, but that's not how it's presented... what we've got looks good there

Comment 4

[Savita TS]

Attachment: "<inline style sheet #n>" changed to "<style> at line n"

Comment 5

[Joe Walker [:jwalker]]

It's not uncommon for things to dynamically add <style> elements, but they become hard to distinguish (and by 'hard' I mean 'practically impossible') because all you get is a #number.

Could we do "<style> at line 42" when the style does not have an id, and "<style id=foo>" when it does? That way people can add style elements with an id and have them distinguishable?

Comment 6

[Jignesh Kakadiya [:jhk]]

> Could we do "<style> at line 42" when the style does not have an id, and
> "<style id=foo>" when it does? That way people can add style elements with
> an id and have them distinguishable?

Is there any way to detect if style element has id or not? here http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditor.jsm#353.

Comment 7

[Joe Walker [:jwalker]]

I would think that you should be able to use 'this.styleSheet.ownerNode.id'.

Comment 8

[Jignesh Kakadiya [:jhk]]

Attachment: Patch(v1)

Comment 9

[Paul Rouget [:paul]]

(In reply to Jignesh Kakadiya [:jhk] from comment #8)
> Created attachment 630937 [details] [diff] [review]
> Patch(v1)

That looks great.
One thing: we can't change the value of a property. We have to change its name (it's much easier for the l10n process). So rename "inlineStyleSheet" to "inlineStyleSheet2" or, even better, to "inlineStyleSheetWithIndex".

Comment 10

[Jignesh Kakadiya [:jhk]]

Attachment: Patch(v2)

Thanks Paul.

Fixed it.

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 631016 [details] [diff] [review]
Patch(v2)

r+! Thank you :)
Can you please change these 2 things? Then I'll land you code.

> +      return id ? _("inlineStyleSheetWithId", id) : _("inlineStyleSheetWithIndex", index);

Can you break that into a simple if () {} block (because the line is getting rather long).

--

> +#LOCALIZATION NOTE (inlineStyleSheetWithId)
> +# This is name used for style sheet that is declared inline in the <style> element
> +# and that style element have id.

Can you update your comment to:

> +# This is the name used for a style sheet that is declared inline in the <style> element
> +# and that has an ID.

Comment 12

[Jignesh Kakadiya [:jhk]]

Attachment: Patch(v3)

Fixed above comments. Patch to Land.

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 631036 [details] [diff] [review]
Patch(v3)

My bad, I've been to fast.

You use the position of the <style> element as its line number. And you should not.

"index" is the position (as in "<style> number 3").
You need to get the actual line number.

Sorry for the precipitated review :/

Comment 14

[Paul Rouget [:paul]]

Would anyone know an easy way to get the line number of the <style> element?

Comment 15

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #14)
> Would anyone know an easy way to get the line number of the <style> element?

CssLogic:1424 does this I think.

Comment 16

[Jignesh Kakadiya [:jhk]]

(In reply to Joe Walker from comment #15)
> (In reply to Paul Rouget [:paul] from comment #14)
> > Would anyone know an easy way to get the line number of the <style> element?
> 
> CssLogic:1424 does this I think.

I looked at that and it seems it returns line of the parent CSSStyleRule in the parent CSSStyleSheet and what we want is line of CSSStyleSheet inside Document.I guess it should be property of (CssSheet)CssLogic.jsm#913. As it handles all the stuff related to stylesheet. Right now all we get is the position/index of the stylesheet. May be we can get it from API which does initial parsing of documents style elements. But I don't know where it does that. Any one knows it?

Comment 17

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

I updated the previous patch and tried to do an approach to get the line number. However, it's still not good enough and doesn't show the exact line, because I wasn't able to get the whole doc text to count all the lines breaks. 
Maybe someone else has an idea on how to improve it?

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 668240 [details] [diff] [review]
Patch v4

That seems fragile. But I can't think of any better way to do it :( This might require some platform work.

Comment 19

[Tiziana Sellitto [:tiziana]]

I'm going through older "good first bugs" that have been inactive for a while.
Paul, this one seems to be blocked. Do you think it is changed something in the meanwhile?

Comment 20

[Paul Rouget [:paul]]

Not really good-first-bug anymore.

Comment 21

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).