Closed Bug 717922 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect, P3)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(6 files, 17 obsolete files)

1.02 KB, image/png
Details
72.13 KB, image/png
Details
106.08 KB, image/png
Details
805 bytes, image/png
Details
545 bytes, image/png
Details
51.10 KB, patch
dao
: review+
Details | Diff | Splinter Review
... and we should have an icon for that.
Assignee: nobody → paul
Depends on: 719607
Attached image icon
Attached patch patch 0.007 - WIP (obsolete) — Splinter Review
(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?
In attachment 588662 [details] we can see the intended result.
> 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.
(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.
Again: Since the html tree button doesn't scroll, it doesn't seem to make sense to have it wrapped by the scroll buttons.
Attached image 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.
Attached image better mockup
Attached image HTMLPanel icon
Attached image overflow arrows
Depends on: 717923
Blocks: 724507
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #594673 - Flags: review?(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?
Attachment #594673 - Flags: review?(dcamp) → review?(dtownsend+bugmail)
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.
You don't need to touch scrollbox.xml here, just listen to underflow/overflow events and set a custom attribute.
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.
Attachment #594673 - Flags: review?(dtownsend+bugmail) → review-
(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).
Attached patch patch 1.1 (obsolete) — Splinter Review
Attachment #594673 - Attachment is obsolete: true
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.
Attachment #600689 - Flags: review?(dao)
Attached patch patch 1.2 (obsolete) — Splinter Review
So I listen to the overflow/underflow events and then check if the scroll buttons
are collapsed or not. It works.
Attachment #600689 - Attachment is obsolete: true
Attachment #600689 - Flags: review?(dao)
Attachment #600719 - Flags: review?(dao)
Comment on attachment 600719 [details] [diff] [review]
patch 1.2

see bug 717923 comment 18
Attachment #600719 - Flags: review?(dao) → review-
Blocks: 683954
Attached patch patch 1.2 (rebased) (obsolete) — Splinter Review
Attachment #600719 - Attachment is obsolete: true
Attached patch patch v1.3 (obsolete) — Splinter Review
no accesskey
Attachment #601240 - Attachment is obsolete: true
(waiting to see how the review goes for bug 717923 before asking Dao to review this patch)
Attached patch patch v1.4 (obsolete) — Splinter Review
removed the accesskey for real
Attachment #602332 - Attachment is obsolete: true
Blocks: 717916
Blocks: 721593
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?
bug 717923 comment 24 actually.
No longer blocks: 717916
Attached patch patch v1.5 (obsolete) — Splinter Review
Attachment #602337 - Attachment is obsolete: true
Attachment #603742 - Flags: review?(dao)
Attached patch wrong patch (obsolete) — Splinter Review
Attachment #603805 - Attachment description: patch v1.1 → wrong patch
Attachment #603805 - Attachment is obsolete: true
Comment on attachment 603742 [details] [diff] [review]
patch v1.5

needs keyboard access
Attachment #603742 - Flags: review?(dao) → review-
Just adding tabindex=0 to the toolbarbutton seems to work. Is that ok?
Attached patch patch v1.6 (obsolete) — Splinter Review
Comment on attachment 604996 [details] [diff] [review]
patch v1.6

Just added tabindex="0" to the button. Hope that's enough.
Attachment #604996 - Flags: review?(dao)
Attachment #603742 - Attachment is obsolete: true
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
Attachment #604996 - Flags: review?(dao) → review-
(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?
I don't think we should regress this. Right now the panel can be toggled easily using Alt+H.
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?
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?
(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.
(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
Not sure what "On Mac, it is nothing" means. Why would we care about ui.key.menuAccessKey being changed?
(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
(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.
(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?
CCing one of the a11y guys, as our support for that on osx is changing, I think.
triage, filter on centaur
Priority: -- → P3
Whiteboard: [firefox14-wanted]
Status: NEW → ASSIGNED
Depends on: 736418
Attached patch patch v1.7 (obsolete) — Splinter Review
Attachment #604996 - Attachment is obsolete: true
Attached patch patch v1.7.1 (obsolete) — Splinter Review
typo
Attachment #606525 - Attachment is obsolete: true
Attachment #606526 - Flags: review?(dao)
Depends on: 735214
Attached patch patch v1.7.2 (obsolete) — Splinter Review
Attachment #606526 - Attachment is obsolete: true
Attachment #606526 - Flags: review?(dao)
Comment on attachment 606609 [details] [diff] [review]
patch v1.7.2

(rebased since bug 736418 is WONTFIX)
Attachment #606609 - Flags: review?(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
Attachment #606609 - Flags: review?(dao) → review-
Attachment #590419 - Attachment is obsolete: true
(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.
Attached patch patch v1.7.3 (obsolete) — Splinter Review
Attachment #606609 - Attachment is obsolete: true
Attachment #610563 - Flags: review?(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.
(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?
What exactly did you try? -moz-border-start? -moz-border-start-width?
-moz-border-start-width:0;

It doesn't seem to work on RTL.
Have you used DOM Inspector to try to figure out why it doesn't work?
(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.
Attached patch patch v1.7.4 (obsolete) — Splinter Review
Attachment #610563 - Attachment is obsolete: true
Attachment #610563 - Flags: review?(dao)
Attached patch patch v1.7.5 (obsolete) — Splinter Review
typo
Attachment #611430 - Attachment is obsolete: true
Attachment #611431 - Flags: review?(dao)
Comment on attachment 611431 [details] [diff] [review]
patch v1.7.5

Need to be rebased, and to address some related comments in bug 735214.
Attachment #611431 - Flags: review?(dao)
Attached patch patch v1.7.6 (obsolete) — Splinter Review
Attachment #611431 - Attachment is obsolete: true
No longer depends on: 736418
Attachment #612161 - Flags: review?(dao)
No longer blocks: 683954
review ping
Comment on attachment 612161 [details] [diff] [review]
patch v1.7.6

patch doesn't apply cleanly
Attachment #612161 - Flags: review?(dao)
Attachment #612161 - Attachment is obsolete: true
Attachment #615660 - Flags: review?(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
Attachment #615660 - Flags: review?(dao)
Comment on attachment 615660 [details] [diff] [review]
patch v1.7.6 (rebased)

You need to apply the 2 patches from bug 735214.
Attachment #615660 - Flags: review?(dao)
Attachment #615660 - Flags: review?(dao) → review+
Verified with a try.

Landed: https://hg.mozilla.org/integration/fx-team/rev/fc195f2e276a
Whiteboard: [firefox14-wanted] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fc195f2e276a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
No longer depends on: 735214
Depends on: 735214
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.