Closed Bug 1026560 Opened 5 years ago Closed 5 years ago

clicking the [docs] link in the type information popup in Scratchpad should open the MDN page in a new window

Categories

(DevTools :: Scratchpad, defect)

defect
Not set

Tracking

(firefox30 unaffected, firefox31 unaffected, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
Firefox 33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: wbamberg, Assigned: bgrins)

References

Details

Attachments

(2 files, 1 obsolete file)

I've been playing with the (amazing) code completion and type information stuff built into Scratchpad by bug 968896. If you press Shift-Space to bring up the type info popup, it includes a [docs] link. If you click that link, the MDN page is loaded in the Scratchpad window. Now there's no way to get back to your pad.

(Also, I think the popup doesn't appear for long enough. Maybe it would be better if it persisted until you press Escape or click outside it? Worth a different bug?)
> (Also, I think the popup doesn't appear for long enough. Maybe it would be
> better if it persisted until you press Escape or click outside it? Worth a
> different bug?)

Yeah, we should probably have a new bug for this.  One other minor annoyance that I've noticed is that if I type: `function ()` and am still holding shift from the parens when I press space then a space doesn't get added and this intellisense pops up.

I don't really know if there is a good workaround for this, other than changing the keyboard shortcut.  I guess we could make it so that it doesn't show up if there have been characters typed within the last N milliseconds, but I'd have to play with it to see if that is helpful.  It could just be me that bumps into this, but it seems like it happens much of the time I write a new function definition.
OS: Mac OS X → All
Hardware: x86 → All
This is pretty annoying.  We should just pop open the docs in a new window instead of loading the page in the scratchpad frame.
Summary: clicking the [docs] link in the type information popup in Scratchpad is a bad thing to do → clicking the [docs] link in the type information popup in Scratchpad should open the MDN page in a new window
Blocks: 968896
Duplicate of this bug: 1024937
This seems to be happening within tern: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/tern.js#235.  We generally don't want to modify the library files, but we may have to make an exception (as we do with a couple of things in Code Mirror).
May as well bump CodeMirror to the latest version while we are at it so we can update to the latest tern plugin
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Depends on: 1026811
Perhaps there is a configuration option or API change that we can propose to the author so that we wouldn't need to patch upstream forever?
(In reply to Panos Astithas [:past] from comment #6)
> Perhaps there is a configuration option or API change that we can propose to
> the author so that we wouldn't need to patch upstream forever?

Actually, it appears that there is already an option that lets us specify our own element - typeTip.  I just overlooked it in the same function earlier: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/tern.js#228
No longer depends on: 1026811
Attached patch tern-doc-link.patch (obsolete) — Splinter Review
Moves the templating out of the tern.js file and into autocomplete.js.  Also uses l10n for the 'docs' string - though the rest of the docs will still come through however tern delivers them (I'm assuming always English).

Here is the old templating function, for reference: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/tern.js#224.  

https://tbpl.mozilla.org/?tree=Try&rev=b2232ddfadca
Attachment #8442069 - Flags: review?(past)
Comment on attachment 8442069 [details] [diff] [review]
tern-doc-link.patch

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

I wonder if it would be better UX to open the link in the existing browser window the scratchpad targets, but it looks great even like this.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +19,5 @@
>  function setupAutoCompletion(ctx, options) {
>    let { cm, ed, Editor } = ctx;
>  
>    let win = ed.container.contentWindow.wrappedJSObject;
> +  let {CodeMirror,document} = win;

Nit: space after comma and around curly braces, if you don't mind.

::: browser/locales/en-US/chrome/browser/devtools/sourceeditor.properties
@@ +50,5 @@
>  annotation.debugLocation.title=Current step: %S
>  
> +# LOCALIZATION NOTE  (autocompletion.docsLink): This is the text shown on
> +# the link inside of the documentation popup.  If you type 'document' in Scratchpad
> +# then press Shift+Space you can see the popup

Nit: missing period.
Attachment #8442069 - Flags: review?(past) → review+
Updates whitespace and grammar.

> I wonder if it would be better UX to open the link in the existing browser window the scratchpad targets, but it looks great even like this.

Yeah, that would be better - though we would then have to expose a new option to the editor (like autocompleteDocWindow or something) so that Scratchpad can access this option and update it whenever the target changes.  I think _blank is a good enough fix for this issue that we can uplift and get this weirdness resolved - and we could improve the UX in a follow up.
Attachment #8442069 - Attachment is obsolete: true
Attachment #8442089 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/802926a2f654
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Comment on attachment 8442089 [details] [diff] [review]
tern-doc-link-r=past.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 968896
User impact if declined: If you click on a [docs] link in Scratchpad, the MDN page is loaded in the Scratchpad window. Now there's no way to get back to your pad.
Testing completed (on m-c, etc.): On m-c since 6-19
Risk to taking this patch (and alternatives if risky): Not much risk - it is limited to the templating of the autocomplete popup inside of Scratchpad
String or IDL/UUID changes made by this patch: Inside of sourceeditor.properties, the following strings were added:
1) autocompletion.docsLink=docs
2) autocompletion.notFound=not found
Attachment #8442089 - Flags: approval-mozilla-aurora?
Comment on attachment 8442089 [details] [diff] [review]
tern-doc-link-r=past.patch

Aurora approval granted.
Attachment #8442089 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Why was this patch allowed to land and break string freeze on mozilla-aurora? This shouldn't happen, unless things are utterly broken.

Wasn't it possible to just fix the issue with the link, and let the full fix (with strings) ride the train?
Flags: needinfo?(lmandel)
Francesco - I don't have a good explanation for how I messed this one up but this is clearly my mistake. The process should be:
1. discuss whether the issue can be addressed without a string change/addition
2. if a string change is required and the fix must be taken, review with l10n to see if you can handle the change before approving for landing

Given that we still have time in this cycle, let's reset.

Ryan - Can you please assist with a backout.

Brian - Can this issue be addressed in 32 as Francesco suggested by opening the MDN link in a new window without any associated messages to the user? If so, can you please create a branch specific patch that does that and allows us to preserve string freeze?
Flags: needinfo?(lmandel) → needinfo?(ryanvm)
Flags: needinfo?(bgrinstead)
(In reply to Lawrence Mandel [:lmandel] from comment #17)
> Francesco - I don't have a good explanation for how I messed this one up but
> this is clearly my mistake. The process should be:
> 1. discuss whether the issue can be addressed without a string
> change/addition
> 2. if a string change is required and the fix must be taken, review with
> l10n to see if you can handle the change before approving for landing

Thanks, that's absolutely the right way to proceed.
Branch specific patch for Aurora (inlined the strings for the template instead of using new strings in sourceeditor.properties).  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c6f7a34a719e
Attachment #8448064 - Flags: review?(past)
Flags: needinfo?(bgrinstead)
Attachment #8448064 - Flags: review?(past) → review+
Attachment #8442089 - Flags: approval-mozilla-aurora+
Comment on attachment 8448064 [details] [diff] [review]
docs-link-aurora.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 968896
User impact if declined: If you click on a [docs] link in Scratchpad, the MDN page is loaded in the Scratchpad window. Now there's no way to get back to your pad.
Testing completed (on m-c, etc.): On m-c since 6-19
Risk to taking this patch (and alternatives if risky): Not much risk - it is limited to the templating of the autocomplete popup inside of Scratchpad
String or IDL/UUID changes made by this patch: None
Attachment #8448064 - Flags: approval-mozilla-aurora?
Comment on attachment 8448064 [details] [diff] [review]
docs-link-aurora.patch

Thanks for creating a solution that doesn't require string changes. Aurora+
Attachment #8448064 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.