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

NEW
Unassigned

Status

enhancement
P3
normal
8 years ago
9 months ago

People

(Reporter: cedricv, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

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

Suggested by jwein during AllHands.
Reporter

Comment 1

8 years ago
This is implemented in add-on v0.4.0.
Assignee: nobody → cedricv
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.
Attachment #588048 - Flags: review? → review-
Reporter

Comment 4

8 years ago
(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?
Reporter

Updated

8 years ago
Depends on: 591902
Reporter

Updated

8 years ago
Depends on: 713924
Reporter

Comment 5

8 years ago
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).
Reporter

Updated

8 years ago
Attachment #588048 - Flags: review?(rcampbell)
Reporter

Comment 6

8 years ago
Attachment #588048 - Attachment is obsolete: true
Attachment #588048 - Flags: review?(rcampbell)
Attachment #588864 - Flags: review?(rcampbell)
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.
Attachment #588864 - Flags: review?(rcampbell) → review-
Reporter

Comment 8

8 years ago
(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...
sorry, I was slow on the response regarding localized links. en/CSS seems to be the way to go.
Reporter

Comment 10

8 years ago
Attachment #588864 - Attachment is obsolete: true
Attachment #589099 - Flags: review?(rcampbell)
Reporter

Updated

8 years ago
Assignee: cedricv → nobody
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Style Editor
QA Contact: developer.tools → developer.tools.style.editor
Reporter

Updated

8 years ago
Assignee: nobody → cedricv
Reporter

Updated

8 years ago
Blocks: 714180
Reporter

Comment 11

8 years ago
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.
Attachment #589099 - Attachment is obsolete: true
Attachment #589099 - Flags: review?(rcampbell)
Attachment #589447 - Flags: review?(dao)
Comment on attachment 589447 [details] [diff] [review]
v4 - just remove unnecessary event handling

wrong bug!
Attachment #589447 - Attachment is obsolete: true
Attachment #589447 - Flags: review?(dao)
Attachment #589099 - Attachment is obsolete: false
Reporter

Updated

8 years ago
Attachment #589099 - Flags: review?(rcampbell)
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! :)
Attachment #589099 - Flags: review?(rcampbell) → review-
Reporter

Updated

8 years ago
Depends on: 719453
Depends on: 725399
Reporter

Comment 14

8 years ago
This feature will likely rather use another approach using an AST.
No longer depends on: 719453, 725399
This bug also applies to the Rule and Computed views.
Summary: Style Editor should display CSS documentation when pressing F1 → Style Editor, rule and computed views should display CSS documentation when pressing F1
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3

Updated

Last year
Product: Firefox → DevTools
Assignee: cedricv → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.