Closed Bug 965172 Opened 6 years ago Closed 6 years ago

Variables view hover popup appears while selecting text.

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: Optimizer, Assigned: rricard)

References

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js])

Attachments

(1 file, 6 obsolete files)

When debugger is paused, try to select some text which spans over a valid variable.

If you are not fast enough, the popup will open and break your selection.

Expected result:

The popup should not appear while anything is being selected.
https://wiki.mozilla.org/DevTools/Hacking

Would need to add a check to make sure the user wasn't selecting text before opening the popup[0]. You can tell the user is *not* selecting text if |window.getSelection().toString() == ""| is truthy[1].

You should also write a new test for this behavior. See [2] for a similar test.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1964
[1] https://developer.mozilla.org/en-US/docs/Web/API/Window.getSelection
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_variables-view-popup-01.js
Priority: -- → P3
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
I can definitely try to fix that ! I am already used to your workflow (solved 958111), but I may need some information on this specific part of the code, although I could figure it out by myself. Anyway, I think I can handle this by myself. So if you're ok, let's do this !
Oh ! And if you've got a good test case (JS+HTML) to reproduce it correctly it would help me a lot !
hi, I was wondering if I could be assigned to this bug, as I have good understanding of html and javascript. This would also be a first good bug, for me to attempt.
Sorry Taj, Ricard got here first :)

Ricard, see comment 1 for an initial overview of what is going on. here is a test case: http://htmlpad.org/debugger-text-select/

Just start slecting "paragraph" above the debugger statement after you click the button with the debugger open.
Assignee: nobody → ricard.robin
Status: NEW → ASSIGNED
It seems to be more tricky than I expected, window.getSelection() is always empty in the context of a XUL window. I'll try to see if I can get access to the underlying code view.
Hmm you're right, but it is actually easy :)

Our editor has a method via CodeMirror called |somethingSelected| which returns true if something is selected.

  if (DebuggerView.editor.somethingSelected()) {
    ...
  }
That's even better ! Thanks !
Attached patch 0001-Basic-Fix.patch (obsolete) — Splinter Review
OK, let's start with this, just to have your feedback. It's deadly simple code, nothing really impressing, I'll work on the tests and create a well-formed patch tomorrow.
Attachment #8383975 - Flags: feedback?(nfitzgerald)
Nice side effect, it seems to neutralize 974542 !
Comment on attachment 8383975 [details] [diff] [review]
0001-Basic-Fix.patch

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

On the right track.

In the future, you can use the "feedback ? :fitzgen" flag when attaching the patch if you'd like some feedback.

::: browser/devtools/debugger/debugger-panes.js
@@ +1965,5 @@
>      // Prevent the variable inspection popup from showing when the thread client
> +    // is not paused, or while a popup is already visible, or when the user tries
> +    // to select text in the editor.
> +    let hasSelect = DebuggerView.editor.somethingSelected();
> +    if (gThreadClient && gThreadClient.state != "paused" || !this._tooltip.isHidden() || hasSelect) {

Yup.

From a style nit picking point of view, this would be a little better like:

  if (gThreadClient && gThreadClient.state != "paused"
      || !this._tooltip.isHidden()
      || DebuggerView.editor.somethingSelected()) {
    ...
  }
Attachment #8383975 - Flags: feedback+
(In reply to ricard.robin from comment #10)
> Nice side effect, it seems to neutralize 974542 !

Well, the underlying gecko bug should still be fixed but it is good that we are reducing the easy ways to trigger it.
Attachment #8383975 - Flags: feedback?(nfitzgerald)
Attached patch 0002-Style-refactoring.patch (obsolete) — Splinter Review
Here's the patch for Style refactoring
Attachment #8384138 - Flags: feedback?(nfitzgerald)
Here's the patch for the test.

If you give me good feedback, I'll amend all the patches in one and submit it for review.
Attachment #8384139 - Flags: feedback?(nfitzgerald)
Already squashed it into one patch ... Need your feedback and review anyway ! Thanks !
Attachment #8383975 - Attachment is obsolete: true
Attachment #8384138 - Attachment is obsolete: true
Attachment #8384139 - Attachment is obsolete: true
Attachment #8384138 - Flags: feedback?(nfitzgerald)
Attachment #8384139 - Flags: feedback?(nfitzgerald)
Attachment #8384146 - Flags: review?(nfitzgerald)
Comment on attachment 8384146 [details] [diff] [review]
0001-Bug-965172-Deactivate-variables-view-hover-popup-whi.patch

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

Just have a couple comments, mostly nit picks. Fix up the patch based on my comments and everything should be good to land. Flag me for review once more and I'll do a super quick once over then we can get it in the tree :)

::: browser/devtools/debugger/test/browser_dbg_variables-view-popup-14.js
@@ +8,5 @@
> +
> +const TAB_URL = EXAMPLE_URL + "doc_frame-parameters.html";
> +
> +function test() {
> +  Task.spawn(function() {

Use ES6 generator functions, make sure to include the star: |function*|

@@ +15,5 @@
> +    let bubble = win.DebuggerView.VariableBubble;
> +
> +    // Allow this generator function to yield first.
> +    executeSoon(() => debuggee.start());
> +    yield waitForSourceAndCaretAndScopes(panel, ".html", 24);

Instead of calling |executeSoon|, could you do something like this:

  const ready = waitForSourceAndCaretAndScopes(panel, ".html", 24);
  debuggee.start();
  yield ready;

Generally, |executeSoon| is a very scary thing to have in tests, given our history of race conditions and timing issues in tests that result in intermittent failures that are super hard to track down.

::: browser/devtools/debugger/test/head.js
@@ +603,4 @@
>    return promise.all([popupShown, fetchedProperties]).then(waitForTick);
>  }
>  
> +function intendOpenVarPopup(aPanel, aCoords) {

Can we make this function a little more general? Like have its promise resolve true if the popup opened and false otherwise? Then in the test assert that it resolved false. basically just moving a little of the logic from the test to here so that it is a little more reusable.

@@ +606,5 @@
> +function intendOpenVarPopup(aPanel, aCoords) {
> +  let bubble = aPanel.panelWin.DebuggerView.VariableBubble;
> +  let editor = aPanel.panelWin.DebuggerView.editor;
> +
> +  let { left, top } = editor.getCoordsFromPosition(aCoords);

Should you rename |aCoords| to |aPosition|? Sure looks like it from this call.

@@ +613,5 @@
> +  const deferred = promise.defer();
> +  window.setTimeout(
> +    function() {
> +      deferred.resolve(null);
> +    }, bubble._tooltip.defaultShowDelay

Style nit: move the delay to its own line, please.
Attachment #8384146 - Flags: review?(nfitzgerald)
Comment on attachment 8384146 [details] [diff] [review]
0001-Bug-965172-Deactivate-variables-view-hover-popup-whi.patch

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

::: browser/devtools/debugger/debugger-panes.js
@@ +1966,5 @@
> +    // is not paused, or while a popup is already visible, or when the user tries
> +    // to select text in the editor.
> +    if (gThreadClient && gThreadClient.state != "paused"
> +        || !this._tooltip.isHidden()
> +        || DebuggerView.editor.somethingSelected()) {

Just a drive-by question.

This works while selecting text, but not after a selection was made. So if some random text is selected in the editor, it's not possible to open an inspection popup anymore. That sounds inconvenient to me, but it's up for discussion.
(In reply to Victor Porof [:vporof][:vp] from comment #17)
> Comment on attachment 8384146 [details] [diff] [review]
> 0001-Bug-965172-Deactivate-variables-view-hover-popup-whi.patch
> 
> Review of attachment 8384146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1966,5 @@
> > +    // is not paused, or while a popup is already visible, or when the user tries
> > +    // to select text in the editor.
> > +    if (gThreadClient && gThreadClient.state != "paused"
> > +        || !this._tooltip.isHidden()
> > +        || DebuggerView.editor.somethingSelected()) {
> 
> Just a drive-by question.
> 
> This works while selecting text, but not after a selection was made. So if
> some random text is selected in the editor, it's not possible to open an
> inspection popup anymore. That sounds inconvenient to me, but it's up for
> discussion.

Good Catch(tm).

We should also check that the event's |buttons| property is 0 (meaning no mouse buttons are being pressed). https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.buttons
Or I guess in this check, we would actually want to know if the buttons property is not 0.
Ok, I'll investigate that in a few days, given I have some other things to work on the next few days. But I want to land it before the end of the week, so I'll work on that ASAP !
Ok, I'm working on it but it raised me some questions. I can fix the behavior quite quickly but it will be harder for the fix you ask me in the tests. First, I my new test case from existing tests (ie. I structurally did kind of the same things...) And the generators and yield part you ask me to fix can be found in previous tests of the browser_dbg_variables-view-popup-*.js series. In fact, when I try to do like you advise me to do, I end up with a timeout.

Anyway, I'll give you another patch soon with what I'm able to fix !
Attached patch 965172.patch (obsolete) — Splinter Review
Ok, I took in account the remark of Victor and I almost did everything you asked me but I can't get rid of |executeSoon|. And I find it weird. Either it's me that still don't understand ES6 Generators + Promises or there's something wrong in |waitForSourceAndCaretAndScopes| ... I'll investigate that later but again, every test in this serie uses |executeSoon|. So if it's wrong, we may fire another bug to refactor that, I think ...
Attachment #8384146 - Attachment is obsolete: true
Attachment #8386913 - Flags: review?(nfitzgerald)
Attached patch 965172.patch (obsolete) — Splinter Review
Uh oh ! Forgot to translate it to hg patch !
Attachment #8386913 - Attachment is obsolete: true
Attachment #8386913 - Flags: review?(nfitzgerald)
Attachment #8386915 - Flags: review?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] from comment #16)
> 
> @@ +15,5 @@
> > +    let bubble = win.DebuggerView.VariableBubble;
> > +
> > +    // Allow this generator function to yield first.
> > +    executeSoon(() => debuggee.start());
> > +    yield waitForSourceAndCaretAndScopes(panel, ".html", 24);
> 
> Instead of calling |executeSoon|, could you do something like this:

Nick, Robin, using executeSoon in this particular case is fine.
Comment on attachment 8386915 [details] [diff] [review]
965172.patch

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

Looks pretty good!

Here is a try push, which will run the tests on all of our platforms: https://tbpl.mozilla.org/?tree=Try&rev=5386323fb1d1

If you supply the final updated patch and the try push looks good, we'll land this :)

::: browser/devtools/debugger/test/head.js
@@ +603,4 @@
>    return promise.all([popupShown, fetchedProperties]).then(waitForTick);
>  }
>  
> +function intendOpenVarPopup(aPanel, aPosition, aButtonPushed) {

Should have a comment documenting the return value of this function and what it means if the promise resolves true or false.
Attachment #8386915 - Flags: review?(nfitzgerald) → review+
Attached patch 965172.patchSplinter Review
Well, tbpl seems to accept it ! Here it is with comments added !

Thank you for guiding me through this bug, I feel I get better at solving mozilla bugs. But that's just the beginning ... and I quite enjoy it !
Attachment #8386915 - Attachment is obsolete: true
Attachment #8388093 - Flags: review?(nfitzgerald)
Attachment #8388093 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/integration/fx-team/rev/1d74cad8ff32
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1d74cad8ff32
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.