Closed Bug 717916 Opened 9 years ago Closed 9 years ago

Add an Inspect button and a node menu to the infobar

Categories

(DevTools :: Inspector, defect, P1)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 files, 11 obsolete files)

The button should popup only once the node is locked.
Assignee: nobody → paul
Depends on: 694958
Summary: Add an Inpsect button to the infobar → Add an Inspect button to the infobar
Attached image mockup
Blocks: 703092
Blocks: 724507
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #594676 - Flags: review?(dcamp)
Summary: Add an Inspect button to the infobar → Add an Inspect button and a node menu to the infobar
Duplicate of this bug: 717919
Depends on: 584407
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.
Attachment #594676 - Flags: review?(dcamp) → review+
Duplicate of this bug: 703092
Depends on: 730596
Attached patch patch 1.1 (obsolete) — Splinter Review
added the menu introduced in bug 584407
Attachment #594676 - Attachment is obsolete: true
Attachment #600696 - Flags: review?(dao)
Comment on attachment 600696 [details] [diff] [review]
patch 1.1

patch doesn't apply cleanly
Attachment #600696 - Flags: review?(dao)
Attached patch patch 1.2 (obsolete) — Splinter Review
rebased
Attachment #600696 - Attachment is obsolete: true
Depends on: 717922
patch queue: 717923 -> 717922 -> 717916
Attached patch patch 1.3 (obsolete) — Splinter Review
Attachment #602365 - Attachment is obsolete: true
Blocks: 733612
Attached patch patch v1.4 (obsolete) — Splinter Review
Attachment #602371 - Attachment is obsolete: true
Depends on: 717923
No longer depends on: 717922
Attachment #603740 - Flags: review?(dao)
Attachment #603740 - Attachment is patch: true
Attached patch patch v1.5 (obsolete) — Splinter Review
missed pseudo-class style
Attachment #603740 - Attachment is obsolete: true
Attachment #603740 - Flags: review?(dao)
Comment on attachment 604150 [details] [diff] [review]
patch v1.5

This patch is important as it blocks the pseudo-class lock feature.
Attachment #604150 - Flags: review?(dao)
review ping.

(We would really like to get this in Firefox 13)
Attached patch patch v1.6 (obsolete) — Splinter Review
the button and the menu are reachable with the tab key
Attachment #604150 - Attachment is obsolete: true
Attachment #604150 - Flags: review?(dao)
Attachment #604989 - Flags: review?(dao)
Status: NEW → ASSIGNED
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?
Attachment #604989 - Flags: review?(dao) → review-
(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.
Attached patch patch v1.7 (obsolete) — Splinter Review
Attachment #604989 - Attachment is obsolete: true
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.
Attachment #605366 - Flags: review?(dao)
See bug 735214
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.
triage, filter on centaur
Priority: -- → P1
Whiteboard: [firefox14-wanted]
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).
Attachment #605366 - Attachment is obsolete: true
Attachment #605366 - Flags: review?(dao)
Attached patch patch v1.8 (obsolete) — Splinter Review
Comment on attachment 606536 [details] [diff] [review]
patch v1.8

see comment 23
Attachment #606536 - Flags: review?(dao)
review ping?
Dao, can you take a look at this soon please?  This bug is holding up some important work.
Attached patch layout cleanup (obsolete) — Splinter Review
This is the result of me trying to clean up your CSS, since it seemed pretty messy and fragile to me.
Attachment #606536 - Flags: review?(dao) → review-
(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?
Why is the whole bubble forced to be LTR?
(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.
I will remove `direction: ltr` from #highlighter-nodeinfobar and move it to #highlighter-nodeinfobar-text.
Attached patch patch v1.9 (obsolete) — Splinter Review
Dao's clean-up + LTR
Attachment #606536 - Attachment is obsolete: true
Attachment #612233 - Flags: review?(dao)
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 !
Attachment #612233 - Flags: review?(dao) → review+
Attached patch patch v1.10Splinter Review
Thank you Dao.
Attachment #612233 - Attachment is obsolete: true
Whiteboard: [firefox14-wanted] → [land-in-fx-team]
is the "layout cleanup" patch part of what needs to land here?
Comment on attachment 612143 [details] [diff] [review]
layout cleanup

it's part of the final patch
Attachment #612143 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/71e852f66edb
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/71e852f66edb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.