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

ASSIGNED
Assigned to

Status

P3
enhancement
ASSIGNED
7 years ago
2 months ago

People

(Reporter: cedricv, Assigned: cedricv)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Suggested by jwein during AllHands.
(Assignee)

Comment 1

7 years ago
This is implemented in add-on v0.4.0.
Assignee: nobody → cedricv
(Assignee)

Comment 2

7 years ago
Created attachment 588048 [details] [diff] [review]
Open MDN when pressing F1 on a CSS property
Attachment #588048 - Flags: review?
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-
(Assignee)

Comment 4

7 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?
(Assignee)

Updated

7 years ago
Depends on: 591902
(Assignee)

Updated

7 years ago
Depends on: 713924
(Assignee)

Comment 5

7 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).
(Assignee)

Updated

7 years ago
Attachment #588048 - Flags: review?(rcampbell)
(Assignee)

Comment 6

7 years ago
Created attachment 588864 [details] [diff] [review]
patch v2 that follows pref-related conventions discussed in bug 687698
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-
(Assignee)

Comment 8

7 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.
(Assignee)

Comment 10

7 years ago
Created attachment 589099 [details] [diff] [review]
v3 - link to english MDN only
Attachment #588864 - Attachment is obsolete: true
Attachment #589099 - Flags: review?(rcampbell)
(Assignee)

Updated

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

Updated

7 years ago
Assignee: nobody → cedricv
(Assignee)

Updated

7 years ago
Blocks: 714180
(Assignee)

Comment 11

7 years ago
Created attachment 589447 [details] [diff] [review]
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.
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)

Updated

7 years ago
Attachment #589099 - Attachment is obsolete: false
(Assignee)

Updated

7 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-
(Assignee)

Updated

7 years ago
Depends on: 719453

Updated

7 years ago
Depends on: 725399
(Assignee)

Comment 14

7 years ago
This feature will likely rather use another approach using an AST.
No longer depends on: 719453, 725399
Duplicate of this bug: 725735
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

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.