717916 - Add an Inspect button and a node menu to the infobar

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Paul Rouget [:paul]]

The button should popup only once the node is locked.

Comment 1

[Paul Rouget [:paul]]

Attachment: mockup

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 4

[Dave Camp (:dcamp)]

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 6

[Paul Rouget [:paul]]

Attachment: patch 1.1

added the menu introduced in bug 584407

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 600696 [details] [diff] [review]
patch 1.1

patch doesn't apply cleanly

Comment 8

[Paul Rouget [:paul]]

Attachment: patch 1.2

rebased

Comment 9

[Paul Rouget [:paul]]

patch queue: 717923 -> 717922 -> 717916

Comment 10

[Paul Rouget [:paul]]

Attachment: patch 1.3

Comment 11

[Paul Rouget [:paul]]

Attachment: patch v1.4

Comment 12

[Paul Rouget [:paul]]

Attachment: patch v1.5

missed pseudo-class style

Comment 13

[Paul Rouget [:paul]]

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]]

review ping.

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

Comment 15

[Paul Rouget [:paul]]

Attachment: patch v1.6

the button and the menu are reachable with the tab key

Comment 16

[Dão Gottwald [:dao]]

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]]

(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]]

Attachment: patch v1.7

Comment 19

[Paul Rouget [:paul]]

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]]

See bug 735214

Comment 21

[Paul Rouget [:paul]]

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]]

triage, filter on centaur

Comment 23

[Paul Rouget [:paul]]

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]]

Attachment: patch v1.8

Comment 25

[Paul Rouget [:paul]]

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

see comment 23

Comment 26

[Paul Rouget [:paul]]

review ping?

Comment 27

[Dave Camp (:dcamp)]

Dao, can you take a look at this soon please?  This bug is holding up some important work.

Comment 28

[Dão Gottwald [:dao]]

Attachment: 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]]

(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]]

Why is the whole bubble forced to be LTR?

Comment 31

[Paul Rouget [:paul]]

(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]]

I will remove `direction: ltr` from #highlighter-nodeinfobar and move it to #highlighter-nodeinfobar-text.

Comment 33

[Paul Rouget [:paul]]

Attachment: patch v1.9

Dao's clean-up + LTR

Comment 34

[Dão Gottwald [: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 !

Comment 35

[Paul Rouget [:paul]]

Attachment: patch v1.10

Thank you Dao.

Comment 36

[Rob Campbell [:rc] (:robcee)]

is the "layout cleanup" patch part of what needs to land here?

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 612143 [details] [diff] [review]
layout cleanup

it's part of the final patch

Comment 38

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/71e852f66edb

Comment 39

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/71e852f66edb