New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI for XHR Breakpoints #6934

Merged
merged 17 commits into from Oct 20, 2018

Conversation

Projects
None yet
6 participants
@AnshulMalik
Copy link
Member

AnshulMalik commented Sep 5, 2018

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)

ezgif com-optimize-2

@AnshulMalik AnshulMalik requested a review from flodolo as a code owner Sep 5, 2018

@@ -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

This comment has been minimized.

@flodolo

flodolo Sep 6, 2018

I think URL should be uppercase

@@ -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

This comment has been minimized.

@flodolo

flodolo Sep 6, 2018

XHR was uppercase in the previous string

@nchevobbe

This comment has been minimized.

Copy link
Member

nchevobbe commented Sep 6, 2018

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

This comment has been minimized.

Copy link
Member Author

AnshulMalik commented Sep 6, 2018

@nchevobbe Yeah, that sounds more practical.

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

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Sep 6, 2018

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

giroud-slide

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! 💯

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

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

@flodolo

flodolo approved these changes Sep 7, 2018

Copy link

flodolo left a comment

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

Letting code review to the actual devs ;-)

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch from f54ab7f to 72bfa24 Sep 13, 2018

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch from 7e3a6b6 to 10ba497 Sep 22, 2018

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Sep 22, 2018

A few behavioral things I'm seeing:

  1. The "+" button in the XHR breakpoints header doesn't do anything.
  2. Toggling the header open focuses on the input; not sure we need that.
  3. 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 AnshulMalik force-pushed the AnshulMalik:xhr branch from 10ba497 to 400d333 Sep 25, 2018

@AnshulMalik

This comment has been minimized.

Copy link
Member Author

AnshulMalik commented Sep 25, 2018

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

  1. 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.

  2. This is strange, since toggling the header does toggle hide/show the panel.

  3. Working on it :)

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Sep 27, 2018

Adding in an older mockup from @violasong:

xhrbreakpoints

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

This comment has been minimized.

Copy link
Member

digitarald commented Oct 1, 2018

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:

  1. allow pausing on any resource
  2. allow pausing on responses matching a URL pattern

Do they make sense?

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Oct 2, 2018

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

This comment has been minimized.

Copy link
Member

digitarald commented Oct 2, 2018

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

This comment has been minimized.

Copy link
Member Author

AnshulMalik commented Oct 2, 2018

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

This comment has been minimized.

Copy link
Member

digitarald commented Oct 2, 2018

@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.

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch from 14d5bfb to 0f7e340 Oct 9, 2018

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

xhrBreakpoints.header=XHR Breakpoints

This comment has been minimized.

@darkwing

darkwing Oct 9, 2018

Contributor

We need a note here.

@darkwing darkwing force-pushed the AnshulMalik:xhr branch from 0f7e340 to 4720629 Oct 10, 2018

# 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

This comment has been minimized.

@flodolo

flodolo Oct 10, 2018

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

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Member Author

AnshulMalik commented Oct 11, 2018

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

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Oct 11, 2018

Current to do:

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

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch 2 times, most recently from ebb82cf to 7bc0a4d Oct 18, 2018

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch 4 times, most recently from c34f8a4 to c730fff Oct 18, 2018

@AnshulMalik AnshulMalik force-pushed the AnshulMalik:xhr branch from c730fff to 3ffbd69 Oct 18, 2018

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

This comment has been minimized.

@darkwing

darkwing Oct 18, 2018

Contributor

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

This comment has been minimized.

@AnshulMalik

AnshulMalik Oct 19, 2018

Author Member

Yep, it should not be here

@darkwing

This comment has been minimized.

Copy link
Contributor

darkwing commented Oct 19, 2018

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

A few things:

  1. 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.

  2. We need to change the label to "Pause on any URL"

  3. 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?

  4. 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 AnshulMalik force-pushed the AnshulMalik:xhr branch from 0ae82b4 to fc47779 Oct 19, 2018

@AnshulMalik

This comment has been minimized.

Copy link
Member Author

AnshulMalik commented Oct 19, 2018

1-3 are present in this commit.

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

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

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mcroud

This comment has been minimized.

Copy link

mcroud commented Oct 22, 2018

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

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

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

@AnshulMalik AnshulMalik deleted the AnshulMalik:xhr branch Dec 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment