Closed Bug 299480 Opened 20 years ago Closed 20 years ago

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

Categories

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

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: linxspider, Assigned: asaf)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 5 obsolete files)

v2
6.44 KB, patch
benjamin
: review+
bzbarsky
: superreview+
benjamin
: approval1.8b4+
Details | Diff | Splinter Review
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 v2 — — Splinter 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: 20 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: