UI for XHR Breakpoints #6934

AnshulMalik · 2018-09-05T20:35:46Z

Fixes Issue: #5255

Summary of Changes

First version of xhr, we can say "pause if the url contains a given string"

We can leave the box empty if we want to pause for any request.

Screenshots/Videos (OPTIONAL)

flodolo · 2018-09-06T05:01:39Z

assets/panel/debugger.properties

@@ -497,6 +497,10 @@ expressions.errorMsg=Invalid expression…
 expressions.label=Add watch expression
 expressions.accesskey=e

+xhrBreakpoints.header=XHR Breakpoints
+xhrBreakpoints.placeholder=Break when url contains


I think URL should be uppercase

flodolo · 2018-09-06T05:01:52Z

assets/panel/debugger.properties

@@ -497,6 +497,10 @@ expressions.errorMsg=Invalid expression…
 expressions.label=Add watch expression
 expressions.accesskey=e

+xhrBreakpoints.header=XHR Breakpoints
+xhrBreakpoints.placeholder=Break when url contains
+xhrBreakpoints.label=Add xhr breakpoint


XHR was uppercase in the previous string

nchevobbe · 2018-09-06T06:00:34Z

That's great @AnshulMalik !
I find that being able to break on URL matching is also super interesting (we could then even have some context menu entries/buttons in the netmonitor to allow adding XHR breakpoints from there).

The only thing that puzzles me a bit is the "leave input empty to break on all XHR".
It could be better, because here, we have a list of matching input, so you could have 1 empty input (match all) and another one for a specific URL, which is a bit weird.
My gut feeling is that we should make break on all XHR the default, and then allow for some matching.

▼ XHR breakpoints
⚪️ Break on all XHR
🔘 Break when URL matches    ➕
       facebook.com
       foo/bar

or something better, but that a clear distinction between breaking on everything and allowing for URL matching.

AnshulMalik · 2018-09-06T08:24:48Z

@nchevobbe Yeah, that sounds more practical.

Since the designs were not yet ready, so I went ahead with what chrome does.

darkwing · 2018-09-06T22:21:48Z

When @AnshulMalik casually submits a PR for an incredibly awesome feature:

I'll take an in-depth look at this tomorrow. Obviously this is awesome but wasn't well spec'd out by us so there will likely be lots of feedback. In the mean time...this is amazing! Thank you! 💯

flodolo

String-wise it looks good, thanks for updating it.

Letting code review to the actual devs ;-)

darkwing · 2018-09-22T13:04:01Z

A few behavioral things I'm seeing:

The "+" button in the XHR breakpoints header doesn't do anything.
Toggling the header open focuses on the input; not sure we need that.
The "x" (close) button for items needs to be shifted a few pixels to the right/end

These are all minor things; I think if you get time to clean this up a bit (i.e. removing commented code), we can try to land something. Great work so far @AnshulMalik !

AnshulMalik · 2018-09-25T18:44:37Z

Hey @darkwing, thanks for the comments, here my my thoughts:

It's the same as watch expressions, add one xor bp, and then click in the editor, so now if you want to add another xhr bp, you need to click "+" to get the input box.
This is strange, since toggling the header does toggle hide/show the panel.
Working on it :)

darkwing · 2018-09-27T15:55:11Z

Adding in an older mockup from @violasong:

Obviously we dont' need to do all of this at once. It might be nice in this first PR if we can add the GET/POST dropdown for adding the breakpoint though.

digitarald · 2018-10-01T04:03:24Z

Just to clarify: The mockup should not inform the UI or requirements for this first implementation. Goal is a simple UI, similar to existing XHR breakpoint implementations (aka Chrome). We will iterate on the UI in the future, @mcroud is working taking over the design work.

Just to re-iterate the basic requirements:

allow pausing on any resource
allow pausing on responses matching a URL pattern

Do they make sense?

darkwing · 2018-10-02T14:48:40Z

Per @digitarald's comment, the only thing missing is a "pause on all XHRs"; maybe in the short term we can simply add a checkbox like we do for "Pause on Exceptions". Up for it @AnshulMalik ? This is sooooooo close!

digitarald · 2018-10-02T17:58:50Z

the only thing missing is a "pause on all XHRs"; maybe in the short term we can simply add a checkbox like we do for "Pause on Exceptions".

Chrome offers a checkbox for this, so maybe. The alternative is would be an XHR breakpoint without a URL filter (which Chrome has before I think).

How are empty XHR breakpoints treated in the current solution?

AnshulMalik · 2018-10-02T18:22:02Z

Okay, we can have a checkbox for pause on all xhr,
@digitarald currently, if you hit enter on empty string, it will work as pause on all xhr breakpoints

digitarald · 2018-10-02T18:38:06Z

@digitarald currently, if you hit enter on empty string, it will work as pause on all xhr breakpoints

Looks like the latest behavior on Chrome removes empty XHR breakpoints.

darkwing · 2018-10-09T15:27:57Z

assets/panel/debugger.properties

@@ -513,6 +513,14 @@ expressions.label=Add watch expression
 expressions.accesskey=e
 expressions.key=CmdOrCtrl+Shift+E

+xhrBreakpoints.header=XHR Breakpoints


We need a note here.

flodolo · 2018-10-10T16:24:12Z

assets/panel/debugger.properties

 # LOCALIZATION NOTE (expressions.errorMsg): Error text for expression
 # input element
 expressions.errorMsg=Invalid expression…
 expressions.label=Add watch expression
 expressions.accesskey=e
 expressions.key=CmdOrCtrl+Shift+E

+# LOCALIZATION NOTE (xhrBreakpoints): The pause on any xhr breakpoints headings


The string ID referenced in the comment should be xhrBreakpoints.header, also XHR (uppercase) in the note itself for consistency.

darkwing · 2018-10-10T16:41:42Z

I've rebased and fixed a header issue. @AnshulMalik Would you be able to make a quick mochitest or two for this functionality? i.e. add an xhr breakpoint and ensure it pauses when hit?

AnshulMalik · 2018-10-11T06:30:00Z

Created a PR here, to allow the functionality to work firefox-devtools/devtools-core#1098

darkwing · 2018-10-11T15:53:09Z

Current to do:

Tests
Localization changnes
Update package.json's version for devtools-core

darkwing · 2018-10-18T23:43:53Z

src/test/mochitest/browser_dbg-xhr-breakpoints.js

+    const dbg = await initDebugger("doc-xhr.html");
+    await waitForSources(dbg, "fetch.js");
+    await dbg.actions.setXHRBreakpoint("doc", "GET");
+    debugger;


Is this supposed to be here? Shouldn't the pause happen from setting the breakpoint?

Yep, it should not be here

darkwing · 2018-10-19T19:54:14Z

OK, cool, manually tested and this is totally in the right direction. Yay!

A few things:

When I add a create an XHR breakpoint (for example "fetch"), then click "Pause on any", then click "Pause on any" again, the "fetch" XHR breakpoint disappears. Ideally this wouldn't have any effect on XHR breakpoints added by the user; it would just disable them.
We need to change the label to "Pause on any URL"
How difficult would it be to persist XHR breakpoint strings? i.e. store them in prefs so that when we refresh the page, the breakpoint is still there?
How difficult would it be to add a green highlight to the currently-broken-on XHR breakpoint? This can definitely come late but worth asking.

AnshulMalik · 2018-10-19T22:19:16Z

1-3 are present in this commit.

This one is interesting, it won't be hard, but would be a nice to have.

DPX-designer · 2018-10-22T08:30:40Z

Here are the final mockups for light and dark mode complete with the agreed "pause" terminology.

Light mode:
https://mozilla.invisionapp.com/share/VTOMJAC7EH5#/326071897_XHR-Breakpoints_2

Dark mode:
https://mozilla.invisionapp.com/share/AWOP2TNUJ2V#/326848311_XHR-Breakpoints_2_Dark

AnshulMalik requested a review from flodolo as a code owner September 5, 2018 20:35

flodolo reviewed Sep 6, 2018

View reviewed changes

darkwing changed the title ~~[WIP] UI for XHR Breakpoints~~ UI for XHR Breakpoints Sep 6, 2018

darkwing added the 📚 pr-wip label Sep 6, 2018

flodolo approved these changes Sep 7, 2018

View reviewed changes

AnshulMalik force-pushed the xhr branch from f54ab7f to 72bfa24 Compare September 13, 2018 07:10

AnshulMalik force-pushed the xhr branch from 7e3a6b6 to 10ba497 Compare September 22, 2018 08:49

AnshulMalik force-pushed the xhr branch from 10ba497 to 400d333 Compare September 25, 2018 18:39

AnshulMalik force-pushed the xhr branch from 14d5bfb to 0f7e340 Compare October 9, 2018 03:31

darkwing reviewed Oct 9, 2018

View reviewed changes

darkwing force-pushed the xhr branch from 0f7e340 to 4720629 Compare October 10, 2018 16:18

flodolo reviewed Oct 10, 2018

View reviewed changes

AnshulMalik force-pushed the xhr branch 2 times, most recently from ebb82cf to 7bc0a4d Compare October 18, 2018 15:28

AnshulMalik added 2 commits October 19, 2018 00:02

UI for XHR Breakpoints

c7b6a9a

allow enable and disable breakpoint

9dbf524

AnshulMalik and others added 11 commits October 19, 2018 00:02

uppercase url and xhr

c6b2e72

discard module-manifest changes

f72010e

fix yarn

85e3885

shift close button

c2e701b

move to array, add checkbox for Pause on Any

02ed188

remove yarn.lock

a3d8304

Add heading to l10n file

14413c3

Fix header text

17b3a1b

wire backend

7a656de

add mochitest

17ee84f

change mc commit number

6f2355b

AnshulMalik force-pushed the xhr branch 4 times, most recently from c34f8a4 to c730fff Compare October 18, 2018 21:53

Correct MC version, backport mochi changes

3ffbd69

AnshulMalik force-pushed the xhr branch from c730fff to 3ffbd69 Compare October 18, 2018 22:20

darkwing reviewed Oct 18, 2018

View reviewed changes

remove debugger statement

1243a15

fix checkbox bug, persisy breakpoints

fc47779

AnshulMalik force-pushed the xhr branch from 0ae82b4 to fc47779 Compare October 19, 2018 22:13

test for checkbox

210523b

darkwing merged commit 9fe51cc into firefox-devtools:master Oct 20, 2018

darkwing pushed a commit that referenced this pull request Oct 23, 2018

UI for XHR Breakpoints (#6934)

8ba7272

egdbear pushed a commit to egdbear/debugger.html that referenced this pull request Oct 23, 2018

UI for XHR Breakpoints (firefox-devtools#6934)

3a99b62

AnshulMalik deleted the xhr branch December 30, 2018 21:16

Archaeopteryx mentioned this pull request Jan 1, 2019

Tooltip to add XHR breakpoint should use correct l10n string #7626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI for XHR Breakpoints #6934

UI for XHR Breakpoints #6934

AnshulMalik commented Sep 5, 2018

flodolo Sep 6, 2018

flodolo Sep 6, 2018

nchevobbe commented Sep 6, 2018

AnshulMalik commented Sep 6, 2018

darkwing commented Sep 6, 2018

flodolo left a comment

darkwing commented Sep 22, 2018

AnshulMalik commented Sep 25, 2018

darkwing commented Sep 27, 2018

digitarald commented Oct 1, 2018

darkwing commented Oct 2, 2018

digitarald commented Oct 2, 2018

AnshulMalik commented Oct 2, 2018

digitarald commented Oct 2, 2018

darkwing Oct 9, 2018

flodolo Oct 10, 2018

darkwing commented Oct 10, 2018

AnshulMalik commented Oct 11, 2018

darkwing commented Oct 11, 2018 •

edited by AnshulMalik

Loading

darkwing Oct 18, 2018

AnshulMalik Oct 19, 2018

darkwing commented Oct 19, 2018

AnshulMalik commented Oct 19, 2018

DPX-designer commented Oct 22, 2018

UI for XHR Breakpoints #6934

UI for XHR Breakpoints #6934

Conversation

AnshulMalik commented Sep 5, 2018

Summary of Changes

Screenshots/Videos (OPTIONAL)

flodolo Sep 6, 2018

Choose a reason for hiding this comment

flodolo Sep 6, 2018

Choose a reason for hiding this comment

nchevobbe commented Sep 6, 2018

AnshulMalik commented Sep 6, 2018

darkwing commented Sep 6, 2018

flodolo left a comment

Choose a reason for hiding this comment

darkwing commented Sep 22, 2018

AnshulMalik commented Sep 25, 2018

darkwing commented Sep 27, 2018

digitarald commented Oct 1, 2018

darkwing commented Oct 2, 2018

digitarald commented Oct 2, 2018

AnshulMalik commented Oct 2, 2018

digitarald commented Oct 2, 2018

darkwing Oct 9, 2018

Choose a reason for hiding this comment

flodolo Oct 10, 2018

Choose a reason for hiding this comment

darkwing commented Oct 10, 2018

AnshulMalik commented Oct 11, 2018

darkwing commented Oct 11, 2018 • edited by AnshulMalik Loading

darkwing Oct 18, 2018

Choose a reason for hiding this comment

AnshulMalik Oct 19, 2018

Choose a reason for hiding this comment

darkwing commented Oct 19, 2018

AnshulMalik commented Oct 19, 2018

DPX-designer commented Oct 22, 2018

darkwing commented Oct 11, 2018 •

edited by AnshulMalik

Loading