Closed Bug 299480 Opened 19 years ago Closed 19 years ago

neterror.xhtml startup document isn't RTL in RTL locales

Categories

(Core :: DOM: Navigation, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta4

People

(Reporter: linxspider, Assigned: asaf)

References

(Blocks 1 open bug)

Details

(Keywords: rtl, Whiteboard: [affects l10n])

Attachments

(1 file, 5 obsolete files)

on rtl locales as Hebrew and Arabic, the startup document on net error remains
LTR on RTL interface.

steps to reproduce:
1. download an Hebrew(rtl) build.
2. set the browser to offline state or limit the network connection(for
instance, by firewall)

actual results:
the error document appears LTR.

Expected results:
the document direction should be as the interface direction.
uhu, taking.
Severity: normal → major
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox1.1
Attached patch fix the direction settings (obsolete) — Splinter Review
this patch uses the locale.dir direction in region.dtd
Comment on attachment 188058 [details] [diff] [review]
fix the direction settings

>Index: docshell/resources/content/netError.xhtml
>===================================================================
>
>   %netErrorDTD;
>+  <!ENTITY % globalRegionDTD 
>+       SYSTEM "chrome://global-region/locale/region.dtd">
>+  %globalRegionDTD;
> ]>

Bad dependency, especially for embeddors.


>-      text-align: left;
>+      text-align: start;

no such thing, afaik.
Attachment #188058 - Flags: review-
Assignee: bugs.mano → adamlock
Status: ASSIGNED → NEW
Component: General → Embedding: Docshell
Flags: review-
Product: Firefox → Core
QA Contact: general → adamlock
Target Milestone: Firefox1.1 → mozilla1.8beta3
Assignee: adamlock → bugs.mano
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
should work (untested).
Attachment #188058 - Attachment is obsolete: true
Attachment #188064 - Flags: superreview?(bzbarsky)
Attachment #188064 - Flags: review?(benjamin)
Attachment #188064 - Flags: review?(smontagu)
+<body style="font: message-box;" dir="locale.dir;">

missing &?
er, indeed.
(In reply to comment #3)
> (From update of attachment 188058 [details] [diff] [review] [edit])
> >Index: docshell/resources/content/netError.xhtml
> >===================================================================
> >
> >-      text-align: left;
> >+      text-align: start;
> 
> no such thing, afaik.
> 
seems to work for me...
see also:
http://www.w3.org/TR/2003/CR-css3-text-20030514/#alignment-prop
This is from the CSS3 drafts, do we actaully support it?
(In reply to comment #8)
> This is from the CSS3 drafts, do we actaully support it?
works for me.
(In reply to comment #8)
> This is from the CSS3 drafts, do we actaully support it?

Yes, at least partially. See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsCSSProps.cpp&mark=833#825
(and blame for that line) for example.
Interesting, "text-align: start;" is supported, "text-align: end;" isn't.
Attachment #188064 - Attachment is obsolete: true
Attachment #188064 - Flags: superreview?(bzbarsky)
Attachment #188064 - Flags: review?(smontagu)
Attachment #188064 - Flags: review?(benjamin)
Attached patch v1.1 (obsolete) — Splinter Review
The support for "text-align: start" is poor enough for not using it at all, but
I guess it's OK in this case.
Attachment #188084 - Flags: superreview?(bzbarsky)
Attachment #188084 - Flags: review?(smontagu)
Attachment #188084 - Flags: review?(benjamin)
same patch as above, using text-align:start
Attached patch minor alignment fix (obsolete) — Splinter Review
fix the alignment in text-align
Attachment #188085 - Attachment is obsolete: true
sorry for the spam... i didn't notice the new patch...
Comment on attachment 188084 [details] [diff] [review]
v1.1

Why do we need *another* locale.dir entity? Please use the one that already
exists in region.dtd.
Attachment #188084 - Flags: superreview?(bzbarsky)
Attachment #188084 - Flags: review?(smontagu)
Attachment #188084 - Flags: review?(benjamin)
Attachment #188084 - Flags: review-
bsmedberg: some embeddors (including Camino) don't have
chrome://global-region/locale/region.dtd at all. Also, the xpfe version of this
entiries file doens't have a locale.dir entity.
Then please fix that, instead of duplicating things.
I'm not sure if making docshell/ dependent on entities from either xpfe/ or
toolkit/ is a good idea (we didn't, so far. regoin.dtd isn't used outside of
xpfe/, toolkit/, mail/, calendar/ and browser).
docshell must not depend on either xpfe or toolkit.  There are existing docshell
localization files that should be used.  If the problem is that an entry is in a
too-high-level properties file, it should move down as needed.
(In reply to comment #20)
>If the problem is that an entry is in a
> too-high-level properties file, it should move down as needed.

so what's the bottom line, are we adding a new entity or using the one in
region.dtd?

> There are existing docshell localization files that should be used.

the only file i can think of is netError.dtd.
Reuven: I'm probably going to add a new entities file to dom/, you could ask me
befroe commenting here.
I would love to add a chrome://global/locale/global.dtd and put this in there.
Attached patch v2Splinter Review
Attachment #188084 - Attachment is obsolete: true
Attachment #188086 - Attachment is obsolete: true
Attachment #188318 - Flags: review?(benjamin)
Attachment #188318 - Flags: superreview?(bzbarsky)
Attachment #188318 - Flags: review?(benjamin)
Attachment #188318 - Flags: review+
It's likely to be a few days before I can look at this...
Flags: blocking1.8b4+
Whiteboard: [affects l10n]
we probably want this in ASAP, to avoid l10n tinderbox builds from turning red
in the last moment.
Attachment #188318 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 188318 [details] [diff] [review]
v2

note that the css change was a bit wrong (unlike the previous patches, i've
changed the wrong 'text-align' rule) I'll fix that on checkin.
Attachment #188318 - Flags: approval1.8b4?
Attachment #188318 - Flags: approval1.8b4? → approval1.8b4+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta4
Blocks: branching1.8
Blocks: 300861
No longer blocks: 300861
Status: RESOLVED → VERIFIED
Blocks: 306980
Blocks: 300891
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: