393820 - expose bounds of accessible for accessible properties view

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[[e-mail]]

Comment on attachment 278373 [details] [diff] [review]
patch

>+    return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
>+     "; height: " + height.value;
Doesn't this need to be a formatted localised string?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 278373 [details] [diff] [review])
> >+    return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
> >+     "; height: " + height.value;
> Doesn't this need to be a formatted localised string?
> 

Dunno. I don't care about localization a lot. If you think it should be localized I will. But should be x and y localized? though probably geometry language is not subjected to localization issue :)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 278373 [details] [diff] [review])
> >+    return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
> >+     "; height: " + height.value;
> Doesn't this need to be a formatted localised string?
> 

Though boxModel view keeps them unlocalized http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/content/viewers/boxModel/boxModel.js#173

Comment 4

[[e-mail]]

Comment on attachment 278373 [details] [diff] [review]
patch

>+    return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
>+     "; height: " + height.value;
OK, so don't worry about the l10n, but the indentation is weird :-P
Here are some suggestions, since I couldn't decide myself:

return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
                                             "; height: " + height.value;

return "x: " + x.value + "; y: " + y.value + "; width: " + width.value +
                         "; height: " + height.value;

return "x: " + x.value + "; y: " + y.value +
                         "; width: " + width.value +
                         "; height: " + height.value;

Comment 5

[Axel Hecht [:Pike]]

I could see both transcription of x and y, as well as RTL changes for that string.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> I could see both transcription of x and y, as well as RTL changes for that
> string.
> 

Sorry, didn't catch you. What is transcription of x and y and what is RTL changes?

Comment 7

[Axel Hecht [:Pike]]

Transcription would mean that a localization in say Cyrillic would use the Cyrillic letters for x and y. And in arab and other RTL languages, folks read from right to left. Not exactly sure how that should look, but the localizers would know.

Please don't add english strings to localizations, but remove them from the ALL_LOCALES in http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/Makefile.in#47.
We're doing this on purpose.

As you're busting l10n anyway, let's go for the full monty. And I'd consider the boxModel non-localizable strings as a different bug, but a bug.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

with Axel's recommendations

Comment 9

[Axel Hecht [:Pike]]

That won't work for RTL. I suggest you use a .properties entry like

# LOCALIZATION NOTE (bounds.label): %1$d is x (width), %2$d is y (height)
bounds.label = x: %1$d; y: %2$d;

I.e., the key value pairs should be one entry, and you can probably just use a single group for both x and y and get the output you had intially.

If you prefer the output you had in your second patch, you should try to get someone from the RTL l10n community to comment on how to mark up the XUL so that it actually reverses the display.

Sorry, I could have been more verbatim on how to achieve this in the first place.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> That won't work for RTL. I suggest you use a .properties entry like
> 
> # LOCALIZATION NOTE (bounds.label): %1$d is x (width), %2$d is y (height)
> bounds.label = x: %1$d; y: %2$d;
>
> I.e., the key value pairs should be one entry, and you can probably just use a
> single group for both x and y and get the output you had intially.

Sorry, how will the end output look?
 
> If you prefer the output you had in your second patch, you should try to get
> someone from the RTL l10n community to comment on how to mark up the XUL so
> that it actually reverses the display.

Do you know someone to cc?

Also do you mean some languages may prefer to show value before label?

Comment 11

[Axel Hecht [:Pike]]

So, there are a few l12y (localizability) problems with your second patch:

You hardcode the ';' to the end of the number. That may be wrong. I don't have any language using ';' semantically, but I wouldn't rule that out. In RTL languages, the display should probably look like
;456 :y ; 123 :x
or so. Adding two RTL localizers for real input.

In my suggestion, you'd get the string bundle, and then you'd do

var params = [x.value, y.value];
return bundle.formatStringFromName('bounds.label', params, params.length);

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3

with Axel's approach. I do not use %d because it doesn't seems to work correctly.

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 278786 [details] [diff] [review]
patch3

Yeah, %d is crap. Sorry, I was looking too much on js, and too little on idl (which converts all params to wstring, thus only S works).

Anyway, I think you want to add a 

dir="&locale.dir;" 

to your <row>s to make RTL languages work, similar to what netError does. http://lxr.mozilla.org/mozilla/source/docshell/resources/content/netError.xhtml#11 shows how to grab dom's global.dtd, too, which has that defined.

The other way to do it is to use chromedir="&locale.dir;" and to make the theme pick that up, similar to what 
http://lxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/browser.css#81
does. I guess that's only really good if you actually expect themes to do that, though.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 278786 [details] [diff] [review]
patch3

Canceling review - please re-request once l10n questions are addressed.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (From update of attachment 278786 [details] [diff] [review])
> Yeah, %d is crap. Sorry, I was looking too much on js, and too little on idl
> (which converts all params to wstring, thus only S works).
> 
> Anyway, I think you want to add a 
> 
> dir="&locale.dir;" 
> 
> to your <row>s to make RTL languages work, similar to what netError does.

I should use @dir to get 
height, widht, y, x
instead of x, y, width, height
right?

Comment 16

[Axel Hecht [:Pike]]

Please play with it. Just hardcode it to "rtl" to see the effect it has, it's hard to describe. It "should just work" (TM).

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch4

correct?

Comment 18

[Axel Hecht [:Pike]]

I have since looked deeper and asked in .xul, see Neil's reply in 
http://groups.google.com/group/mozilla.dev.tech.xul/browse_frm/thread/99b59ef1f8a54fb9/482a36f9bb272622?lnk=raot#482a36f9bb272622

Sorry for the confusion, but could you test this in the original version with

window,prefwindow,dialog,wizard,page,menu { direction: rtl; }

in userChrome.css? That should mimic what localizers do in intl.css in general.

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 279723 [details] [diff] [review]
patch4

Setting this to tentative r-.

I'm waiting for some testing results from Alexander, see my previous comment.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch5

It looks correct with window,prefwindow,dialog,wizard,page,menu { direction: rtl; }. All is from right to left. When I append @dir attribute then the order is changed and it looks strange a bit.

Comment 21

[Axel Hecht [:Pike]]

Comment on attachment 292369 [details] [diff] [review]
patch5

Looks good to me, you'll need domi reviewers still, not sure who'd that be.

Comment 22

[[e-mail]]

Comment on attachment 292369 [details] [diff] [review]
patch5

>+  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd"> %globalDTD;
Not used?

>+        <grid>
>+          <columns>
>+            <column/>
>+            <column/>
>+            <column/>
>+            <column/>
>+          </columns>
>+          <rows>
>+            <row>
>+              <description id="bounds-x"/>
>+              <description id="bounds-y"/>
>+              <description id="bounds-width"/>
>+              <description id="bounds-height"/>
>+            </row>
>+          </rows>
>+        </grid>
Isn't this overkill? Why not just format all four values in one string?
accBounds = x: %1$S; y: %2$S; width: %3$S; height: %4$S;

>+* locale/@AB_CD@/inspector/viewers/accessibleProps.properties  (@AB_CD@/viewers/accessibleProps.properties)
Nit: not in alphabetical order.

>+accBoundsX = x: %1$s;
>+accBoundsY = y: %1$s;
>+accBoundsWidth = width: %1$s;
>+accBoundsHeight = height: %1$s;
These are supposed to be uppercase S.

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 292369 [details] [diff] [review]
patch5

please address sr comments before asking for review again.

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> >+        <grid>
> >+          <columns>
> >+            <column/>
> >+            <column/>
> >+            <column/>
> >+            <column/>
> >+          </columns>
> >+          <rows>
> >+            <row>
> >+              <description id="bounds-x"/>
> >+              <description id="bounds-y"/>
> >+              <description id="bounds-width"/>
> >+              <description id="bounds-height"/>
> >+            </row>
> >+          </rows>
> >+        </grid>
> Isn't this overkill? Why not just format all four values in one string?
> accBounds = x: %1$S; y: %2$S; width: %3$S; height: %4$S;

Possibly, but it looks a bit better, there are some space between items.

> >+* locale/@AB_CD@/inspector/viewers/accessibleProps.properties  (@AB_CD@/viewers/accessibleProps.properties)
> Nit: not in alphabetical order.

Why? properties files are listed at the end.
 
> >+accBoundsX = x: %1$s;
> >+accBoundsY = y: %1$s;
> >+accBoundsWidth = width: %1$s;
> >+accBoundsHeight = height: %1$s;
> These are supposed to be uppercase S.
> 

Comment 26

[[e-mail]]

(In reply to comment #25)
> > >+* locale/@AB_CD@/inspector/viewers/accessibleProps.properties  (@AB_CD@/viewers/accessibleProps.properties)
> > Nit: not in alphabetical order.
> Why? properties files are listed at the end.
There's no excuse to copy caillon's mistake in bug 109891 ;-)

Comment 27

[[e-mail]]

Comment on attachment 294050 [details] [diff] [review]
patch6

>+accBoundsX = x: %1$S;
>+accBoundsY = y: %1$S;
>+accBoundsWidth = width: %1$S;
>+accBoundsHeight = height: %1$S;
Nit: assuming you're only going to have one insertion per string, there's hardly any point using 1$ - %S on its own would do.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch7

Comment 29

[Shawn Wilsher :sdwilsh]

Comment on attachment 294321 [details] [diff] [review]
patch7

>Index: extensions/inspector/resources/locale/en-US/viewers/accessibleProps.dtd
>+<!ENTITY descBounds.label "Bounds:">
>Index: extensions/inspector/resources/locale/en-US/viewers/accessibleProps.properties
>+accBoundsX = x: $S;
>+accBoundsY = y: $S;
>+accBoundsWidth = width: $S;
>+accBoundsHeight = height: $S;
Note: these changes will break the build since other locale's won't have them.  You'll need to remove them from the Makefile.

r=sdwilsh with that fixed.

Comment 30

[alexander :surkov (:asurkov)]

Attachment: patch8

with fixed Shawn's comment

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch9

correct patch

Comment 32

[alexander :surkov (:asurkov)]

Attachment: patch10

sorry for the spam. this is the correct patch

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch10

sorry for the spam. this is the correct patch

Comment 34

[Damon Sicore (:damons)]

Comment on attachment 299969 [details] [diff] [review]
patch10

a1.9+=damons

Comment 35

[Mike Connor [:mconnor]]

Comment on attachment 299969 [details] [diff] [review]
patch10

too late for b3, especially with the string change.

Comment 36

[Shawn Wilsher :sdwilsh]

why would a string change matter for the DOM Inspector?

Comment 37

[Cédric Corazza]

Because DOMi is localized and shipped, at least in some locales.

Comment 38

[alexander :surkov (:asurkov)]

checked in