Closed Bug 151407 Opened 22 years ago Closed 11 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: 11 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: