Last Comment Bug 717922 - In the PageInspector toolbar, move the HTML Tree button in front of the breadcrumbs display
: In the PageInspector toolbar, move the HTML Tree button in front of the bread...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: x86 All
: P3 normal (vote)
: Firefox 14
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 717923 719607 735214
Blocks: 721593 724507
  Show dependency treegraph
 
Reported: 2012-01-13 07:05 PST by Paul Rouget [:paul]
Modified: 2012-05-21 01:23 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
icon (1.02 KB, image/png)
2012-01-20 17:18 PST, Paul Rouget [:paul]
no flags Details
patch 0.007 - WIP (16.52 KB, patch)
2012-01-20 20:01 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
mockup (72.13 KB, image/png)
2012-01-23 10:20 PST, Paul Rouget [:paul]
no flags Details
better mockup (106.08 KB, image/png)
2012-01-23 10:31 PST, Paul Rouget [:paul]
no flags Details
HTMLPanel icon (805 bytes, image/png)
2012-01-23 10:42 PST, Paul Rouget [:paul]
no flags Details
overflow arrows (545 bytes, image/png)
2012-01-23 10:42 PST, Paul Rouget [:paul]
no flags Details
patch v1 (48.86 KB, patch)
2012-02-06 04:20 PST, Paul Rouget [:paul]
dtownsend: review-
Details | Diff | Review
patch 1.1 (48.26 KB, patch)
2012-02-25 10:16 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch 1.2 (46.88 KB, patch)
2012-02-25 14:00 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch 1.2 (rebased) (47.03 KB, patch)
2012-02-28 04:37 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.3 (46.83 KB, patch)
2012-03-02 06:10 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.4 (47.25 KB, patch)
2012-03-02 06:27 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.5 (47.25 KB, patch)
2012-03-07 08:49 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
wrong patch (47.25 KB, patch)
2012-03-07 11:41 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.6 (47.29 KB, patch)
2012-03-12 10:23 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch v1.7 (49.80 KB, patch)
2012-03-16 04:22 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.1 (49.80 KB, patch)
2012-03-16 04:26 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.2 (52.57 KB, patch)
2012-03-16 09:50 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Review
patch v1.7.3 (50.63 KB, patch)
2012-03-29 08:51 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.4 (50.48 KB, patch)
2012-04-02 06:08 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.5 (50.48 KB, patch)
2012-04-02 06:10 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.6 (50.94 KB, patch)
2012-04-04 05:53 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.7.6 (rebased) (51.10 KB, patch)
2012-04-17 04:22 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Review

Description Paul Rouget [:paul] 2012-01-13 07:05:36 PST
... and we should have an icon for that.
Comment 1 Paul Rouget [:paul] 2012-01-20 17:18:38 PST
Created attachment 590398 [details]
icon
Comment 2 Paul Rouget [:paul] 2012-01-20 20:01:51 PST
Created attachment 590419 [details] [diff] [review]
patch 0.007 - WIP
Comment 3 Paul Rouget [:paul] 2012-01-20 20:13:15 PST
(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] 2012-01-20 20:15:34 PST
In attachment 588662 [details] we can see the intended result.
Comment 5 Dão Gottwald [:dao] 2012-01-22 12:23:11 PST
> 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] 2012-01-22 12:40:50 PST
(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] 2012-01-22 12:47:34 PST
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] 2012-01-23 10:20:08 PST
Created attachment 590772 [details]
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] 2012-01-23 10:31:12 PST
Created attachment 590776 [details]
better mockup
Comment 10 Paul Rouget [:paul] 2012-01-23 10:42:32 PST
Created attachment 590781 [details]
HTMLPanel icon
Comment 11 Paul Rouget [:paul] 2012-01-23 10:42:59 PST
Created attachment 590782 [details]
overflow arrows
Comment 12 Paul Rouget [:paul] 2012-02-06 04:20:41 PST
Created attachment 594673 [details] [diff] [review]
patch v1
Comment 13 Dave Camp (:dcamp) 2012-02-06 16:06:24 PST
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: gavin@gavinsharp.com] 2012-02-06 16:17:52 PST
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] 2012-02-06 16:22:27 PST
See bug 717923 comment 8 and below.
Comment 16 Dão Gottwald [:dao] 2012-02-06 16:23:38 PST
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] 2012-02-06 16:38:57 PST
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] 2012-02-25 08:59:38 PST
(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] 2012-02-25 10:16:16 PST
Created attachment 600689 [details] [diff] [review]
patch 1.1
Comment 20 Paul Rouget [:paul] 2012-02-25 10:27:15 PST
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] 2012-02-25 12:36:13 PST
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] 2012-02-25 14:00:22 PST
Created attachment 600719 [details] [diff] [review]
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] 2012-02-26 07:20:17 PST
Comment on attachment 600719 [details] [diff] [review]
patch 1.2

see bug 717923 comment 18
Comment 24 Paul Rouget [:paul] 2012-02-28 04:37:51 PST
Created attachment 601240 [details] [diff] [review]
patch 1.2 (rebased)
Comment 25 Paul Rouget [:paul] 2012-03-02 06:10:44 PST
Created attachment 602332 [details] [diff] [review]
patch v1.3

no accesskey
Comment 26 Paul Rouget [:paul] 2012-03-02 06:13:15 PST
(waiting to see how the review goes for bug 717923 before asking Dao to review this patch)
Comment 27 Paul Rouget [:paul] 2012-03-02 06:27:31 PST
Created attachment 602337 [details] [diff] [review]
patch v1.4

removed the accesskey for real
Comment 28 Paul Rouget [:paul] 2012-03-06 02:57:31 PST
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] 2012-03-06 02:58:23 PST
bug 717923 comment 24 actually.
Comment 30 Paul Rouget [:paul] 2012-03-07 08:49:06 PST
Created attachment 603742 [details] [diff] [review]
patch v1.5
Comment 31 Paul Rouget [:paul] 2012-03-07 11:41:18 PST
Created attachment 603805 [details] [diff] [review]
wrong patch
Comment 32 Dão Gottwald [:dao] 2012-03-12 08:26:31 PDT
Comment on attachment 603742 [details] [diff] [review]
patch v1.5

needs keyboard access
Comment 33 Paul Rouget [:paul] 2012-03-12 10:16:35 PDT
Just adding tabindex=0 to the toolbarbutton seems to work. Is that ok?
Comment 34 Paul Rouget [:paul] 2012-03-12 10:23:06 PDT
Created attachment 604996 [details] [diff] [review]
patch v1.6
Comment 35 Paul Rouget [:paul] 2012-03-12 10:28:29 PDT
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] 2012-03-13 04:04:01 PDT
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] 2012-03-13 04:21:02 PDT
(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] 2012-03-13 04:26:43 PDT
I don't think we should regress this. Right now the panel can be toggled easily using Alt+H.
Comment 39 Paul Rouget [:paul] 2012-03-13 04:39:28 PDT
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] 2012-03-13 09:05:44 PDT
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] 2012-03-13 09:10:02 PDT
(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] 2012-03-13 09:16:16 PDT
(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] 2012-03-13 09:21:11 PDT
Not sure what "On Mac, it is nothing" means. Why would we care about ui.key.menuAccessKey being changed?
Comment 44 Paul Rouget [:paul] 2012-03-13 09:34:50 PDT
(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] 2012-03-13 10:01:49 PDT
(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] 2012-03-13 10:42:36 PDT
(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] 2012-03-13 10:55:39 PDT
CCing one of the a11y guys, as our support for that on osx is changing, I think.
Comment 48 Paul Rouget [:paul] 2012-03-14 09:14:53 PDT
triage, filter on centaur
Comment 49 Paul Rouget [:paul] 2012-03-16 04:22:08 PDT
Created attachment 606525 [details] [diff] [review]
patch v1.7
Comment 50 Paul Rouget [:paul] 2012-03-16 04:26:57 PDT
Created attachment 606526 [details] [diff] [review]
patch v1.7.1

typo
Comment 51 Paul Rouget [:paul] 2012-03-16 09:50:16 PDT
Created attachment 606609 [details] [diff] [review]
patch v1.7.2
Comment 52 Paul Rouget [:paul] 2012-03-16 09:52:38 PDT
Comment on attachment 606609 [details] [diff] [review]
patch v1.7.2

(rebased since bug 736418 is WONTFIX)
Comment 53 Dão Gottwald [:dao] 2012-03-20 06:38:03 PDT
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] 2012-03-20 06:51:11 PDT
(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] 2012-03-29 08:51:37 PDT
Created attachment 610563 [details] [diff] [review]
patch v1.7.3
Comment 56 Dão Gottwald [:dao] 2012-03-30 01:35:30 PDT
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] 2012-03-30 10:08:58 PDT
(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] 2012-03-30 11:13:45 PDT
What exactly did you try? -moz-border-start? -moz-border-start-width?
Comment 59 Paul Rouget [:paul] 2012-03-30 11:17:32 PDT
-moz-border-start-width:0;

It doesn't seem to work on RTL.
Comment 60 Dão Gottwald [:dao] 2012-04-02 03:46:11 PDT
Have you used DOM Inspector to try to figure out why it doesn't work?
Comment 61 Paul Rouget [:paul] 2012-04-02 06:05:21 PDT
(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] 2012-04-02 06:08:06 PDT
Created attachment 611430 [details] [diff] [review]
patch v1.7.4
Comment 63 Paul Rouget [:paul] 2012-04-02 06:10:56 PDT
Created attachment 611431 [details] [diff] [review]
patch v1.7.5

typo
Comment 64 Paul Rouget [:paul] 2012-04-04 05:14:50 PDT
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] 2012-04-04 05:53:48 PDT
Created attachment 612161 [details] [diff] [review]
patch v1.7.6
Comment 66 Paul Rouget [:paul] 2012-04-12 08:09:18 PDT
review ping
Comment 67 Dão Gottwald [:dao] 2012-04-14 11:20:23 PDT
Comment on attachment 612161 [details] [diff] [review]
patch v1.7.6

patch doesn't apply cleanly
Comment 68 Paul Rouget [:paul] 2012-04-17 04:22:03 PDT
Created attachment 615660 [details] [diff] [review]
patch v1.7.6 (rebased)
Comment 69 Dão Gottwald [:dao] 2012-04-18 06:18:52 PDT
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] 2012-04-18 06:29:38 PDT
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] 2012-04-19 06:17:39 PDT
Verified with a try.

Landed: https://hg.mozilla.org/integration/fx-team/rev/fc195f2e276a
Comment 72 Tim Taubert [:ttaubert] 2012-04-20 01:59:05 PDT
https://hg.mozilla.org/mozilla-central/rev/fc195f2e276a

Note You need to log in before you can comment on or make changes to this bug.