Closed Bug 1543261 Opened 5 years ago Closed 5 years ago

Opening conditional panel with keyboard shortcut should display previously saved condition

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox71 --- fixed

People

(Reporter: anthonyxie64, Assigned: arosenfeld2003, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: good-first-bug, Whiteboard: [debugger-reserve])

Attachments

(2 files)

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

  1. Open debugger on a page and right click on a gutter line number
  2. Popup menu should appear, click "add condition"
  3. Conditional panel should open, add a condition
  4. Press Enter to save conditional breakpoint, conditional panel should close
  5. Click on the line with conditional breakpoint in the editor
  6. Now use the keyboard shortcut to open the conditional panel on that line (Ctrl+Shift+B or Cmd+Shift+B)

What happened?

Actual Result: Conditional panel opens without previously saved condition

What should have happened?

Expected Result: Conditional panel opens populated with previously saved condition

Anything else we should know?

If a conditional breakpoint is created using the keyboard shortcut, we do not see this problem

Keywords: good-first-bug
Priority: -- → P3

Since we now use column breakpoints, this requires a bit more thought. A few things to think about:

  • The gutter marker represents the first activated or potential breakpoint in the line.

  • What do we do if there's an active column breakpoint and another existing conditional breakpoint on the line?

Hi David and Anthony. I've actually been working on this and came up with a potential solution to the original bug:

It involves a couple changes to src/components/Editor/ConditionalPanel.js:

  • the function getBreakpointForLocation returns only the location, while we want the breakpoint object from state (which includes the condition).

  • importing and replacing this with the similarly named function getBreakpointAtLocation from src/selectors/breakpointAtLocation.js solves the problem.

Here's a demo:
http://g.recordit.co/pXQd2RIzdC.gif

This causes a flow error which I am still investigating:

Cannot call `getBreakpointAtLocation` with `location` bound to `location` 
because  null [1] is incompatible with  `LineColumn` [2].Flow(InferError)
ui.js(177, 4): [1] null 
breakpointAtLocation.js(66, 65): [2] `LineColumn`

I was also thinking about the case of additional column breakpoints...
Currently (as you stated above) my solution always opens the gutter breakpoint (first breakpoint on the line).

Would it be preferable for the shortcut to open the column breakpoint closest to where the user has clicked?
Or, maybe we want to display all the column breakpoints on the line and allow the user to choose one using the keyboard?

I'm happy to continue working on this...
Thanks.

Priority: P3 → P2
Status: UNCONFIRMED → NEW
Ever confirmed: true

One approach for dealing with column breakpoints would be to select the closest to the cursor position (when conditional panel is opened).

(possibly with getBreakpointsAtLine from src/selectors/breakpointAtLocation.js.)

The cursor position is rendered to the DOM inside the Editor/SourceFooter, src/components/Editor/Footer.js, and I'm contemplating how to pass cursorPosition and line breakpoints to the Props in src/components/Editor/ConditionalPanel.js.

Does this seem like a reasonable approach for handling opening conditional panel with the keyboard shortcut?
Is it reasonable to create a new action that would get the cursor position?

I think using the cursor position would be a good idea! I don't think we need a new action but a function in the utils/editor directory. We'll probably find some edge cases along the way. Worst case scenario is we default to the first column position and use that for the conditional add/edit.

Previously, the conditional panel did not correctly load breakpoint conditions when opened with the keyboard shortcut.
This patch loads the conditions, and selects the closest active column breakpoint on same line as cursor position in the Editor.

Attachment #9067943 - Attachment description: Conditional Panel: Load correct conditions when opened with keyboard shortcut (Bug 1543261). Add functionality of selecting the closest active column breakpoint based on current cursorPosition in Editor. r=jlast, davidwalsh → Conditional Panel: Load correct conditions when opened with keyboard shortcut (Bug 1543261).Add functionality of selecting the closest active column breakpoint based on current cursorPosition in Editor. r=jlast, davidwalsh
Whiteboard: [debugger-reserve]
Blocks: 1565711
Blocks: 1565713
Assignee: nobody → arosenfeld2003
Status: NEW → ASSIGNED
No longer blocks: 1565711
No longer blocks: 1565713
Blocks: dbg-71
No longer blocks: dbg-70
Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/458af3b79e09
Conditional Panel: Load correct conditions when opened with keyboard shortcut .Add functionality of selecting the closest active column  breakpoint based on current cursorPosition in Editor.  r=davidwalsh
Attachment #9067943 - Attachment description: Conditional Panel: Load correct conditions when opened with keyboard shortcut (Bug 1543261).Add functionality of selecting the closest active column breakpoint based on current cursorPosition in Editor. r=jlast, davidwalsh → Bug 1543261 - Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh
Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1630eaa0a61c
Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2f41d635be8
Backed out changeset 1630eaa0a61c for causing devtools failure on browser_dbg-breakpoints-cond-source-maps.js CLOSED TREE
Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25a282bca05b
Conditional Panel: Load correct conditions when opened with keyboard shortcut r=davidwalsh
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
QA Whiteboard: [qa-71b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: