300270 - GetDir() always returns empty string

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Simon Montagu :smontagu]

The testcase in the URL field should alert "foo.dir=rtl", and does so in 1.7.8.
In trunk it alerts "foo.dir=".

I believe this is regression from bug 236476.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

One-line fix

Comment 2

[Simon Montagu :smontagu]

This will break half the bidi-enabled web apps in the world (all those that use
the dir attribute on specific elements instead of document.dir or style).

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there a reason not to let GetDir return 'somevalue' for markup like

<div dir='somevalue'>

?

If that is ok you can just replace the entire function with

return GetAttrHelper(nsHTMLAtoms::dir, aValue)

Comment 4

[Simon Montagu :smontagu]

The trouble with returning 'somevalue' is that typically applications will do
something like

if (foo.dir == 'ltr' || 
    foo.dir == '') // no value set, default to ltr
  // do ltr stuff
else
  // do rtl stuff

so the 'somevalue' case will be treated as rtl. The smart thing to do is

if (foo.dir != 'rtl')
  // do ltr stuff
else
  // do rtl stuff

but I haven't seen that very often.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's only a problem if the markup actually contains 'somevalue' though.
Reading the bug that regressed this it seems like IE does not return
'somevalue', though for the sake of consistency i'd think it'd be nice to do
that. Unless it's a problem on realworld sites of course.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 188842 [details] [diff] [review]
Patch

r=me if you really do think that sites would break if we returned 'somevalue'.
But i'd prefer to do comment 3

Comment 7

[Asa Dotzler [:asa]]

This is gonna need to make b4 if it's gonna make 1.1 so the 1.1 nomination is
redundant.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 188842 [details] [diff] [review]
Patch

sr=jst

Comment 9

[Mike Shaver (:shaver emeritus)]

Can we get this checked in on trunk to bake, then nominated for the branch?

Comment 10

[Simon Montagu :smontagu]

Checked in to trunk.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 188842 [details] [diff] [review]
Patch

See comment 2. This is a serious regression for bidi web apps.

Comment 12

[Asaf Romano (gone)]

Checking in nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <-- 
nsGenericHTMLElement.cpp
new revision: 1.596.2.2; previous revision: 1.596.2.1
done

Comment 13

[Hixie (not reading bugmail)]

I've now specced this in HTML5 to match IE:
   http://www.whatwg.org/specs/web-apps/current-work/#dir

See the last paragraph (and follow the links therein).