742777 - A couple of minor tweaks to the Media Tab.

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Philip Chee]

Port the following Firefox PageInfo patches:
Bug 492250 - URLs in Page Info Media tab should be LTR.
Bug 573603 - Speed up Page Info > Media.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed Fix.

Well nothing looks broken but I don't know how to test.
And don't we need some css in the skin like?:

treechildren::-moz-tree-cell-text(ltr) {
  direction: ltr !important;
}

Comment 2

[[e-mail]]

(In reply to Philip Chee from comment #1)
> And don't we need some css in the skin like?:
> 
> treechildren::-moz-tree-cell-text(ltr) {
>   direction: ltr !important;
> }
Bug 477902.

Comment 3

[[e-mail]]

Actually, I just noticed we need to fix the Links and Forms tabs too. Also, what about the URL fields such as Address(2×) and Referring URL?

Comment 4

[Philip Chee]

Attachment: Patch v1.1 Fix Links and Forms tabs too.

> Actually, I just noticed we need to fix the Links and Forms tabs too.
Fixed.

> Also, what about the URL fields such as Address(2×) and Referring URL?
I think I'll ask Ehsan, he's the expert after all. Ehsan?

Comment 5

[[e-mail]]

Comment on attachment 612900 [details] [diff] [review]
Patch v1.1 Fix Links and Forms tabs too.

[Sorry if these apply to the previous patch but I had only done a ui review.]

>+  this._ltrAtom = gAtomSvc.getAtom("ltr");
>+  this._brokenAtom = gAtomSvc.getAtom("broken");
I don't like this because it means that each view has their own reference to these atoms, even though they may never use them, and additionally I think I would prefer it if you override getCellProperties in the views that need it as this seems to be a better match for the code style in the rest of the file.

>   getRowProperties: function(row, prop) { },
>-  getCellProperties: function(row, column, prop) { },
>+
>+  getCellProperties: function(row, column, prop)
[getCellProperties was here because it was a dummy method. Now that it has a real body, you would need to move it before getRowProperties.]

>+    switch (column.element.id) {
FYI column.id works.

Comment 6

[Philip Chee]

Attachment: Patch v1.2 Fix Address (x2) and Referer textboxes too.

>>+  this._ltrAtom = gAtomSvc.getAtom("ltr");
>>+  this._brokenAtom = gAtomSvc.getAtom("broken");
> I don't like this because it means that each view has their own reference to
> these atoms, even though they may never use them, and additionally I think I
> would prefer it if you override getCellProperties in the views that need it
> as this seems to be a better match for the code style in the rest of the file.
Fixed.

>>   getRowProperties: function(row, prop) { },
>>-  getCellProperties: function(row, column, prop) { },
>>+
>>+  getCellProperties: function(row, column, prop)
> [getCellProperties was here because it was a dummy method. Now that it has
> a real body, you would need to move it before getRowProperties.]
Back to dummy.

>>+    switch (column.element.id) {
> FYI column.id works.
Fixed.

> Also, what about the URL fields such as Address(2×) and Referring URL?
Fixed.

Comment 7

[(no longer active)]

Comment on attachment 613116 [details]
Patch v1.2 Fix Address (x2) and Referer textboxes too.

text-align: right is a bad idea.  If you want the text to be aligned to the right in LTR builds and to the left in RTL builds, you want text-align: end.

Also, please submit a screenshot in RTL and LTR modes in addition to the patch.

Comment 8

[[e-mail]]

(In reply to Ehsan Akhgari from comment #7)
> text-align: right is a bad idea.  If you want the text to be aligned to the
> right in LTR builds and to the left in RTL builds, you want text-align: end.
Unfortunately he also uses direction: ltr; so start and end will always be the same as left and right. I don't know what we can do about this.

Comment 9

[Philip Chee]

Comment on attachment 613116 [details]
Patch v1.2 Fix Address (x2) and Referer textboxes too.

Yes sorry. Thinko on my part.

Comment 10

[Philip Chee]

Attachment: Patch v1.3 Fix LTR/RTL issues.

> Ehsan Akhgari [:ehsan]      2012-04-07 10:03:38 PDT

> Comment on attachment 613116 [details]
> Patch v1.2 Fix Address (x2) and Referer textboxes too.
> 
> text-align: right is a bad idea.  If you want the text to be aligned to the
> right in LTR builds and to the left in RTL builds, you want text-align: end.
Fixed. Now using:

> +.urltext:-moz-locale-dir(rtl) {
> +  direction: ltr !important;
> +  text-align: end !important;
> +}

> Also, please submit a screenshot in RTL and LTR modes in addition to the
> patch.
Will do.

> Unfortunately he also uses direction: ltr; so start and end will always be
> the same as left and right. I don't know what we can do about this.
I intended to use |:-moz-locale-dir(rtl)| in the previous patch but somehow forgot to do so.

Comment 11

[Philip Chee]

Attachment: Media Tab LTR mode.

Comment 12

[Philip Chee]

Attachment: Media Tab RTL mode.

Comment 13

[[e-mail]]

Comment on attachment 613164 [details] [diff] [review]
Patch v1.3 Fix LTR/RTL issues.

>+gImageView._brokenAtom = atomSvc.getAtom("broken");
>+gImageView._ltrAtom = atomSvc.getAtom("ltr");
>+gFormView._ltrAtom = atomSvc.getAtom("ltr");
>+gLinkView._ltrAtom = atomSvc.getAtom("ltr");
File style doesn't go with prefixes (either _ or m) so drop the _s please.

>-    props.AppendElement(gBrokenAtom);
>+    props.AppendElement(this._brokenAtom);
Or you could always switch to gBrokenAtom and gLtrAtom ;-)

Comment 14

[Philip Chee]

Attachment: Patch v1.4 (as checked in) r=ehsan r=Neil

>>+gImageView._brokenAtom = atomSvc.getAtom("broken");
>>+gImageView._ltrAtom = atomSvc.getAtom("ltr");
>>+gFormView._ltrAtom = atomSvc.getAtom("ltr");
>>+gLinkView._ltrAtom = atomSvc.getAtom("ltr");
> File style doesn't go with prefixes (either _ or m) so drop the _s please.

>>-    props.AppendElement(gBrokenAtom);
>>+    props.AppendElement(this._brokenAtom);
> Or you could always switch to gBrokenAtom and gLtrAtom ;-)
Now using gBrokenAtom and gLtrAtom!

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/0f8b5129cfe6