Last Comment Bug 151407 - BiDi: Setting document.dir has no effect on <html dir="rtl">
: BiDi: Setting document.dir has no effect on <html dir="rtl">
Status: RESOLVED FIXED
[oracle-nls]
: dev-doc-needed, rtl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla23
Assigned To: Simon Montagu :smontagu
:
Mentors:
: 256470 (view as bug list)
Depends on: 300270
Blocks: 312023
  Show dependency treegraph
 
Reported: 2002-06-12 19:59 PDT by Kyae-Young Kim
Modified: 2016-02-21 06:01 PST (History)
13 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case of 151407 (1.08 KB, text/html)
2002-06-12 20:00 PDT, Kyae-Young Kim
no flags Details
Patch (23.44 KB, patch)
2013-04-21 08:23 PDT, Simon Montagu :smontagu
ehsan: review+
Details | Diff | Splinter Review
Changes to existing tests (2.26 KB, patch)
2013-04-21 08:24 PDT, Simon Montagu :smontagu
Ms2ger: review+
Details | Diff | Splinter Review
GetDir on root elelement (1.48 KB, patch)
2013-04-22 22:57 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review

Description Kyae-Young Kim 2002-06-12 19:59:20 PDT
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
Comment 1 Kyae-Young Kim 2002-06-12 20:00:11 PDT
Created attachment 87467 [details]
Test case of 151407
Comment 2 Shoshannah Forbes 2002-06-13 03:16:17 PDT
CONFIRMED with moz 1.0 on win98
Comment 3 Simon Montagu :smontagu 2002-06-14 12:24:46 PDT
Adding regression keyword. I remember seeing something just like this working in
the past.
Comment 4 Simon Montagu :smontagu 2002-06-21 10:31:32 PDT
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
Comment 5 Uri Bernstein (Google) 2005-07-06 12:59:10 PDT
Changing summary to be more informative; removing "regression" keyword per
comment #4.
Comment 6 Simon Montagu :smontagu 2005-07-10 05:18:56 PDT
(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>.
Comment 7 Simon Montagu :smontagu 2005-07-11 02:32:33 PDT
The same is true in reverse: <html dir="rtl"> does not set document.dir to rtl
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-08-25 00:04:40 PDT
*** Bug 256470 has been marked as a duplicate of this bug. ***
Comment 9 :Ehsan Akhgari 2008-04-07 13:57:22 PDT
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Comment 10 Neil Fraser 2013-03-24 16:17:11 PDT
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
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-03-24 18:16:30 PDT
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.
Comment 12 Neil Fraser 2013-03-24 18:33:55 PDT
A workaround is to get or set:
  document.body.parentNode.dir
instead of:
  document.dir
Comment 13 :Ehsan Akhgari 2013-03-25 08:08:56 PDT
(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.
Comment 14 Simon Montagu :smontagu 2013-04-21 08:23:33 PDT
Created attachment 740091 [details] [diff] [review]
Patch

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".
Comment 15 Simon Montagu :smontagu 2013-04-21 08:24:57 PDT
Created attachment 740092 [details] [diff] [review]
Changes to existing tests
Comment 16 :Ehsan Akhgari 2013-04-21 10:15:55 PDT
Comment on attachment 740091 [details] [diff] [review]
Patch

Review of attachment 740091 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2013-04-21 23:47:36 PDT
Comment on attachment 740092 [details] [diff] [review]
Changes to existing tests

Review of attachment 740092 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2013-04-21 23:51:15 PDT
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
Comment 20 Boris Zbarsky [:bz] 2013-04-22 22:26:01 PDT
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?
Comment 21 Simon Montagu :smontagu 2013-04-22 22:36:32 PDT
only that Element doesn't have GetDir
Comment 22 Simon Montagu :smontagu 2013-04-22 22:57:03 PDT
Created attachment 740656 [details] [diff] [review]
GetDir on root elelement

would this have been better?
Comment 23 :Ehsan Akhgari 2013-04-22 23:30:52 PDT
(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?
Comment 24 Simon Montagu :smontagu 2013-04-22 23:51:04 PDT
We know that it's an <html> element. Is that equivalent?
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2013-04-23 01:12:39 PDT
(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.
Comment 27 Boris Zbarsky [:bz] 2013-04-23 04:59:40 PDT
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.
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-04-23 12:10:32 PDT
https://hg.mozilla.org/mozilla-central/rev/7c44612d9cea
Comment 30 j.j. 2013-04-25 13:31:11 PDT
needs documentation at least here
 https://developer.mozilla.org/en-US/docs/DOM/Document.dir
and probably elsewhere

Note You need to log in before you can comment on or make changes to this bug.