Last Comment Bug 717916 - Add an Inspect button and a node menu to the infobar
: Add an Inspect button and a node menu to the infobar
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: x86 All
: P1 normal (vote)
: Firefox 14
Assigned To: Paul Rouget [:paul]
:
: Patrick Brosset <:pbro>
Mentors:
: 703092 717919 (view as bug list)
Depends on: 584407 694958 717923 730596
Blocks: 703092 724507 733612
  Show dependency treegraph
 
Reported: 2012-01-13 06:49 PST by Paul Rouget [:paul]
Modified: 2012-04-13 12:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mockup (51.45 KB, image/png)
2012-01-17 14:54 PST, Paul Rouget [:paul]
no flags Details
patch v1 (19.02 KB, patch)
2012-02-06 04:32 PST, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review
patch 1.1 (19.19 KB, patch)
2012-02-25 11:18 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch 1.2 (22.88 KB, patch)
2012-03-02 07:48 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch 1.3 (22.22 KB, patch)
2012-03-02 08:03 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.4 (22.21 KB, patch)
2012-03-07 08:34 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.5 (22.26 KB, patch)
2012-03-08 12:05 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.6 (23.22 KB, patch)
2012-03-12 09:53 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.7 (25.38 KB, patch)
2012-03-13 06:23 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.8 (24.37 KB, patch)
2012-03-16 05:06 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
layout cleanup (2.22 KB, patch)
2012-04-04 03:59 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v1.9 (25.37 KB, patch)
2012-04-04 09:44 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v1.10 (25.19 KB, patch)
2012-04-04 11:41 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-01-13 06:49:00 PST
The button should popup only once the node is locked.
Comment 1 Paul Rouget [:paul] 2012-01-17 14:54:40 PST
Created attachment 589311 [details]
mockup
Comment 2 Paul Rouget [:paul] 2012-02-06 04:32:05 PST
Created attachment 594676 [details] [diff] [review]
patch v1
Comment 3 Paul Rouget [:paul] 2012-02-06 10:31:04 PST
*** Bug 717919 has been marked as a duplicate of this bug. ***
Comment 4 Dave Camp (:dcamp) 2012-02-06 16:01:09 PST
Comment on attachment 594676 [details] [diff] [review]
patch v1

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

r+, but would like a second opinion on css changes please.
Comment 5 Paul Rouget [:paul] 2012-02-25 10:55:15 PST
*** Bug 703092 has been marked as a duplicate of this bug. ***
Comment 6 Paul Rouget [:paul] 2012-02-25 11:18:33 PST
Created attachment 600696 [details] [diff] [review]
patch 1.1

added the menu introduced in bug 584407
Comment 7 Dão Gottwald [:dao] 2012-02-27 07:40:36 PST
Comment on attachment 600696 [details] [diff] [review]
patch 1.1

patch doesn't apply cleanly
Comment 8 Paul Rouget [:paul] 2012-03-02 07:48:11 PST
Created attachment 602365 [details] [diff] [review]
patch 1.2

rebased
Comment 9 Paul Rouget [:paul] 2012-03-02 07:49:55 PST
patch queue: 717923 -> 717922 -> 717916
Comment 10 Paul Rouget [:paul] 2012-03-02 08:03:34 PST
Created attachment 602371 [details] [diff] [review]
patch 1.3
Comment 11 Paul Rouget [:paul] 2012-03-07 08:34:06 PST
Created attachment 603740 [details] [diff] [review]
patch v1.4
Comment 12 Paul Rouget [:paul] 2012-03-08 12:05:20 PST
Created attachment 604150 [details] [diff] [review]
patch v1.5

missed pseudo-class style
Comment 13 Paul Rouget [:paul] 2012-03-08 12:11:54 PST
Comment on attachment 604150 [details] [diff] [review]
patch v1.5

This patch is important as it blocks the pseudo-class lock feature.
Comment 14 Paul Rouget [:paul] 2012-03-12 03:09:24 PDT
review ping.

(We would really like to get this in Firefox 13)
Comment 15 Paul Rouget [:paul] 2012-03-12 09:53:38 PDT
Created attachment 604989 [details] [diff] [review]
patch v1.6

the button and the menu are reachable with the tab key
Comment 16 Dão Gottwald [:dao] 2012-03-13 03:55:18 PDT
Comment on attachment 604989 [details] [diff] [review]
patch v1.6

>+    inspect.setAttribute("tooltiptext", "&inspectButton.tooltiptext;");

This doesn't work.

>+    inspect.setAttribute("tabindex", "0");

This doesn't really seem to work either. When I hit Tab, focus moves in the page. It seems like the devtools UI should steal focus, and maybe prevent content from re-gaining focus?

In this particular case though the button doesn't need to be focusable, since it has a shortcut key (Enter).

>+    let nodemenu = this.chromeDoc.createElement("toolbarbutton");
>+    nodemenu.setAttribute("type", "menu");
>+    nodemenu.id = "highlighter-nodeinfobar-menu";
>+    nodemenu.className = "highlighter-nodeinfobar-button"
>+    nodemenu.setAttribute("tabindex", "0");

Can this button also get a tooltip?

Also, I find these two buttons on the sides of the bubble rather weird. Can this get ui-review?
Comment 17 Paul Rouget [:paul] 2012-03-13 04:12:06 PDT
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 604989 [details] [diff] [review]
> patch v1.6
> 
> >+    inspect.setAttribute("tooltiptext", "&inspectButton.tooltiptext;");
> 
> This doesn't work.

I'll fix that (I will copy the tooltiptext from the toolbar inspect button).

> >+    inspect.setAttribute("tabindex", "0");
> 
> This doesn't really seem to work either. When I hit Tab, focus moves in the
> page. It seems like the devtools UI should steal focus, and maybe prevent
> content from re-gaining focus?

For now, we want the content to keep the focus.
But when the Inspector UI get the focus, it works (click on the sidebar for example, and then click shift-tab, then press the space bar to activate the buttons).

> In this particular case though the button doesn't need to be focusable,
> since it has a shortcut key (Enter).
> 
> >+    let nodemenu = this.chromeDoc.createElement("toolbarbutton");
> >+    nodemenu.setAttribute("type", "menu");
> >+    nodemenu.id = "highlighter-nodeinfobar-menu";
> >+    nodemenu.className = "highlighter-nodeinfobar-button"
> >+    nodemenu.setAttribute("tabindex", "0");
> 
> Can this button also get a tooltip?

Ok.

> Also, I find these two buttons on the sides of the bubble rather weird. Can
> this get ui-review?

Not sure I can get the flag in time, but this has been discussed with Shorlander.
He also has tested a build.
Comment 18 Paul Rouget [:paul] 2012-03-13 06:23:03 PDT
Created attachment 605366 [details] [diff] [review]
patch v1.7
Comment 19 Paul Rouget [:paul] 2012-03-13 06:35:52 PDT
Comment on attachment 605366 [details] [diff] [review]
patch v1.7

Fixed the tooltiptext.
I don't think the focus behavior should block this bug.

I'll re-think the overall focus behavior in a follow-up bug.
Comment 20 Paul Rouget [:paul] 2012-03-13 07:27:48 PDT
See bug 735214
Comment 21 Paul Rouget [:paul] 2012-03-13 09:30:29 PDT
Does the node menu needs to be easily reachable with the tab key? Since we keep the focus in the content, we have to press tabs a lot of times before reaching the menu.
Comment 22 Paul Rouget [:paul] 2012-03-14 09:14:42 PDT
triage, filter on centaur
Comment 23 Paul Rouget [:paul] 2012-03-16 04:54:00 PDT
I won't make the button and the menu reachable with tab.

The inspector button is accessible from the toolbar, and the node menu should be accessible as a context menu in the breadcrumbs (bug 724600).
Comment 24 Paul Rouget [:paul] 2012-03-16 05:06:33 PDT
Created attachment 606536 [details] [diff] [review]
patch v1.8
Comment 25 Paul Rouget [:paul] 2012-03-16 05:08:36 PDT
Comment on attachment 606536 [details] [diff] [review]
patch v1.8

see comment 23
Comment 26 Paul Rouget [:paul] 2012-03-29 09:44:39 PDT
review ping?
Comment 27 Dave Camp (:dcamp) 2012-04-03 08:36:52 PDT
Dao, can you take a look at this soon please?  This bug is holding up some important work.
Comment 28 Dão Gottwald [:dao] 2012-04-04 03:59:59 PDT
Created attachment 612143 [details] [diff] [review]
layout cleanup

This is the result of me trying to clean up your CSS, since it seemed pretty messy and fragile to me.
Comment 29 Paul Rouget [:paul] 2012-04-04 07:33:56 PDT
(In reply to Dão Gottwald [:dao] from comment #28)
> Created attachment 612143 [details] [diff] [review]
> layout cleanup
> 
> This is the result of me trying to clean up your CSS, since it seemed pretty
> messy and fragile to me.

Thank you for the clean-up.

Beside this update (I will add your code to the patch), do you see anything else to improve/fix?
Comment 30 Dão Gottwald [:dao] 2012-04-04 08:09:16 PDT
Why is the whole bubble forced to be LTR?
Comment 31 Paul Rouget [:paul] 2012-04-04 08:20:48 PDT
(In reply to Dão Gottwald [:dao] from comment #30)
> Why is the whole bubble forced to be LTR?

The original reason was to get the text LTR. I guess this should not include the buttons.
Comment 32 Paul Rouget [:paul] 2012-04-04 08:27:01 PDT
I will remove `direction: ltr` from #highlighter-nodeinfobar and move it to #highlighter-nodeinfobar-text.
Comment 33 Paul Rouget [:paul] 2012-04-04 09:44:04 PDT
Created attachment 612233 [details] [diff] [review]
patch v1.9

Dao's clean-up + LTR
Comment 34 Dão Gottwald [:dao] 2012-04-04 11:07:24 PDT
Comment on attachment 612233 [details] [diff] [review]
patch v1.9

>+.highlighter-nodeinfobar-button {
>+  -moz-appearance: none;
>+  border-style: solid;
>+  border-color: hsla(210,8%,5%,.45);

border: 0 solid hsla(210,8%,5%,.45);

>+#highlighter-nodeinfobar-inspectbutton {
>+  border-width: 0;

>+#highlighter-nodeinfobar-menu {
>+  border-width: 0;

remove border-width: 0; here

>+#highlighter-nodeinfobar-menu > .toolbarbutton-menu-dropmarker {
>+  -moz-appearance: none!important;

insert space before !
Comment 35 Paul Rouget [:paul] 2012-04-04 11:41:13 PDT
Created attachment 612275 [details] [diff] [review]
patch v1.10

Thank you Dao.
Comment 36 Rob Campbell [:rc] (:robcee) 2012-04-12 12:16:19 PDT
is the "layout cleanup" patch part of what needs to land here?
Comment 37 Dão Gottwald [:dao] 2012-04-13 04:04:06 PDT
Comment on attachment 612143 [details] [diff] [review]
layout cleanup

it's part of the final patch
Comment 38 Rob Campbell [:rc] (:robcee) 2012-04-13 09:04:24 PDT
https://hg.mozilla.org/integration/fx-team/rev/71e852f66edb
Comment 39 Rob Campbell [:rc] (:robcee) 2012-04-13 12:18:04 PDT
https://hg.mozilla.org/mozilla-central/rev/71e852f66edb

Note You need to log in before you can comment on or make changes to this bug.