Cannot open the expression panel of conditional breakpoint

RESOLVED FIXED in Firefox 48

Status

P1
normal
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: magicp.jp, Assigned: jlast)

Tracking

unspecified
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8733520 [details]
conditional-breakpoint-expression-panel.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160322030417

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. about:home)
3. Open Debugger
4. Add two or more conditional breakpoint
5. Open the expression panel selecting conditional breakpoint detail
6. Select next conditional breakpoint detail remain opening the expression panel


Actual results:

Cannot open the expression panel of conditional breakpoint. (cannot keep open, because immediately close automatically)

Reproduce on Nightly, but other versions will have same issue. Because "Add conditional breakpoint" does not work correctly in other versions now.


Expected results:

The expression panel should open when selecting conditional breakpoint detail.
(Reporter)

Updated

3 years ago
Has STR: --- → yes
status-firefox48: --- → affected
Component: Untriaged → Developer Tools: Debugger
OS: Unspecified → All
Hardware: Unspecified → All
This looks like a regression. Jlast, could this be fallout from bug 900763?

In any case, this should be fixed asap. Marking as p1.
Flags: needinfo?(jlaster)
Priority: -- → P1
(Assignee)

Comment 2

3 years ago
Looking into it now ejbruel. It seems likely that bug 900763 caused it.
Flags: needinfo?(jlaster)
(Assignee)

Comment 3

3 years ago
Actually ejbruel, magicp, it looks like the regression was masked by bug 1256704, which was breaking conditional breakpoints entirely. I reverted the patch from 900763 and the regression was still there. 

I'm going to continue to look into the bug and see if I can get to the bottom of it.
(Assignee)

Comment 4

3 years ago
I believe the issue is the popup's opening/closing animation. Basically, we open the second popup immediately, and when the first popup is done animating away we close both of them. 

One way to see this happening is to set a breakpoint here: 
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/content/views/sources-view.js#L687
and evaluating _cbPanel.getAttribute('animation')

This patch fixes it https://gist.github.com/jasonLaster/2768b58da831460e7f57, but uses a hard coded value. I'm going to see if I can observe the animation attribute changing, which avoid the hard coded value.
(Assignee)

Comment 6

3 years ago
Created attachment 8735440 [details] [diff] [review]
cpopup.1.patch

Problem: the conditional breakpoint panel is shared between breakpoints. When you try to see a breakpoint expression and the panel is already open it breaks. (see attachment 1 [details] [diff] [review]). It breaks because the panel is animating out while attempting to show the new expression at the same time.

The fix is to wait on opening the panel until the panel has been hidden. 

Note, I'm using the `_cbPanel.hidden` property to check if the panel is open or closed. Previously, the hidden property did not have a default. Also, it was set to hidden immediately. This patch fixes those things so the property is reliable.
Attachment #8735440 - Flags: review?(jlong)
Comment on attachment 8735440 [details] [diff] [review]
cpopup.1.patch

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

Looks good! I only looked at the general idea and I assume that the flow works as it should.

::: devtools/client/debugger/content/views/sources-view.js
@@ +696,5 @@
> +      cbPanel.removeEventListener('popuphidden', openPopup, false);
> +    }
> +
> +    // Wait until the other cb panel is closed
> +    if (!this._cbPanel.hidden) {

You assigned `this._cbPanel` to a variable above so you could use that here also
Attachment #8735440 - Flags: review?(jlong) → review+
You should assign yourself to bugs that you work on :)
(Assignee)

Updated

3 years ago
Assignee: nobody → jlaster
(Assignee)

Comment 9

3 years ago
> You assigned `this._cbPanel` to a variable above so you could use that here also

Yep, that's correct `this._cbPanel.hidden` was not reliable the way it was setup before.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
has problems to apply:

patching file devtools/client/debugger/content/views/sources-view.js
Hunk #1 FAILED at 59
Hunk #3 FAILED at 144
2 out of 5 hunks FAILED -- saving rejects to file devtools/client/debugger/content/views/sources-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh cpopup.1.patch
Flags: needinfo?(jlaster)
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
Created attachment 8736326 [details] [diff] [review]
cpopup.2.patch
Attachment #8733520 - Attachment is obsolete: true
Attachment #8735440 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/646cc46e3cfc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.