687707 - Style Editor, rule and computed views should display CSS documentation when pressing F1

Status: NEW

(DevTools :: Style Editor, enhancement, P3)

Description

[Cedric Vivier [:cedricv]]

PRessing F1 when caret is on a property or value name should display CSS documentation.

Suggested by jwein during AllHands.

Comment 1

[Cedric Vivier [:cedricv]]

This is implemented in add-on v0.4.0.

Comment 2

[Cedric Vivier [:cedricv]]

Attachment: Open MDN when pressing F1 on a CSS property

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 588048 [details] [diff] [review]
Open MDN when pressing F1 on a CSS property

MDN doesn't support all locales that Firefox supports. I think general.useragent.locale is deprecated, too.

Comment 4

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 588048 [details] [diff] [review]
> Open MDN when pressing F1 on a CSS property
> 
> MDN doesn't support all locales that Firefox supports.

This sounds like a MDN bug. If a locale is not available, MDN should redirect to en-US or whatever makes more sense wrt the requested locale.


> I think general.useragent.locale is deprecated, too.

What is the recommended way to get the user locale?

Comment 5

[Cedric Vivier [:cedricv]]

CssHtmlTree.jsm:510:  this.link = "https://developer.mozilla.org/en/CSS/" + aName;(In reply to Cedric Vivier [cedricv] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Comment on attachment 588048 [details] [diff] [review]
> > Open MDN when pressing F1 on a CSS property
> > 
> > MDN doesn't support all locales that Firefox supports.
> 
> This sounds like a MDN bug. If a locale is not available, MDN should
> redirect to en-US or whatever makes more sense wrt the requested locale.

Ideally, we would fix MDN bug 713924 so that documentation links are locale neutral.

FWIW, the similar feature that already landed in the Style Inspector does not handle locales :

"""
CssHtmlTree.jsm:510:  this.link = "https://developer.mozilla.org/en/CSS/" + aName;
"""

If sending users to the English MDN is preferred we should do the change in this patch... or do what this patch does in Style Inspector as well for consistency.

Alternatively, it might be a good idea to add a new "devtools.locale" pref since DevTools have slightly different localization needs than the rest of Firefox (eg. our strings are not required to be translated iirc).

Comment 6

[Cedric Vivier [:cedricv]]

Attachment: patch v2 that follows pref-related conventions discussed in bug 687698

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 588864 [details] [diff] [review]
patch v2 that follows pref-related conventions discussed in bug 687698

>+const LOCALE_PREF = "general.useragent.locale";
>+const MDN_CSS_URL = "https://developer.mozilla.org/{locale}/CSS/{token}";

I explicitly r-'d this already :/

Until bug 713924 is fixed, you should link to the English version.

Comment 8

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 588864 [details] [diff] [review]
> patch v2 that follows pref-related conventions discussed in bug 687698
> >+const LOCALE_PREF = "general.useragent.locale";
> I explicitly r-'d this already :/

Sorry Dao, consider this as a mere patch queue update to make it still apply after the  pref change in bug 687698...


> Until bug 713924 is fixed, you should link to the English version.

I didn't get answer to my questions in comment #5 until now hence why I didn't change the behavior yet.

Will update the patch to link to EN locale only...

Comment 9

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

sorry, I was slow on the response regarding localized links. en/CSS seems to be the way to go.

Comment 10

[Cedric Vivier [:cedricv]]

Attachment: v3 - link to english MDN only

Comment 11

[Cedric Vivier [:cedricv]]

Attachment: v4 - just remove unnecessary event handling

So, looking back at this... I end up thinking the whole event handling is kinda unnecessary complexity anyways - we can just use the timer-based fallback every time.

Updated patch.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 589447 [details] [diff] [review]
v4 - just remove unnecessary event handling

wrong bug!

Comment 13

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

Comment on attachment 589099 [details] [diff] [review]
v3 - link to english MDN only

@@ -66,16 +66,19 @@ const TRANSITION_DURATION_MS = 500; /* sync with TRANSITION_RULE */
 const TRANSITION_RULE = "\
 :root.moz-styleeditor-transitioning, :root.moz-styleeditor-transitioning * {\

this'll need another rebasing since landing the transition patch.

in function SE_getTokenAtCursor():

+    let text = this._sourceEditor.getText();

that is the whole file. Could we limit our testing to just the line at cursor?

+    while (!/[\s;:\{\}\[\]\"\']/.test(text[end] || " ")) {
+      end++;
+    }
+    while (!/[\s;:\{\}\[\]\"\']/.test(text[start] || " ")) {
+      start--;
+    }

these make me a little concerned.

First, the regex is repeated, add it to a const and reuse it in each loop.

"test the character at text[position] if it is in the range of non-space characters or allowed punctuation" is how I read those. True?

if test fails or " " (space)?

increment or decrement the boundaries.

Are these tests ever going to be reused? Should we make an isValidCSSCharacter method to test the characters and hide that regex from the getTokenAtCursor method altogether?

+  openInfoForTokenAtCursor: function SE_openInfoForTokenAtCursor()
+  {
+    let token = this.getTokenAtCursor();
+    if (!token.text.length ||
+        !/\s/.test(token.previousCharacter) || token.nextCharacter != ":") {
+      return;
+    }

what happens here if we're in a selector with a pseudo-class like ::hover as the next character?

R- for this pass. Please forgive me! :)

Comment 14

[Cedric Vivier [:cedricv]]

This feature will likely rather use another approach using an AST.

Comment 16

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

This bug also applies to the Rule and Computed views.

Comment 17

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 18

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).