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

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Will Bamberg [:wbamberg]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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

Comment 2

[Brian Grinstead [:bgrins]]

This is pretty annoying.  We should just pop open the docs in a new window instead of loading the page in the scratchpad frame.

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[Brian Grinstead [:bgrins]]

May as well bump CodeMirror to the latest version while we are at it so we can update to the latest tern plugin

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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?

Comment 7

[Brian Grinstead [:bgrins]]

(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

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: tern-doc-link.patch

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

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: tern-doc-link-r=past.patch

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/802926a2f654

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/802926a2f654

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8442089 [details] [diff] [review]
tern-doc-link-r=past.patch

Aurora approval granted.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c738b09cda1

Comment 16

[Francesco Lodolo [:flod]]

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?

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 18

[Francesco Lodolo [:flod]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7003d83dec14

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: docs-link-aurora.patch

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

Comment 21

[Brian Grinstead [:bgrins]]

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

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8448064 [details] [diff] [review]
docs-link-aurora.patch

Thanks for creating a solution that doesn't require string changes. Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7228e871d72e