Add an Inspect button and a node menu to the infobar

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Inspector
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 14
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

5 years ago
The button should popup only once the node is locked.
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Updated

5 years ago
Depends on: 694958

Updated

5 years ago
Summary: Add an Inpsect button to the infobar → Add an Inspect button to the infobar
(Assignee)

Comment 1

5 years ago
Created attachment 589311 [details]
mockup
(Assignee)

Updated

5 years ago
Blocks: 703092
(Assignee)

Updated

5 years ago
Blocks: 724507
(Assignee)

Comment 2

5 years ago
Created attachment 594676 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #594676 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Summary: Add an Inspect button to the infobar → Add an Inspect button and a node menu to the infobar
(Assignee)

Updated

5 years ago
Duplicate of this bug: 717919
(Assignee)

Updated

5 years ago
Depends on: 584407

Comment 4

5 years ago
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+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 703092
(Assignee)

Updated

5 years ago
Depends on: 730596
(Assignee)

Comment 6

5 years ago
Created attachment 600696 [details] [diff] [review]
patch 1.1

added the menu introduced in bug 584407
(Assignee)

Updated

5 years ago
Attachment #594676 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 8

5 years ago
Created attachment 602365 [details] [diff] [review]
patch 1.2

rebased
(Assignee)

Updated

5 years ago
Attachment #600696 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 717922
(Assignee)

Comment 9

5 years ago
patch queue: 717923 -> 717922 -> 717916
(Assignee)

Comment 10

5 years ago
Created attachment 602371 [details] [diff] [review]
patch 1.3
(Assignee)

Updated

5 years ago
Attachment #602365 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 733612
(Assignee)

Comment 11

5 years ago
Created attachment 603740 [details] [diff] [review]
patch v1.4
Attachment #602371 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 717923
No longer depends on: 717922
(Assignee)

Updated

5 years ago
Attachment #603740 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #603740 - Attachment is patch: true
(Assignee)

Comment 12

5 years ago
Created attachment 604150 [details] [diff] [review]
patch v1.5

missed pseudo-class style
(Assignee)

Updated

5 years ago
Attachment #603740 - Attachment is obsolete: true
Attachment #603740 - Flags: review?(dao)
(Assignee)

Comment 13

5 years ago
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)
(Assignee)

Comment 14

5 years ago
review ping.

(We would really like to get this in Firefox 13)
(Assignee)

Comment 15

5 years ago
Created attachment 604989 [details] [diff] [review]
patch v1.6

the button and the menu are reachable with the tab key
(Assignee)

Updated

5 years ago
Attachment #604150 - Attachment is obsolete: true
Attachment #604150 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #604989 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 605366 [details] [diff] [review]
patch v1.7
(Assignee)

Updated

5 years ago
Attachment #604989 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
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)
(Assignee)

Comment 20

5 years ago
See bug 735214
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
triage, filter on centaur
Priority: -- → P1
Whiteboard: [firefox14-wanted]
(Assignee)

Comment 23

5 years ago
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).
(Assignee)

Updated

5 years ago
Attachment #605366 - Attachment is obsolete: true
Attachment #605366 - Flags: review?(dao)
(Assignee)

Comment 24

5 years ago
Created attachment 606536 [details] [diff] [review]
patch v1.8
(Assignee)

Comment 25

5 years ago
Comment on attachment 606536 [details] [diff] [review]
patch v1.8

see comment 23
Attachment #606536 - Flags: review?(dao)
(Assignee)

Comment 26

5 years ago
review ping?

Comment 27

5 years ago
Dao, can you take a look at this soon please?  This bug is holding up some important work.
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.

Updated

5 years ago
Attachment #606536 - Flags: review?(dao) → review-
(Assignee)

Comment 29

5 years ago
(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?
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 32

5 years ago
I will remove `direction: ltr` from #highlighter-nodeinfobar and move it to #highlighter-nodeinfobar-text.
(Assignee)

Comment 33

5 years ago
Created attachment 612233 [details] [diff] [review]
patch v1.9

Dao's clean-up + LTR
(Assignee)

Updated

5 years ago
Attachment #606536 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 35

5 years ago
Created attachment 612275 [details] [diff] [review]
patch v1.10

Thank you Dao.
(Assignee)

Updated

5 years ago
Attachment #612233 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.