Closed Bug 151407 Opened 23 years ago Closed 12 years ago

BiDi: Setting document.dir has no effect on <html dir="rtl">

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: kyae-young.kim, Assigned: smontagu)

References

Details

(Keywords: dev-doc-needed, rtl, Whiteboard: [oracle-nls])

Attachments

(4 files)

Test case of 151407
1.08 KB, text/html
Details
Patch
23.44 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Changes to existing tests
2.26 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
GetDir on root elelement
1.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Env. : Mozilla 1.0, Windows NT/2000, Des. : The LTR direction of windows does not support Reproducible step: 1) Load test case in IE 5.5 2) Click the Window Direction button 3) Window will be fliped for LTR direction 4) It does not support in Mozilla 1.0
Attached file Test case of 151407 —
CONFIRMED with moz 1.0 on win98
Status: UNCONFIRMED → NEW
Ever confirmed: true
Adding regression keyword. I remember seeing something just like this working in the past.
Keywords: regression
The page that I was thinking of was http://www.qsm.co.il/Hebrew/HebrewTest/rtl.htm, which still works. The significant difference seems to be that that page uses document.dir while the attachment here uses top.document.dir
Whiteboard: [oracle-nls]
Changing summary to be more informative; removing "regression" keyword per comment #4.
Keywords: regression
Summary: Bidi : The LTR direction of windows does not support → BiDi: Setting top.document.dir has no effect
(In reply to comment #4) > The page that I was thinking of was http://www.qsm.co.il/Hebrew/HebrewTest/rtl.htm, > which still works. The significant difference seems to be that that page uses > document.dir while the attachment here uses top.document.dir That was wrong. The significant difference is that the attachment here has <HTML dir="rtl"> and the page at www.qsm.co.il just has <HTML>.
Depends on: 300270
Summary: BiDi: Setting top.document.dir has no effect → BiDi: Setting document.dir has no effect on <html dir="rtl">
The same is true in reverse: <html dir="rtl"> does not set document.dir to rtl
*** Bug 256470 has been marked as a duplicate of this bug. ***
Blocks: 312023
OS: Windows NT → All
Hardware: PC → All
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
Assignee: mozilla → nobody
This bug is now over a decade old. Confirming error on OS X in Firefox. The behaviour has become so well-known that it has now reached the documentation: https://developer.mozilla.org/en-US/docs/DOM/Document.dir
http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#dom-document-dir says: The dir IDL attribute on Document objects must reflect the dir content attribute of the html element, if any, limited to only known values. If there is no such element, then the attribute must return the empty string and do nothing on setting.
A workaround is to get or set: document.body.parentNode.dir instead of: document.dir
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #11) > http://www.whatwg.org/specs/web-apps/current-work/multipage/elements. > html#dom-document-dir says: > > The dir IDL attribute on Document objects must reflect the dir content > attribute of the html element, if any, limited to only known values. If > there is no such element, then the attribute must return the empty string > and do nothing on setting. Hmm, given that, this is a valid bug, right? The problem is that nsIDocument::SetDir just doesn't set the attribute on the root element, it just uses SetDirectionality on it.
Component: Layout: Text → DOM: Core & HTML
Attached patch Patch — — Splinter Review
I believe this makes our behaviour compatible with the HTML5 spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#dom-dir The patch removes the directionality property on nsIDocument, which is now redundant, and makes the dir IDL attribute on documents reflect the dir attribute of the html element, rather than the root element (this makes an existing test fail, see following patch). It also stops using the bidi.direction config option to determine the default direction, in response to various comments in https://www.w3.org/Bugs/Public/show_bug.cgi?id=18340. I'll remove other uses of option in a follow-up bug. Note that document.dir can now return four possible values: "auto", "ltr", "rtl", or the empty string. Previously it would always return either "ltr" or "rtl".
Assignee: nobody → smontagu
Attachment #740091 - Flags: review?(ehsan)
Attachment #740092 - Flags: review?(Ms2ger)
Comment on attachment 740091 [details] [diff] [review] Patch Review of attachment 740091 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #740091 - Flags: review?(ehsan) → review+
Comment on attachment 740092 [details] [diff] [review] Changes to existing tests Review of attachment 740092 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #740092 - Flags: review?(Ms2ger) → review+
Comment on attachment 740091 [details] [diff] [review] Patch Review of attachment 740091 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +6253,5 @@ > return NS_OK; > } > > void > +nsIDocument::GetDir(nsAString& aDirection) I'd prefer making GetHtmlElement() const. @@ +6283,5 @@ > return rv.ErrorCode(); > } > > void > nsIDocument::SetDir(const nsAString& aDirection, ErrorResult& rv) Drop the ErrorResult here and the [SetterThrows] from Document.webidl
Was there a reason to not just call GetDir on the root element instead of doing a GetAttr and then comparing to a copy of the dir enum table?
only that Element doesn't have GetDir
would this have been better?
Attachment #740656 - Flags: review?(bzbarsky)
(In reply to comment #22) > Created attachment 740656 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=740656&action=edit > GetDir on root elelement > > would this have been better? Do we know that the root element is always going to be an HTML element?
We know that it's an <html> element. Is that equivalent?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #23) > (In reply to comment #22) > > Created attachment 740656 [details] [diff] [review] > > --> https://bugzilla.mozilla.org/attachment.cgi?id=740656&action=edit > > GetDir on root elelement > > > > would this have been better? > > Do we know that the root element is always going to be an HTML element? We know that it's an html element in the HTML namespace, so, yes. I guess we could make GetHtmlElement() return something more specific, but I don't know if all callers want to include nsGenericHTMLElement.h.
https://hg.mozilla.org/mozilla-central/rev/f3165dffa51c https://hg.mozilla.org/mozilla-central/rev/b17e0eb827c0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 740656 [details] [diff] [review] GetDir on root elelement Yes, perfect. Thank you! And yes, GetHtmlElement always returns an <html:html> or null, so this is safe.
Attachment #740656 - Flags: review?(bzbarsky) → review+
needs documentation at least here https://developer.mozilla.org/en-US/docs/DOM/Document.dir and probably elsewhere
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: