Closed Bug 1495944 Opened 6 years ago Closed 6 years ago

about:devtools-toolbox can open nested inspecter for debugging about:devtools-toolbox

Categories

(DevTools :: about:debugging, defect, P1)

Product:
DevTools
Component:
about:debugging
Version:
64 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

(firefox66 verified, firefox67 verified, firefox68 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox66 --- verified
firefox67 --- verified
firefox68 --- verified

People

(Reporter: m_kato, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

(Whiteboard: old-remote-debugging-ng-m3)

Attachments

(7 files)

screenshot
109.85 KB, image/png
Details
Bug 1495944: Enable/Disable devtools menu items and the shortcut key. r=jdescottes!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1495944: Show/Hide 'Inspect Element' item in content context menu. r=jdescottes!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1495944: Add a test whether the menu items that open inspector was hidden. r=jdescottes!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1495944: Add a test for context menu in about:devtools-toolbox. r=jdescottes!
47 bytes, text/x-phabricator-request
Details | Review
about-pages-listed-in-tabs-category.png
69.24 KB, image/png
Details
Bug 1495944: Add a test for shortcut keys on about:devtools-toolbox page. r?jdescottes
47 bytes, text/x-phabricator-request
Details | Review
Attached image screenshot — 
Step
1. Launch Nightly/Android with remote debugging via USB connection
2. Open about:debugging
3. Selection 1's target, then [Connect] and [Inspect] any tab.

Then about:devtools-tools is opened.

4. [developer tool] - [Inspector] from hamburger menu

Result
New inspector panel is opened for debugging about:devtools-toolbox (see screenshot)

Expected
Disable this menu, or select [Inspector] panel instead of opening new panel.
filter on remote-debugging-next-move-m3-to-m2
Blocks: remote-debugging-ng-m2
filter on remote-debugging-next-move-m3-to-m2
filter on remote-debugging-next-move-m3-to-m2
No longer blocks: remote-debugging-ng-m3
Priority: -- → P2
Whiteboard: old-remote-debugging-ng-m3
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: P2 → P1

Hi Matt,

To resolve this, I have an approach like below as a first prototype.

When open about:devtools-toolbox (Click Inspect button of tab in about:debugging),

  1. Hide menu items of devtools such as Toggle Tools from Tools -> Web Developer menu.
  2. Disable short cut key of above menus
  3. Hide Inspect Element menu item from content context menu

Or, should we do another approach? At least, displaying two inspectors in one page makes us to confuse.
What do you think?

Flags: needinfo?(mcroud)

An advice from Julian on slack,
If we choose my approach, it is better this feature can turn on/off by pref.
I agree because "about:devtools-toolbar" page developing will become hard a bit.

Interesting! I have just replicated the issue and it's extremely confusing.

Is there a purpose for being able to Inspect the DevTools Inspector while remote debugging?
I agree with your suggestion of hiding the Inspect Element tools because we don't want users to inspect the Inspector, but what would be the purpose of giving people the option of turning it back on, is there a use for it I am missing?

Thank you very much Matt!
Julian asked to devtools team about this, it seems that nobody had any objections to disable the opening devtools on about:devtools-toolbox page.
Thus, I think we can ignore the pref option.
So, I'll proceed with the approach in this time.

If you have opinions, please let me know!
Thanks again.

Yes, preventing this "double Inspector" sounds like a good idea.
If no one can nominate a good reason why a user might want to Inspect the Inspector while remote debugging then we should hide the option to do so, in case a user does it by accident and gets completely lost :) (I would!)

Flags: needinfo?(mcroud)

Is there a purpose for being able to Inspect the DevTools Inspector while remote debugging?

The main added value is for us developing about:devtools-toolbox. That's useful to inspect the markup of the header we recently added for instance, fine tune the CSS etc... Basically be able to do regular development. We have a workaround, which is to use another toolbox (the Browser Toolbox) but it's not as well integrated nor easy to use.

Initially about:devtools-toolbox was also praised as an easy way to inspect the markup of DevTools themselves, but as Daisuke said, nobody in the team is using them.

Matt, two additional questions for you :)

  • for the buttons / menu-items impacted here, should they be hidden or disabled?
  • should the regular shortcuts that toggle a specific tool (cmd shift c for inspector) focus the relevant tool in about:devtools-toolbox? (I guess this will be a yes but still asking)

see question above

Flags: needinfo?(mcroud)
This patch removes "Inspect Element" menu item from content context menu in
case that about:devtools-toolbox are opening.
However, we may be able to remove after fixing 1515265.

Depends on D16684

Ah so yes there is absolutely a purpose for a developer, but perhaps not for a user who is currently remote debugging.

I don't think there would be much value in showing users disabled options which we don't want them to click, they may feel that they have/haven't done something in order to gain access to those options.

Yes to the second question, that makes a lot of sense to me.

For a user who sees DevTools as a unique app used to debug a webpage (me :P), it is perhaps jarring to find that my right click menus in about:devtools-toolbox treat it as an ordinary webpage, with options to View Page Source and Save Page As.. etc.

So to cater equally for both the developer and remote debugger folk, an option to reveal these options makes sense to me.

Flags: needinfo?(mcroud)

Related to this, maybe we should also filter some tabs from our tabs list. Right now we list about:debugging and about:devtools-toolbox tabs, which users most likely don't want to inspect!

Probably follow-up material.

Risk for users would be hitting shortcuts and getting lost in devtools multi-verse.

There is a benefit to debug devtools without the browser toolbox; and we might eventually need to debug remote-debugging specific devtools issues. Would it be reasonable to keep the option for users with Browser chrome debugging on?

(In reply to :Harald Kirschner :digitarald from comment #18)

Risk for users would be hitting shortcuts and getting lost in devtools multi-verse.

And as we know, we have many shortcuts that can open devtools :)

There is a benefit to debug devtools without the browser toolbox; and we might eventually need to debug remote-debugging specific devtools issues. Would it be reasonable to keep the option for users with Browser chrome debugging on?

I would also like to keep the option open, but I suggest to handle this in a follow up since we didn't get any feedback when we asked the team if anybody would miss the feature.

Attachment #9036890 - Attachment description: Bug 1495944: Add a test whether the menu items and shortcut key that open inspector was hidden. r=jdescottes! → Bug 1495944: Add a test whether the menu items that open inspector was hidden. r=jdescottes!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85f355a22192
Enable/Disable devtools menu items and the shortcut key. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f78876cf49e6
Show/Hide 'Inspect Element' item in content context menu. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7862a0a1c3ec
Add a test whether the menu items that open inspector was hidden. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/cd627a79a250
Add a test for context menu in about:devtools-toolbox. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f365b9fa9697
Add a test for shortcut keys on about:devtools-toolbox page. r=jdescottes
See Also: → 1520984

Backed out 5 changesets (Bug 1495944) for browser_989751_subviewbutton_class.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=browser-chrome&revision=f365b9fa9697d6700adf1ea5db66f541001a78f8&selectedJob=222617197

Backout link: https://hg.mozilla.org/integration/autoland/rev/fd6b8be34aed51a18d91261abeb586bbbf350e50

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222617197&repo=autoland&lineNumber=2211

17:39:26 INFO - TEST-START | browser/components/customizableui/test/browser_989751_subviewbutton_class.js
17:39:27 INFO - TEST-INFO | started process screencapture
17:39:27 INFO - TEST-INFO | screencapture: exit 0
17:39:27 INFO - Buffered messages logged at 17:39:26
17:39:27 INFO - Entering test bound
17:39:27 INFO - Checking for items without the subviewbutton class in developer-button widget
17:39:27 INFO - Waiting for overflow button to have non-0 size
17:39:27 INFO - Buffered messages logged at 17:39:27
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Subview should have a kid -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Toggle Tools' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - Buffered messages finished
17:39:27 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Toggle Tools' (classes: subviewbutton) should still have its own class, too. -
17:39:27 INFO - Stack trace:
17:39:27 INFO - chrome://mochikit/content/browser-test.js:test_ok:1307
17:39:27 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_989751_subviewbutton_class.js:checkSubviewButtonClass/<:46
17:39:27 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1108
17:39:27 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1099
17:39:27 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:997
17:39:27 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Inspector' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Web Console' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Debugger' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Style Editor' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Performance' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Network' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Storage Inspector' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Accessibility' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'WebIDE' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Browser Console' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Responsive Design Mode' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Eyedropper' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Scratchpad' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Service Workers' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Page Source' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Get More Tools' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - TEST-PASS | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Item 'Work Offline' (classes: subviewbutton) should have the subviewbutton class. -
17:39:27 INFO - Leaving test bound
17:39:27 INFO - GECKO(908) | MEMORY STAT | vsize 4554MB | residentFast 528MB | heapAllocated 214MB
17:39:27 INFO - TEST-OK | browser/components/customizableui/test/browser_989751_subviewbutton_class.js | took 571ms

Flags: needinfo?(dakatsuka)

Thanks Bogdan,

I hope this patch fix the failure..
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7b8e4257e26c9937f779d6d7095d0cce1836bb7

Flags: needinfo?(dakatsuka)
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9418ac4c2fa1
Enable/Disable devtools menu items and the shortcut key. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/be64b571b600
Show/Hide 'Inspect Element' item in content context menu. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/df656c9d03b1
Add a test whether the menu items that open inspector was hidden. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/025ffd6f441f
Add a test for context menu in about:devtools-toolbox. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/52e5f0d01900
Add a test for shortcut keys on about:devtools-toolbox page. r=jdescottes

Backed out 5 changesets (bug 1495944) for DevTools failures in devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_devtoolstoolbox_menubar.js

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222713220&repo=autoland&lineNumber=2852

INFO - GECKO(797) | FATAL ERROR: AsyncShutdown timeout in ShutdownLeaks: Wait for cleanup to be finished before checking for leaks Conditions: [{"name":"DevTools: Wait until toolbox is destroyed","state":"(none)","filename":"resource://devtools/client/framework/toolbox.js","lineNumber":3001,"stack":["resource://devtools/client/framework/toolbox.js:leakCheckObserver:3001","chrome://mochikit/content/browser-test.js:nextTest/<:962","chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803"]}] At least one completion condition failed to complete within a reasonable amount of time. Causing a crash to ensure that we do not leave the user with an unresponsive process draining resources.
05:48:18 INFO - GECKO(797) | [Parent 797, Main Thread] ###!!! ABORT: file resource://devtools/client/framework/toolbox.js, line 3001
05:48:18 INFO - GECKO(797) | [Parent 797, Main Thread] ###!!! ABORT: file resource://devtools/client/framework/toolbox.js, line 3001
05:48:18 INFO - GECKO(797) | Hit MOZ_CRASH() at /builds/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - GECKO(797) | Exiting due to channel error.
05:48:18 INFO - TEST-INFO | Main app process: exit 1
05:48:18 ERROR - TEST-UNEXPECTED-FAIL | ShutdownLeaks | process() called before end of test suite
05:48:18 INFO - Buffered messages finished
05:48:18 ERROR - TEST-UNEXPECTED-FAIL | Last test finished | application terminated with exit code 1
05:48:18 INFO - runtests.py | Application ran for: 0:06:03.780085
05:48:18 INFO - zombiecheck | Reading PID log: /var/folders/rh/dfnhqtfj7qb0wy8z1ph3hwxw00000x/T/tmpRhTYWzpidlog

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=52e5f0d019001b4c9a3ca168d3624fe11dc9e7f5

Backout:
https://hg.mozilla.org/integration/autoland/rev/f93fc53741db6086041008a87b3213af336e3cc1

Flags: needinfo?(dakatsuka)
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a93d114f9bc9
Enable/Disable devtools menu items and the shortcut key. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/4674df9d14e3
Show/Hide 'Inspect Element' item in content context menu. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1f135369b9f9
Add a test whether the menu items that open inspector was hidden. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/bffac1c44562
Add a test for context menu in about:devtools-toolbox. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e3d901befdc1
Add a test for shortcut keys on about:devtools-toolbox page. r=jdescottes

Verified on Nightly 68.0a1, Beta 67.0b6, release 66.0.2 -on several machines: Windows 10, MAC OS X, Ubuntu 16.04

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: --- → verified
status-firefox68: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: