717922 - In the PageInspector toolbar, move the HTML Tree button in front of the breadcrumbs display

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Paul Rouget [:paul]]

... and we should have an icon for that.

Comment 1

[Paul Rouget [:paul]]

Attachment: icon

Comment 2

[Paul Rouget [:paul]]

Attachment: patch 0.007 - WIP

Comment 3

[Paul Rouget [:paul]]

(this patch is very ugly. It's just an experiment)

I have a problem.

The current configuration (after bug 719607) is:

<toolbarbutton id="htmltreepanel"/> <arrowscrollbox id="breadcrumbs"/>


I want to move the HTML button *inside* the breadcrumbs. In front of the buttons, in a fixed position (won't disappear if we scroll the breadcrumbs).

I want to do that because we don't want to have the scrollbutton to show up between the HTMLTreePanel button and the breadcrumbs.

My current approach is to extend chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll (what the breadcrumbs currently use) - add a new <content> where an additional toolbarbutton is added in front of the scrollbox. This makes me duplicate <content>. It is not really easy to style and use this button from outside. But it's possible.

Is that the right approach? Is there a better way to do that?

Comment 4

[Paul Rouget [:paul]]

In attachment 588662 [details] we can see the intended result.

Comment 5

[Dão Gottwald [:dao]]

> I want to do that because we don't want to have the scrollbutton to show up
> between the HTMLTreePanel button and the breadcrumbs.

Why not? Since the html tree button doesn't scroll, it doesn't seem to make sense to have it wrapped by the scroll buttons.

Comment 6

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #5)
> > I want to do that because we don't want to have the scrollbutton to show up
> > between the HTMLTreePanel button and the breadcrumbs.
> 
> Why not? Since the html tree button doesn't scroll, it doesn't seem to make
> sense to have it wrapped by the scroll buttons.

We want to get this kind of design: attachment 588662 [details]
where the HTMLTree button is "merged" with the breadcrumbs.

Comment 7

[Dão Gottwald [:dao]]

Again: Since the html tree button doesn't scroll, it doesn't seem to make sense to have it wrapped by the scroll buttons.

Comment 8

[Paul Rouget [:paul]]

Attachment: mockup

I have been talking to Shorlander about this problem. Here is a different approach. We will always show the scrollbar buttons, deemphasize them when we don't need them.

Not sure about the right button though.

Comment 9

[Paul Rouget [:paul]]

Attachment: better mockup

Comment 10

[Paul Rouget [:paul]]

Attachment: HTMLPanel icon

Comment 11

[Paul Rouget [:paul]]

Attachment: overflow arrows

Comment 12

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 13

[Dave Camp (:dcamp)]

Comment on attachment 594673 [details] [diff] [review]
patch v1

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

This will need at least a toolkit peer to look at the scrollbox.xml change.  Mossop, can you review that change or pass on please?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 594673 [details] [diff] [review]
patch v1

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>       <toolbarbutton id="inspector-treepanel-toolbutton"
>                      class="devtools-toolbarbutton"
>-                     label="&htmlPanel.label;"
>                      accesskey="&htmlPanel.accesskey;"
>                      tooltiptext="&htmlPanel.tooltiptext;"
>                      command="Inspector:HTMLPanel"/>

I don't think it makes sense to have an accesskey if you don't have a label.

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>-<!ENTITY htmlPanel.tooltiptext        "HTML panel">
>+<!ENTITY htmlPanel.tooltiptext        "HTML panel (&htmlPanel.accesskey;)">

You would need to change string name here, but I don't understand why you're putting the accesskey in the tooltiptext.

Comment 15

[Dão Gottwald [:dao]]

See bug 717923 comment 8 and below.

Comment 16

[Dão Gottwald [:dao]]

You don't need to touch scrollbox.xml here, just listen to underflow/overflow events and set a custom attribute.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 594673 [details] [diff] [review]
patch v1

What dao said I think, but if that turns out to be incorrect then ask enn for review.

Comment 18

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #16)
> You don't need to touch scrollbox.xml here, just listen to
> underflow/overflow events and set a custom attribute.

As you can see here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml#523
and here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml#557

… listening to underflow/overflow events is not enough (see the try/catch blocks).
I can copy the try/catch blocks to inspector.jsm, but it seems a little cleaner to add the attribute directly from scrollbox.xml.

I am adding this attribute to know if the buttons are collapsed or not (the style of the HTML breadcrumbs depends on that).

Comment 19

[Paul Rouget [:paul]]

Attachment: patch 1.1

Comment 20

[Paul Rouget [:paul]]

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

I changed the tooltip (Alt+).

If you think the change in scrollbox.xml is justified, I'll ask Enn for a review too.

Comment 21

[Dão Gottwald [:dao]]

You don't need to catch any exception here. See also <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2705>.

Comment 22

[Paul Rouget [:paul]]

Attachment: patch 1.2

So I listen to the overflow/underflow events and then check if the scroll buttons
are collapsed or not. It works.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 600719 [details] [diff] [review]
patch 1.2

see bug 717923 comment 18

Comment 24

[Paul Rouget [:paul]]

Attachment: patch 1.2 (rebased)

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v1.3

no accesskey

Comment 26

[Paul Rouget [:paul]]

(waiting to see how the review goes for bug 717923 before asking Dao to review this patch)

Comment 27

[Paul Rouget [:paul]]

Attachment: patch v1.4

removed the accesskey for real

Comment 28

[Paul Rouget [:paul]]

Dao, what do you think we should do here? In bug 717923 comment 22, Neil suggested that we don't use any accesskey. Do you think we should still append the "(Alt + H)" string to the tooltip?

Comment 29

[Paul Rouget [:paul]]

bug 717923 comment 24 actually.

Comment 30

[Paul Rouget [:paul]]

Attachment: patch v1.5

Comment 31

[Paul Rouget [:paul]]

Attachment: wrong patch

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 603742 [details] [diff] [review]
patch v1.5

needs keyboard access

Comment 33

[Paul Rouget [:paul]]

Just adding tabindex=0 to the toolbarbutton seems to work. Is that ok?

Comment 34

[Paul Rouget [:paul]]

Attachment: patch v1.6

Comment 35

[Paul Rouget [:paul]]

Comment on attachment 604996 [details] [diff] [review]
patch v1.6

Just added tabindex="0" to the button. Hope that's enough.

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 604996 [details] [diff] [review]
patch v1.6

Same problem as in bug 717916, the button is really hard to reach with the Tab key.

Also, it doesn't have a focus indicator.

>       <toolbarbutton id="inspector-treepanel-toolbutton"
>                      class="devtools-toolbarbutton"
>-                     label="&htmlPanel.label;"
>-                     accesskey="&htmlPanel.accesskey;"
>+                     tabindex="0"
>                      tooltiptext="&htmlPanel.tooltiptext;"
>                      command="Inspector:HTMLPanel"/>

> <!ENTITY htmlPanel.tooltiptext        "HTML panel">

Not really this bug's fault, but "HTML panel" is a bad name since it can display any kind of DOM tree (SVG, XUL, whatever).

>--- a/testing/peptest/tests/firefox/server/mozilla.org/index.html.orig
>+++ /dev/null
>@@ -1,8 +0,0 @@
>-<html>
>-<body id="home">
>-<div id="header">
>-Test mozilla home page.
>-</div>
>-</body>
>-</html>

Remove junk

Comment 37

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #36)
> Comment on attachment 604996 [details] [diff] [review]
> patch v1.6
> 
> Same problem as in bug 717916, the button is really hard to reach with the
> Tab key.

Can we fix that in a follow-up bug?

Comment 38

[Dão Gottwald [:dao]]

I don't think we should regress this. Right now the panel can be toggled easily using Alt+H.

Comment 39

[Paul Rouget [:paul]]

If we focus the inspector toolbar once we start inspecting, would that work for you?
There's no visual way to know which button is focused, should we do something about that?

Comment 40

[Paul Rouget [:paul]]

So I can focus the toolbar, but then, the arrows keys are used to navigate the toolbar, not the nodes.

So to summarize the problem:
- we want to use an icon, with no text
- so we can't show the accesskey
- we can't use the tooltip because we don't know what is the name of the "alt" key
- we can't make the button easily reachable with the tab key, because it would imply focusing the toolbar, not the content

Can we just use a hard-coded Alt/Cmd-H shortcut?

Comment 41

[Dão Gottwald [:dao]]

(In reply to Paul Rouget [:paul] from comment #40)
> - we can't use the tooltip because we don't know what is the name of the
> "alt" key

Can you clarify? platformKeys.properties contains VK_ALT which should be right for Windows and Linux. Not sure about Mac.

Comment 42

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #41)
> (In reply to Paul Rouget [:paul] from comment #40)
> > - we can't use the tooltip because we don't know what is the name of the
> > "alt" key
> 
> Can you clarify? platformKeys.properties contains VK_ALT which should be
> right for Windows and Linux. Not sure about Mac.

See bug 717923 comment 22

Comment 43

[Dão Gottwald [:dao]]

Not sure what "On Mac, it is nothing" means. Why would we care about ui.key.menuAccessKey being changed?

Comment 44

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #43)
> Not sure what "On Mac, it is nothing" means.

Afaik, there is not accesskey on Mac.

> Why would we care about ui.key.menuAccessKey being changed?

Because if it changes, its name is not the same anymore. Can we just not take this case into account and just consider its Alt?

Or I can not use an accesskey, and use a hard-coded shortcut: Alt/Cmd-H

Comment 45

[Dão Gottwald [:dao]]

(In reply to Paul Rouget [:paul] from comment #44)
> (In reply to Dão Gottwald [:dao] from comment #43)
> > Not sure what "On Mac, it is nothing" means.
> 
> Afaik, there is not accesskey on Mac.

OS X makes me sad.

> > Why would we care about ui.key.menuAccessKey being changed?
> 
> Because if it changes, its name is not the same anymore. Can we just not
> take this case into account and just consider its Alt?

I don't think we expect users to touch that pref.

> Or I can not use an accesskey, and use a hard-coded shortcut: Alt/Cmd-H

Then it shouldn't be Alt, as shortcut keys use Ctrl, Ctrl+Shift, etc.. Might be a sensible idea if we have enough keys for these kind of combos available, but we probably don't.

Comment 46

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #45)
> (In reply to Paul Rouget [:paul] from comment #44)
> > (In reply to Dão Gottwald [:dao] from comment #43)
> > > Not sure what "On Mac, it is nothing" means.
> > 
> > Afaik, there is not accesskey on Mac.
> 
> OS X makes me sad.
> 
> > > Why would we care about ui.key.menuAccessKey being changed?
> > 
> > Because if it changes, its name is not the same anymore. Can we just not
> > take this case into account and just consider its Alt?
> 
> I don't think we expect users to touch that pref.

So we can build a tooltip dynamically.

We will need two properties:
htmlPanel.tooltip = HTML Panel
htmlPanel.tooltipWithAccesskey = HTML Panel (%S + %S)
We will use htmlPanel.tooltip for Mac, and htmlPanel.tooltipWithAccesskey for Linux and Windows.
We will use the VK_ALT and the accesskey to expand htmlPanel.tooltipWithAccesskey.

Would that work for you?

Comment 47

[Axel Hecht [:Pike]]

CCing one of the a11y guys, as our support for that on osx is changing, I think.

Comment 48

[Paul Rouget [:paul]]

triage, filter on centaur

Comment 49

[Paul Rouget [:paul]]

Attachment: patch v1.7

Comment 50

[Paul Rouget [:paul]]

Attachment: patch v1.7.1

typo

Comment 51

[Paul Rouget [:paul]]

Attachment: patch v1.7.2

Comment 52

[Paul Rouget [:paul]]

Comment on attachment 606609 [details] [diff] [review]
patch v1.7.2

(rebased since bug 736418 is WONTFIX)

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 606609 [details] [diff] [review]
patch v1.7.2

> .devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
>   display: none;
> }
>+
>+.devtools-toolbarbutton-icononly > .toolbarbutton-text {
>+  visibility: collapse;
>+}

display:none (and merge with the previous rule)

>+htmlButton.tooltip=HTML Panel
>+htmlButton.tooltipWithAccesskey=HTML Panel (%1$S + %2$S)

These should be made markup language agnostic, i.e. not HTML specific. This could be done in a separate bug, technically, but doing so would mean extra work for localizers.

>+#inspector-breadcrumbs > .scrollbutton-up:-moz-locale-dir(ltr) {
>+  border-left-width: 0;
>+}
>+
>+#inspector-breadcrumbs > .scrollbutton-up:-moz-locale-dir(rtl) {
>+  border-right-width: 0;
>+}

-moz-border-start

Comment 54

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #53)
> Comment on attachment 606609 [details] [diff] [review]
> patch v1.7.2
> 
> > .devtools-toolbarbutton:not([label]) > .toolbarbutton-text {
> >   display: none;
> > }
> >+
> >+.devtools-toolbarbutton-icononly > .toolbarbutton-text {
> >+  visibility: collapse;
> >+}
> 
> display:none (and merge with the previous rule)

Actually, it's probably better if you just use aria-label instead of the label attribute.

Comment 55

[Paul Rouget [:paul]]

Attachment: patch v1.7.3

Comment 56

[Dão Gottwald [:dao]]

Comment on attachment 610563 [details] [diff] [review]
patch v1.7.3

>+    keysbundle = Services.strings.createBundle(
>+      "chrome://global-platform/locale/platformKeys.properties");
>+    let altString = keysbundle.GetStringFromName("VK_ALT");
>+    let accesskey = button.getAttribute("accesskey");
>+    tooltip = this.strings.formatStringFromName("markupButton.tooltipWithAccesskey",
>+      [altString, accesskey], 2);

>+# On Windows and Linux, we use markupButton.tooltipWithAccesskey, where we append
>+# the name of the Alt key (VK_ALT in chrome://global-platform/locale/platformKeys.properties)
>+# and the acccess key (htmlPanel.accesskey in chrome://browser/locale/browser.dtd).
>+markupButton.tooltip=Markup Panel
>+markupButton.tooltipWithAccesskey=Markup Panel (%1$S + %2$S)

Use MODIFIER_SEPARATOR and let the script merge it with the VK_ALT string and the access key in order to pass everything as one parameter to formatStringFromName.

I think DOM Tree would make more sense than Markup, but your choice.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#inspector-breadcrumbs > .scrollbutton-down > .toolbarbutton-icon:-moz-locale-dir(ltr) {
>+  -moz-transform: scaleX(-1);
>+}
>+
>+#inspector-breadcrumbs > .scrollbutton-up > .toolbarbutton-icon:-moz-locale-dir(rtl) {
>+  -moz-transform: scaleX(-1);
> }

merge

> .inspector-breadcrumbs-button {
>   -moz-appearance: none;
>   background-color: transparent;
>   border-style: solid;
>   border-width: 1px 13px 2px 13px;
>   color: hsl(210,30%,85%);
>-  max-width: 85px;
>+  width: 85px; /* Can't use min-width. See bug 723132 */

"Can't use max-width"

> .inspector-breadcrumbs-button:-moz-locale-dir(ltr):first-of-type {
>   margin-left: 0;
>+  border-left-width: 0;
> }

>+#inspector-breadcrumbs[overflows] > .inspector-breadcrumbs-button:-moz-locale-dir(ltr):last-of-type {
>+  border-right-width: 0;
>+}

> .inspector-breadcrumbs-button:-moz-locale-dir(rtl):first-of-type {
>   margin-right: 0;
>+  border-right-width: 0;
> }

>+#inspector-breadcrumbs[overflows] > .inspector-breadcrumbs-button:last-of-type:-moz-locale-dir(rtl) {
>+  border-left-width: 0;
>+}

Look like -moz-margin-start and -moz-border-start/end should be used here.

Comment 57

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #56)
> > .inspector-breadcrumbs-button:-moz-locale-dir(ltr):first-of-type {
> >   margin-left: 0;
> >+  border-left-width: 0;
> > }
> 
> >+#inspector-breadcrumbs[overflows] > .inspector-breadcrumbs-button:-moz-locale-dir(ltr):last-of-type {
> >+  border-right-width: 0;
> >+}
> 
> > .inspector-breadcrumbs-button:-moz-locale-dir(rtl):first-of-type {
> >   margin-right: 0;
> >+  border-right-width: 0;
> > }
> 
> >+#inspector-breadcrumbs[overflows] > .inspector-breadcrumbs-button:last-of-type:-moz-locale-dir(rtl) {
> >+  border-left-width: 0;
> >+}
> 
> Look like -moz-margin-start and -moz-border-start/end should be used here.

I have tried that, but I didn't manage to make it work. Is -moz-border-start/end supposed to work with border-images?

Comment 58

[Dão Gottwald [:dao]]

What exactly did you try? -moz-border-start? -moz-border-start-width?

Comment 59

[Paul Rouget [:paul]]

-moz-border-start-width:0;

It doesn't seem to work on RTL.

Comment 60

[Dão Gottwald [:dao]]

Have you used DOM Inspector to try to figure out why it doesn't work?

Comment 61

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #60)
> Have you used DOM Inspector to try to figure out why it doesn't work?

Actually, it didn't work because the buttons are forced to ltr (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/highlighter.css#33). So we can't use -moz-border-start/end.

Comment 62

[Paul Rouget [:paul]]

Attachment: patch v1.7.4

Comment 63

[Paul Rouget [:paul]]

Attachment: patch v1.7.5

typo

Comment 64

[Paul Rouget [:paul]]

Comment on attachment 611431 [details] [diff] [review]
patch v1.7.5

Need to be rebased, and to address some related comments in bug 735214.

Comment 65

[Paul Rouget [:paul]]

Attachment: patch v1.7.6

Comment 66

[Paul Rouget [:paul]]

review ping

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 612161 [details] [diff] [review]
patch v1.7.6

patch doesn't apply cleanly

Comment 68

[Paul Rouget [:paul]]

Attachment: patch v1.7.6 (rebased)

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 615660 [details] [diff] [review]
patch v1.7.6 (rebased)

applying https://bug717922.bugzilla.mozilla.org/attachment.cgi?id=615660
patching file browser/base/content/browser.xul
Hunk #1 FAILED at 1002
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/browser.xul.rej
patching file browser/devtools/highlighter/inspector.jsm
Hunk #1 FAILED at 100
Hunk #2 FAILED at 128
Hunk #3 FAILED at 164
3 out of 5 hunks FAILED -- saving rejects to file browser/devtools/highlighter/inspector.jsm.rej
patching file browser/locales/en-US/chrome/browser/devtools/inspector.properties
Hunk #1 FAILED at 26
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/devtools/inspector.properties.rej
patching file browser/themes/winstripe/browser.css
Hunk #1 succeeded at 2830 with fuzz 1 (offset 0 lines).
abort: patch failed to apply

Comment 70

[Paul Rouget [:paul]]

Comment on attachment 615660 [details] [diff] [review]
patch v1.7.6 (rebased)

You need to apply the 2 patches from bug 735214.

Comment 71

[Paul Rouget [:paul]]

Verified with a try.

Landed: https://hg.mozilla.org/integration/fx-team/rev/fc195f2e276a

Comment 72

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fc195f2e276a