348233 - FTP and Gopher index pages are LTR always

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Behnam Esfahbod [:zwnj]]

FTP and Gopher index pages are LTR always.

Screenshots in Persian (RTL) locale will be attached.

Comment 1

[Behnam Esfahbod [:zwnj]]

Attachment: FTP index in Persian locale

Comment 2

[Behnam Esfahbod [:zwnj]]

Attachment: Gopher index in Persian locale

Comment 3

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 4

[Tomer Cohen :tomer]

I can reproduce in Firefox 3.1 beta 2 with Hebrew UI. 

It seems that this page is being rendered in nsIndexedToHTML.cpp. I think it won't be too complicated to fix it.

Comment 5

[(no longer active)]

I'm going to give this a shot.

Comment 6

[(no longer active)]

Does anyone know of a working gopher server which can be used to test the patch here?  Does Mozilla still support the gopher protocol at all?

Comment 7

[Behnam Esfahbod [:zwnj]]

This one works yet: gopher://quux.org/

Comment 8

[Behnam Esfahbod [:zwnj]]

And this is a gopher-based gopher search site, which you can find a lot of working servers: gopher://gopher.floodgap.com/7/v2/vs?firefox

Comment 9

[(no longer active)]

Attachment: WIP Patch

We need to import chrome://global/content/global.dtd for the locale.dir entity here.  Since this doesn't seem to be possible for HTML documents (or I don't know how), I tired to change the document to XHTML.  With this patch, any attempt to view a file:// or gopher URL results in a blank page.  DOM Inspector only shows a HEAD element under HTML, and nothing gets logged to the error console.

Can someone point out what's wrong here?

Comment 10

[(no longer active)]

Attachment: Patch (v1)

The meat of this patch is based on that of bug 437844.  I will comment on some parts of the patch shortly to make the review process easier.  I extended the test suite for bug 437844 to test for this bug as well.

Comment 11

[(no longer active)]

Comment on attachment 359599 [details] [diff] [review]
Patch (v1)

Promised comments:

>@@ -296,27 +297,35 @@ nsIndexedToHTML::OnStartRequest(nsIReque
>+                         // Bug 475986 makes this necessary, otherwise
>+                         // "text-align: start" would be fine
>+                         "body[chromedir=\"rtl\"] th {\n"
>+                         "  text-align: right;\n"
>+                         "}\n"

Bug 475986 (which I found when developing this patch) caused text-align:start not to work for <th> elements, so we have to use chromedir here.

>+                         "}\n"
>+                         "body[chromedir=\"rtl\"] table[order] > thead > tr > th::after {\n"
>+                         "  text-align: left;\n"

We don't have text-align:end...

>@@ -343,31 +352,37 @@ nsIndexedToHTML::OnStartRequest(nsIReque
>+                         "  }\n"
>+                         "  table {\n"
>+                         "    direction: ltr;\n"

It doesn't make sense to reverse the direction of gopher listings, because they're not usually in RTL language.  Doing so would look like reversing the direction of LTR documents if the locale direction is RTL.  This simple rule makes sure that the direction of the gopher listing remains LTR, while the rest of the page (header) is correctly converted to RTL.

>diff --git a/toolkit/themes/pinstripe/global/dirListing/dirListing.css b/toolkit/themes/pinstripe/global/dirListing/dirListing.css
> /* last modified */
> th:last-child {
>-  text-align: center;
>+  text-align: center !important;
> }

Without this, the Last Modified column header in RTL pages would be aligned to right.

>diff --git a/toolkit/themes/winstripe/global/dirListing/dirListing.css b/toolkit/themes/winstripe/global/dirListing/dirListing.css
> /* last modified */
> th:last-child {
>-  text-align: center;
>+  text-align: center !important;
> }

Same here.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Probably good to get Dao to review this, since he wrote most of this code...

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'd rather we fix bug 475986 or at least sort out what the desired behavior is there, btw, before we hack around it here.

Comment 14

[(no longer active)]

Comment on attachment 359599 [details] [diff] [review]
Patch (v1)

Dao: can you please take a look as well?  Thanks!

Comment 15

[Dão Gottwald [:dao]]

The WIP patch is a lot nicer. Can we continue there?

Comment 16

[(no longer active)]

(In reply to comment #15)
> The WIP patch is a lot nicer. Can we continue there?

Converting this to XHTML did not work.  I suspect it was because of the differences between the two (for example an item in this list <http://wiki.whatwg.org/wiki/HTML_vs._XHTML>)) but the major problem was that nothing was reported to the error console and DOM Inspector was useless as well.

Do you know of any way to dump the resulting converted string somewhere to make it possible for me to debug the problem?

Comment 17

[Dão Gottwald [:dao]]

The xml namespace is missing from the html element. But it doesn't like any data is being processed, so there must be more wrong...

Comment 18

[(no longer active)]

So, what's happening is that nsDirectoryViewerFactory is creating an HTML document for directory listings <http://mxr.mozilla.org/mozilla-central/source/xpfe/components/directory/nsDirectoryViewer.cpp#1493>, whereas the content of the document tells the parser it's HTML.  So, the parser ends up using expat on an HTML content sink, which fails on this line: <http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsExpatDriver.cpp#1222>.  I'm not sure if blindly switching to XHTML mimetype in nsDirectoryViewerFactory::CreateInstance is the way to go here, or if it affects other pieces of the puzzle that I may not be aware of...

Comment 19

[(no longer active)]

Attachment: Patch (XHTML)

I tried switching to the XHTML doctype, and based on testing it seems to work correctly.  I also ported the test and the CSS fixes from the other patch into this patch, which is hopefully cleaner.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 359914 [details] [diff] [review]
Patch (XHTML)

>+                         "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.1//EN\"\n"
>+                         "  \"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd\" ["

I think "<!DOCTYPE html [" would be sufficient.

>                          "table[order] > thead > tr > th::after {\n"
[...]
>                          "  text-align: right;\n"
>+                         "}\n"
>+                         "body[dir=\"rtl\"] table[order] > thead > tr > th::after {\n"
>+                         "  text-align: left;\n"

This could use a reference to bug 299837.

> th:last-child {
>-  text-align: center;
>+  text-align: center !important;
> }

When bug 475986 gets fixed, this should also be reverted.

Comment 21

[(no longer active)]

(In reply to comment #20)
> > th:last-child {
> >-  text-align: center;
> >+  text-align: center !important;
> > }
> 
> When bug 475986 gets fixed, this should also be reverted.

Sure.  I'll do this upon landing (in case bug 475986 doesn't get fixed in time, I'll file a followup bug so that this won't get forgotten).

Comment 22

[(no longer active)]

Attachment: Patch (XHTML)

Addressed Dao's review comments.

Comment 23

[Boris Zbarsky [:bzbarsky]]

So is there a good reason to not just fix bug 475986 and bug 299837?

Also, there are some places in this patch that are using left+right when they should just use start on <td>, no?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ehsan, want to update the patch now that the text-align bugs are fixed?

Comment 25

[(no longer active)]

Attachment: Updated patch

Comment 26

[(no longer active)]

(In reply to comment #23)
> Also, there are some places in this patch that are using left+right when they
> should just use start on <td>, no?

I'm not sure which changes you were referring to here.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 360924 [details] [diff] [review]
Updated patch

Looks OK except for the test.  Tests must not require network access to run.

Comment 28

[(no longer active)]

(In reply to comment #27)
> (From update of attachment 360924 [details] [diff] [review])
> Looks OK except for the test.  Tests must not require network access to run.

Hmm, do we have support for testing FTP and gopher servers any other way?  Would it be OK if I handle the no-network situation inside the test so that it doesn't fail?

Comment 29

[(no longer active)]

Attachment: Final patch

This is the same as the previous patch, except that I limited the test to file:// URIs because the FTP and gopher listings are made by the same code, and it should be enough to test the file:// URIs here.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Hmm, do we have support for testing FTP and gopher servers any other way? 

Probably not yet.  Sorry I missed that comment; wasnt cced on the bug.

For what it's worth, if I say "r+sr with this one change" there's generally no need to ask me to look again (esp. if the change was as simple as just removing a hunk of code I asked you to remove, as here).  I trust you to do that right on your own and all. ;)

Comment 31

[(no longer active)]

(In reply to comment #30)
> > Hmm, do we have support for testing FTP and gopher servers any other way? 
> 
> Probably not yet.  Sorry I missed that comment; wasnt cced on the bug.
> 
> For what it's worth, if I say "r+sr with this one change" there's generally no
> need to ask me to look again (esp. if the change was as simple as just removing
> a hunk of code I asked you to remove, as here).  I trust you to do that right
> on your own and all. ;)

Yeah, I was just not sure whether you're OK with removing the tests, or you're suggesting to do the tests another way (hence the question about whether we have support for testing FTP and gopher servers any other way).  :-)

BTW, am I good to go to request approval on this patch for 1.9.1, and land it without the CSS fixes in the two dependencies?

Comment 32

[Boris Zbarsky [:bzbarsky]]

It won't work there without those fixes, will it?  And in fact will trigger CSS warnings for the unimplemented "end" value....

Comment 33

[(no longer active)]

(In reply to comment #32)
> It won't work there without those fixes, will it?  And in fact will trigger CSS
> warnings for the unimplemented "end" value....

I meant to use the CSS styles in attachment 359989 [details] [diff] [review], which work without those two fixes (and in fact work around those bugs).  It won't be an ideal solution but it works, and I think it's enough for 1.9.1.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  That would be fine; I assumed by "this patch" you meant the one going into m-c.

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/35d3b66853a8

Comment 36

[(no longer active)]

Comment on attachment 361135 [details] [diff] [review]
Final patch

This patch fixes the directionality of the directory listing pages in RTL locales.  It includes a test.  Requesting approval to land on 1.9.1.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 361135 [details] [diff] [review]
Final patch

a191=beltzner

Comment 38

[(no longer active)]

Landed on 1.9.1 with the required changes to handle the fact that bugs 299837 and 475986 will not be fixed on that branch.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/452aef18ffa2

Comment 39

[Tomer Cohen :tomer]

Attachment: Firefox 3.5 with Hebrew locale

Ehsan, I am afraid I am seeing a regression in your fix. Please look on the screenshot above, the string "file:" is overlapping the file name!

Comment 40

[Tomer Cohen :tomer]

Attachment: ar screenshot

Also reproducible for ar and fa.