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

RESOLVED FIXED in Firefox 32

Status

()

Firefox
Developer Tools: Scratchpad
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wbamberg, Assigned: bgrins)

Tracking

unspecified
Firefox 33
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

4 years ago
> (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
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 968896
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1024937
(Assignee)

Comment 4

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

Comment 5

4 years ago
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
(Assignee)

Updated

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

Comment 7

4 years ago
(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
(Assignee)

Comment 8

4 years ago
Created attachment 8442069 [details] [diff] [review]
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
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+
(Assignee)

Comment 10

4 years ago
Created attachment 8442089 [details] [diff] [review]
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.
Attachment #8442069 - Attachment is obsolete: true
Attachment #8442089 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/802926a2f654
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/802926a2f654
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
(Assignee)

Comment 13

4 years ago
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?
status-firefox30: --- → unaffected
status-firefox31: --- → unaffected
status-firefox32: --- → affected
status-firefox33: --- → fixed
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.
Backed out:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7003d83dec14
status-firefox32: fixed → affected
Flags: needinfo?(ryanvm)
(Assignee)

Comment 20

4 years ago
Created attachment 8448064 [details] [diff] [review]
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
Attachment #8448064 - Flags: review?(past)
Flags: needinfo?(bgrinstead)
Attachment #8448064 - Flags: review?(past) → review+
(Assignee)

Updated

4 years ago
Attachment #8442089 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 21

4 years ago
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+
You need to log in before you can comment on or make changes to this bug.