Closed Bug 1111599 Opened 9 years ago Closed 7 years ago

Shift-click elements while in picker mode should select the element but keep the picker mode active

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: pbro, Assigned: xfq, Mentored)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [good first bug])

Attachments

(2 files, 2 obsolete files)

The idea comes from this tweet: https://twitter.com/shannmcnicholl/status/544475934393647104

Here's the use case:

While in "picker mode" (i.e. the user has clicked on the picker icon in the toolbox), any hovered element is shown in the markup-view, but the element isn't really selected, the breadcrumbs doesn't update, nor does the styles in the sidebar.
It would be useful to be able to select an element but remain in picker mode so that you could select another element right after.

Using shift-click could be used for this.

This would let users shift-click their way to the element they care about, being able to check the style as they go.

This wouldn't be very discoverable, but I don't think it needs to be. It's similar to the alt+click in the markup-view which expands the whole sub-tree of a node.
This should be enough to make it work.
This patch doesn't contain any new tests though.
And I'm the middle of something else at the moment, so I'm going to leave this bug unassigned for anyone who wants to do it. If not, I'll try to come back to this in a few days/weeks.
In case someone else wants to do this and needs help, I can mentor.
Mentor: pbrosset
Hey Patrick,
I am willing to work on this bug. Assign me some work to start with.
(In reply to kunal jain from comment #3)
> Hey Patrick,
> I am willing to work on this bug. Assign me some work to start with.
Cool, thanks for wanting to do this.
The very first step is to follow the dev environment and build steps here: https://wiki.mozilla.org/DevTools/Hacking

Then, as you can see in comment 1, I actually already started to write some code for this. Once your environment is ready, you should be able to download that patch, apply it locally, build, and start from there.

I suggest you do that and look at the code changes I made, let me know if you understand them, if they make sense to you. Since this would be your first patch, there's quite a bit of new concepts you'll need to understand, so please, don't hesitate to ask questions on anything you don't understand.

Once you feel comfortable with the code, the next step will be to make sure it functions as we expect it to, in all cases (let's try and make sure we test all possible scenarios).
And after that, your next task will be to write some new automated tests to ensure that new feature doesn't break unexpectedly in the future when that code is touched again.

I'll add a comment about this later when you've gone through the first steps.
Assignee: nobody → kkunal95jjain
Status: NEW → ASSIGNED
Are you still working on this, Kunal?
Flags: needinfo?(kkunal95jjain)
yess josh i am working on it. since i am new to this its taking a little longer time for me understand things.
Flags: needinfo?(kkunal95jjain)
No activity for a year, removing assignment.
This bug already has a patch attached and a mentor, marking as good first bug!

See https://developer.mozilla.org/en-US/docs/Tools/Contributing to get started.

Inspector bug triage. Filter on CLIMBING SHOES.
Assignee: kkunal95jjain → nobody
Severity: normal → enhancement
Status: ASSIGNED → NEW
Priority: -- → P3
Whiteboard: [good first bug]
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Hi Patrick,

I'm interested in this bug. I've applied your patch (with some minor changes) and tested it (manually, using the inspector for web pages and the inspector in the browser toolbox). Will submit it via MozReview later.

In general the result looked good to me, but Sometimes™ when I click a link (when the picker is active), Firefox opens the link in a new window instead of previews that element. I don't know how to debug this.

BTW, I don't understand what the code in devtools/shared/spec/ is for (devtools/shared/spec/inspector.js in this case), would you please explain it to me?

One last question: how to write automated tests for this feature? I'm not familiar with Mochitest/xpcshell.
(In reply to Xue Fuqiao [:xfq] from comment #9)
> In general the result looked good to me, but Sometimes™ when I click a link

click -> shift+click
(In reply to Xue Fuqiao [:xfq] from comment #9)
> Hi Patrick,
> 
> I'm interested in this bug. I've applied your patch (with some minor
> changes) and tested it (manually, using the inspector for web pages and the
> inspector in the browser toolbox). Will submit it via MozReview later.
> 
> In general the result looked good to me, but Sometimes™ when I click a link
> (when the picker is active), Firefox opens the link in a new window instead
> of previews that element. I don't know how to debug this.
Hm, that's weird, does this happen on any page, or only on, say, about:home (which I know has been causing problems in the past).
Normally, events on the content page should not happen while the picker mode is ON (see _preventContentEvent in devtools/server/actors/highlighters.js).

> BTW, I don't understand what the code in devtools/shared/spec/ is for
> (devtools/shared/spec/inspector.js in this case), would you please explain
> it to me?
These "specs" are filed that define the devtools protocol. Devtools is made of 2 parts: client (the toolbox UI) and server (the code that actually interacts with the page being inspected), and they communicate through a RDP protocol that allows the client to call methods on the server. These methods are separated in various actors, and all the files you see in the spec directory just define these actors and methods, and are used to auto-implement Front classes (the client-side classes we use to call remote methods).

> One last question: how to write automated tests for this feature? I'm not
> familiar with Mochitest/xpcshell.
Here's a bit of doc:
- how to run them https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
- how to write one: https://wiki.mozilla.org/DevTools/mochitests_coding_standards
I think you could add a new devtools mochitest for this feature. One that looks like
devtools\client\inspector\test\browser_inspector_highlighter-N.js
There is a lot of code in this test directory that you can probably reuse. Please take a look and see if you can find what you need for your test, and let me know if you need more help.
Comment on attachment 8799101 [details]
Bug 1111599 - Shift-click while in picker mode selects the element but doesn't stop the picker;

This looks good. I haven't tested it locally yet, but the code changes look good.
Ping me when you have a new test, or if you need more help, and I'll spend more time on it.
Attachment #8799101 - Flags: review?(pbrosset) → feedback+
Attachment #8536568 - Attachment is obsolete: true
Assignee: nobody → xfq.free
Status: NEW → ASSIGNED
(In reply to Patrick Brosset <:pbro> from comment #12)
> (In reply to Xue Fuqiao [:xfq] from comment #9)
> > In general the result looked good to me, but Sometimes™ when I click a link
> > (when the picker is active), Firefox opens the link in a new window instead
> > of previews that element. I don't know how to debug this.
> Hm, that's weird, does this happen on any page, or only on, say, about:home
> (which I know has been causing problems in the past).
> Normally, events on the content page should not happen while the picker mode
> is ON (see _preventContentEvent in devtools/server/actors/highlighters.js).

Since it's a heisenbug, I spent a lot of time reproducing it. Finally I found that it usually happens when I open DevTools (with cmd+option+i or F12) for the first time in a Firefox session, and before the main content (the HTML pane) of the inspector panel finishes loading.

> > BTW, I don't understand what the code in devtools/shared/spec/ is for
> > (devtools/shared/spec/inspector.js in this case), would you please explain
> > it to me?
> These "specs" are filed that define the devtools protocol. Devtools is made
> of 2 parts: client (the toolbox UI) and server (the code that actually
> interacts with the page being inspected), and they communicate through a RDP
> protocol that allows the client to call methods on the server. These methods
> are separated in various actors, and all the files you see in the spec
> directory just define these actors and methods, and are used to
> auto-implement Front classes (the client-side classes we use to call remote
> methods).

If I understand correctly, the "specs" define how the debugger (client) interacts with the server (the browser), i.e. specify how data should be packetized/transmitted/received, and how the client and server exchanges the JSON packets, right?

> > One last question: how to write automated tests for this feature? I'm not
> > familiar with Mochitest/xpcshell.
> Here's a bit of doc:
> - how to run them
> https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
> - how to write one:
> https://wiki.mozilla.org/DevTools/mochitests_coding_standards
> I think you could add a new devtools mochitest for this feature. One that
> looks like
> devtools\client\inspector\test\browser_inspector_highlighter-N.js
> There is a lot of code in this test directory that you can probably reuse.
> Please take a look and see if you can find what you need for your test, and
> let me know if you need more help.

I just ran the DevTools Mochitests with:

   ./mach mochitest --subsuite devtools --tag devtools

Unexpectedly (to me), it took 3865s[1] to run the tests on my machine. IIUC I should focus on the browser window when the tests are running, so I can't do anything else. What are ways to speed up the test-running?

I'll have a look at devtools\client\inspector\test and try to write some new automated tests for this feature later.

[1] For comparison, a './mach build' without using artifact mode only takes ~45min (~2700s).
Flags: needinfo?(pbrosset)
(In reply to Xue Fuqiao [:xfq] from comment #14)
> If I understand correctly, the "specs" define how the debugger (client)
> interacts with the server (the browser), i.e. specify how data should be
> packetized/transmitted/received, and how the client and server exchanges the
> JSON packets, right?
Yes.

> I just ran the DevTools Mochitests with:
> 
>    ./mach mochitest --subsuite devtools --tag devtools
> 
> Unexpectedly (to me), it took 3865s[1] to run the tests on my machine. IIUC
> I should focus on the browser window when the tests are running, so I can't
> do anything else. What are ways to speed up the test-running?
> 
> I'll have a look at devtools\client\inspector\test and try to write some new
> automated tests for this feature later.
> 
> [1] For comparison, a './mach build' without using artifact mode only takes
> ~45min (~2700s).
Yeah, running the whole test suite take a long time, and you indeed can't use your computer because it requires focus (unless you have a VM to run them in, or on Linux).
You most likely don't need to run the whole test suite though. The changes you are making here are most probably not going to unexpectedly break things in other panels.

To run a single test:
./mach mochitest devtools/client/inspector/test/browser_inspector_highlighter-01.js

To run a whole directory:
./mach mochitest devtools/client/inspector/test/

If you're changing things in the inspector, then running the whole inspector/test folder is a good idea. If you're unsure what your code changes are going to affect, you can also run:

./mach mochitest devtools/client/inspector/

which will run the tests in inspector/test but also the ones in inspector/markup/test, inspector/rules/test, ...
If they all pass, there is very little changes that you'd have broken anything else.

In any case, we have a CI environment that builds and runs all the tests on various platforms before your commit lands. This takes a bit of time, but at least doesn't use your computer. So when you're fairly certain that your changes are safe, you can push the patch to TRY and see if everything looks ok there: https://wiki.mozilla.org/ReleaseEngineering/TryServer

I can push to TRY for you if you don't have access to it.
Flags: needinfo?(pbrosset)
(In reply to Xue Fuqiao [:xfq] from comment #14)
> > > In general the result looked good to me, but Sometimes™ when I click a link
> > > (when the picker is active), Firefox opens the link in a new window instead
> > > of previews that element. I don't know how to debug this.
> > Hm, that's weird, does this happen on any page, or only on, say, about:home
> > (which I know has been causing problems in the past).
> > Normally, events on the content page should not happen while the picker mode
> > is ON (see _preventContentEvent in devtools/server/actors/highlighters.js).
> 
> Since it's a heisenbug, I spent a lot of time reproducing it. Finally I
> found that it usually happens when I open DevTools (with cmd+option+i or
> F12) for the first time in a Firefox session, and before the main content
> (the HTML pane) of the inspector panel finishes loading.

I forget to say that it happens on any page (including about:home).
(In reply to Xue Fuqiao [:xfq] from comment #16)
> (In reply to Xue Fuqiao [:xfq] from comment #14)
> > > > In general the result looked good to me, but Sometimes™ when I click a link
> > > > (when the picker is active), Firefox opens the link in a new window instead
> > > > of previews that element. I don't know how to debug this.
> > > Hm, that's weird, does this happen on any page, or only on, say, about:home
> > > (which I know has been causing problems in the past).
> > > Normally, events on the content page should not happen while the picker mode
> > > is ON (see _preventContentEvent in devtools/server/actors/highlighters.js).
> > 
> > Since it's a heisenbug, I spent a lot of time reproducing it. Finally I
> > found that it usually happens when I open DevTools (with cmd+option+i or
> > F12) for the first time in a Firefox session, and before the main content
> > (the HTML pane) of the inspector panel finishes loading.
Not sure I get this part. Opening with cmd+option+i doesn'tt start the picker mode. So I'm wondering how you manage to enter the picker mode before the HTML inspector panel finishes loading.
In any case, I'd suggest not worrying about this particular bug here, and instead file another bug so we can investigate. It doesn't seem related to shift+click, right? I assume this happens even when you click on links while things are still loading.
(In reply to Patrick Brosset <:pbro> from comment #17)
> > > > > In general the result looked good to me, but Sometimes™ when I click a link
> > > > > (when the picker is active), Firefox opens the link in a new window instead
> > > > > of previews that element. I don't know how to debug this.
> > > > Hm, that's weird, does this happen on any page, or only on, say, about:home
> > > > (which I know has been causing problems in the past).
> > > > Normally, events on the content page should not happen while the picker mode
> > > > is ON (see _preventContentEvent in devtools/server/actors/highlighters.js).
> > > 
> > > Since it's a heisenbug, I spent a lot of time reproducing it. Finally I
> > > found that it usually happens when I open DevTools (with cmd+option+i or
> > > F12) for the first time in a Firefox session, and before the main content
> > > (the HTML pane) of the inspector panel finishes loading.
> Not sure I get this part. Opening with cmd+option+i doesn'tt start the
> picker mode. So I'm wondering how you manage to enter the picker mode before
> the HTML inspector panel finishes loading.

Although cmd+option+i doesn't start the picker mode, I can click the little "Pick an element from the page" button before the HTML pane finishes loading. 

> In any case, I'd suggest not worrying about this particular bug here, and
> instead file another bug so we can investigate. It doesn't seem related to
> shift+click, right? I assume this happens even when you click on links while
> things are still loading.

Yes, you're right. Thanks.
I have a problem when writing the Mochitests.

Please see the attachment for what I've already written. I don't know how to write the "preview a node" (shift-click a node) part. Can you point me in the right direction?

BTW, I can't use the try server now. Maybe I'll apply to become a Mozilla committer in the future, if needed.
Flags: needinfo?(pbrosset)
I recommend applying for level 1 access. I will vouch for such a request.
(In reply to Xue Fuqiao [:xfq] from comment #19)
> BTW, I can't use the try server now. Maybe I'll apply to become a Mozilla
> committer in the future, if needed.
Sure, as Josh said, you can apply easily if you want to. Otherwise I can push to TRY for you later when the patch is ready.
I'll take a look at that test now.
Flags: needinfo?(pbrosset)
How about this test? Let me know if there's anything you want me to explain.
Sorry I ended up writing the whole test rather than giving you pointers, but mochitests aren't the most fun part and they are hard to write (they rely on a lot of hard-to-find helper functions that you just need to know). Hopefully this is OK.
Attachment #8799780 - Attachment is obsolete: true
(In reply to Patrick Brosset <:pbro> from comment #22)
> Created attachment 8800243 [details]
> browser_inspector_highlighter_preview.js
> 
> How about this test? Let me know if there's anything you want me to explain.
> Sorry I ended up writing the whole test rather than giving you pointers, but
> mochitests aren't the most fun part and they are hard to write (they rely on
> a lot of hard-to-find helper functions that you just need to know).
> Hopefully this is OK.

It looks good. Thank you.

I can understand most of the code. For the parts I don't understand, I'll do some research later.

I'll apply for level 1 access.
(In reply to Josh Matthews [:jdm] from comment #20)
> I recommend applying for level 1 access. I will vouch for such a request.

OK. I've filed a bug: Bug 1309757

Thank you!
Flags: needinfo?(josh)
Clear the ni? since Patrick has vouched for me.
Flags: needinfo?(josh)
Comment on attachment 8799101 [details]
Bug 1111599 - Shift-click while in picker mode selects the element but doesn't stop the picker;

https://reviewboard.mozilla.org/r/84398/#review84464

Perfect! This looks great, thanks.
So now that you have level 1 commit acces, you can push this to TRY to make sure this change didn't break any other test that we might have missed.
To do this, you should take a quick look at https://wiki.mozilla.org/ReleaseEngineering/TryServer#Scheduling_jobs_with_Try_Syntax
This is the syntax I normally use: try: -b do -p linux64,macosx64,win32,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-dt-e10s,mochitest-o,mochitest-e10s-dt,mochitest-clipboard,mochitest-clipboard-e10s -t none
Now there's another, simpler way too, you can now trigger a TRY push right from mozreview. There's an automation menu you can use to push to TRY, and you can paste a custom try syntax in there too.
I'll let you decide what you want to use.

In any case, once the TRY build completes and once we've verified that there are no problems, then we can just proceed and land the change (which, btw, can also be done right here on mozreview, by requesting a push to autoland).
Attachment #8799101 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset <:pbro> from comment #27)
> Comment on attachment 8799101 [details]
> Bug 1111599 - Shift-click while in picker mode selects the element but
> doesn't stop the picker;
> 
> https://reviewboard.mozilla.org/r/84398/#review84464
> 
> Perfect! This looks great, thanks.

Thanks for your help and review!

> So now that you have level 1 commit acces, you can push this to TRY to make
> sure this change didn't break any other test that we might have missed.
> To do this, you should take a quick look at
> https://wiki.mozilla.org/ReleaseEngineering/
> TryServer#Scheduling_jobs_with_Try_Syntax
> This is the syntax I normally use: try: -b do -p
> linux64,macosx64,win32,win64,eslint-gecko -u
> xpcshell,mochitest-dt,mochitest-dt-e10s,mochitest-o,mochitest-e10s-dt,
> mochitest-clipboard,mochitest-clipboard-e10s -t none

The command failed:

$ ./mach try -b do -p linux64,macosx64,win32,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-dt-e10s,mochitest-o,mochitest-e10s-dt,mochitest-clipboard,mochitest-clipboard-e10s -t none
mach try is under development, please file bugs blocking 1149670.
Creating temporary commit for remote...
pushing to ssh://hg.mozilla.org/try
remote: Warning: Permanently added the ED25519 host key for IP address '63.245.215.102' to the list of known hosts.
remote: Permission denied (publickey).
temporary commit removed, repository restored
abort: no suitable response from remote hg!
ERROR hg command ['hg', 'push-to-try', '-m', 'try: -b do -p win32,win64,linux64,macosx64,eslint-gecko -u mochitest-clipboard,mochitest-clipboard-e10s,mochitest-dt,mochitest-dt-e10s,mochitest-e10s-dt,mochitest-o,xpcshell -t none'] returned 255

I checked my SSH keypair but didn't find any problem.

> Now there's another, simpler way too, you can now trigger a TRY push right
> from mozreview. There's an automation menu you can use to push to TRY, and
> you can paste a custom try syntax in there too.

The "trigger a try build" button and the "land commits" button are grey (they can't be pressed).
Flags: needinfo?(pbrosset)
Hi Vinh, Xue's commit level 1 access was added in bug 1309849, but apparently there's a problem, see comment 28.
Are you able to help Xue?
Flags: needinfo?(pbrosset) → needinfo?(vhua)
I'm not familiar with the specifics of how hg works.  When users report seeing "remote: Permission denied (publickey)" usually regenerating and adding their keys work.
Flags: needinfo?(vhua)
(In reply to Vinh Hua [:vinh] from comment #30)
> I'm not familiar with the specifics of how hg works.  When users report
> seeing "remote: Permission denied (publickey)" usually regenerating and
> adding their keys work.

Thanks for your reply.

Is there any other way? My SSH key is used on many other sites (like GitHub and Savannah, and the key works without any problem). I have to change all other sites if I regenerate the keypair.
In that case regenerating would not be ideal.  Something must be off in your hg configuration, i.e. username?

http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/auth.html
(In reply to Vinh Hua [:vinh] from comment #32)
> In that case regenerating would not be ideal.  Something must be off in your
> hg configuration, i.e. username?
> 
> http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/auth.
> html

Thank you very much. I forgot to configure the SSH authentication of hg and MozReview. I confirm that All Goes Well now.
(In reply to Patrick Brosset <:pbro> from comment #27)
> In any case, once the TRY build completes and once we've verified that there
> are no problems, then we can just proceed and land the change (which, btw,
> can also be done right here on mozreview, by requesting a push to autoland).

Patrick, here are the results of my tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2743b81043e0
Flags: needinfo?(pbrosset)
Perfect, thanks for working this out!
Looks like the TRY build is all green except for a minor eslint error that you should fix before we land the change. On the TRY URL, you can see that one job is orange, the ES job, it runs ESLint on the whole code and reports any errors.
Here's the error it reported:
devtools/client/inspector/test/browser_inspector_highlighter-preview.js:56:2 | Newline required at end of file but not found. (eol-last)

More info about eslint here: https://wiki.mozilla.org/DevTools/CodingStandards

Could you please append a newline at the end of this file and then just push one last time to mozreview. We'll then land the change!

Thanks a lot for your help.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #35)
> Could you please append a newline at the end of this file and then just push
> one last time to mozreview. We'll then land the change!

Done.

Thanks for the patient help!
Would you please land the change, Patrick? IIUC Autoland requires L3 SCM access.
Flags: needinfo?(pbrosset)
(In reply to Xue Fuqiao [:xfq] from comment #38)
> Would you please land the change, Patrick? IIUC Autoland requires L3 SCM
> access.
I have already asked autoland to land your commit. Unfortunately it errored out: "We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again", so I'll keep the needinfo flag as a reminder for me to land this manually instead.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e4282badddda
Shift-click while in picker mode selects the element but doesn't stop the picker; r=pbro
https://hg.mozilla.org/mozilla-central/rev/e4282badddda
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Clearing the needinfo flag.
Flags: needinfo?(pbrosset)
> It was documented in the Tips[1] page. 

Thanks for that!

I've added a note here:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Select_an_element#With_the_node_picker

and here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts#Node_picker

Marking this as dev-doc-complete, but let me know if we need anything else.
(In reply to Will Bamberg [:wbamberg] from comment #44)
> > It was documented in the Tips[1] page. 
> 
> Thanks for that!
> 
> I've added a note here:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Select_an_element#With_the_node_picker
> 
> and here:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/
> Keyboard_shortcuts#Node_picker
> 
> Marking this as dev-doc-complete, but let me know if we need anything else.

Cool. Thanks for documenting this feature! I tweaked the docs on these two pages a little bit.
Verified fixed on Fx 52.0b7 Ubuntu 16.04
Shift clicking allows you to select elements and remain in the picker mode.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.