629172 - [bidi] Switch direction not working in Firefox 4 beta 10

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Tomer Cohen :tomer]

ctrl-shift-x is the keyboard shortcut to switch direction, and it has visible context menu for RTL locales. Since beta 9 something went wrong, and it is now impossible to switch page direction. 

Steps to reproduce: 
a. Type the following in the browser location bar – data:text/html,<textarea>
b. Focus the textarea, try to switch the text direction using ctrl-shift-x

Expected result: 
Text should be aligned to the right, and direction should be changed to RTL/LTR. 

Actual result:
Up to beta 9 (including every version up to 3.6) it was working well, on beta 10 nothing happening. 

Reproduced on Linux and Windows, also reproducing on recent nightly. 

Thanks to Amir for spotting this issue.

Comment 1

[(no longer active)]

This is a regression from bug 581536.  We reconstruct the frame, and we lose the internal directionality while doing that.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like the code that sets the root node direction is duplicated. Would be slightly simpler to nsEditor that syncs the dir attribute to the flag value.

Comment 4

[(no longer active)]

(In reply to comment #3)
> Seems like the code that sets the root node direction is duplicated. Would be
> slightly simpler to nsEditor that syncs the dir attribute to the flag value.

Nothing is duplicated, as far as I can tell.  Let me describe how the patch works:

nsTextEditorState::BindToFrame sets the dir attribute on the div *before* passing it to the CSS frame constructor.  It uses the RTL flag stored on the editor (which survives reframes).

nsEditor::Init sets the *initial* value of the RTL flag on the editor based on the directionality of the root frame.  This makes sure that things like <textarea dir=rtl> end up with the RTL flag set on the editor when it's initialized.

nsEditor::SwitchTextDirection flips the internal value of the flag, and sets the dir attribute on the anonymous div.  This will reframe the text control, and the dir attribute will be set on the new anonymous div which will be created later on using nsTextEditorState::BindToFrame.

This is kind of confusing, but I hope this description makes things easier to follow.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    rv = mEditor->GetFlags(&flags);
+    NS_ENSURE_SUCCESS(rv, rv);
+    nsAutoString direction;
+    if (flags & nsIPlaintextEditor::eEditorRightToLeft) {
+      direction.AssignLiteral("rtl");
+    } else {
+      direction.AssignLiteral("ltr");
+    }
+    rootNode->SetAttr(kNameSpaceID_None, nsGkAtoms::dir, direction, PR_FALSE);




   // Apply the opposite direction
-  if (frame->GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL)
+  if (mFlags & nsIPlaintextEditor::eEditorRightToLeft) {
+    mFlags &= ~nsIPlaintextEditor::eEditorRightToLeft;
     rv = rootElement->SetAttribute(NS_LITERAL_STRING("dir"), NS_LITERAL_STRING("ltr"));
-  else
+  } else {
+    mFlags |= nsIPlaintextEditor::eEditorRightToLeft;
     rv = rootElement->SetAttribute(NS_LITERAL_STRING("dir"), NS_LITERAL_STRING("rtl"));
+  }


Aren't these setting the attribute on the same node? Can't we have a helper function nsEditor::UpdateDirAttributeOnRoot()?

Comment 6

[(no longer active)]

(In reply to comment #5)
> +    rv = mEditor->GetFlags(&flags);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsAutoString direction;
> +    if (flags & nsIPlaintextEditor::eEditorRightToLeft) {
> +      direction.AssignLiteral("rtl");
> +    } else {
> +      direction.AssignLiteral("ltr");
> +    }
> +    rootNode->SetAttr(kNameSpaceID_None, nsGkAtoms::dir, direction, PR_FALSE);
> 
> 
> 
>    // Apply the opposite direction
> -  if (frame->GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL)
> +  if (mFlags & nsIPlaintextEditor::eEditorRightToLeft) {
> +    mFlags &= ~nsIPlaintextEditor::eEditorRightToLeft;
>      rv = rootElement->SetAttribute(NS_LITERAL_STRING("dir"),
> NS_LITERAL_STRING("ltr"));
> -  else
> +  } else {
> +    mFlags |= nsIPlaintextEditor::eEditorRightToLeft;
>      rv = rootElement->SetAttribute(NS_LITERAL_STRING("dir"),
> NS_LITERAL_STRING("rtl"));
> +  }
> 
> Aren't these setting the attribute on the same node? Can't we have a helper
> function nsEditor::UpdateDirAttributeOnRoot()?

No, they're not setting the attribute on the same node.  The latter's only purpose is to reconstruct the frame, so that the former code kicks in.  (Note that at the first call site, we don't have a frame yet, so there would be no frame reconstruction following that).

Or do you mean that I should add something like nsContentUtils::SetDirOnNode(nsIContent* aNode, const char* dir) and call it from both places?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> No, they're not setting the attribute on the same node.

They're both setting the attribute on the anonymous div, right?

> The latter's only purpose is to reconstruct the frame, so that the former code
> kicks in.

Really? Why don't we just restyle it with a frame reconstruction hint, then?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like surely we could at least have nsEditor::UpdateDirAttribute(Element* aElement), which sets aElements' dir attribute according to the editor's mFlags.

Comment 9

[(no longer active)]

(In reply to comment #7)
> (In reply to comment #6)
> > No, they're not setting the attribute on the same node.
> 
> They're both setting the attribute on the anonymous div, right?

Yes, but the anonymous div on the second call would get destroyed soon after, and we'd get a new anonymous div (the one on the first call site).

> > The latter's only purpose is to reconstruct the frame, so that the former code
> > kicks in.
> 
> Really? Why don't we just restyle it with a frame reconstruction hint, then?

By calling nsIPresShell::RecreateFramesFor (or something like that)?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, that's synchronous.

Just go with comment #8?

Comment 11

[(no longer active)]

Attachment: Patch (v2)

The previous patch was native in that it didn't differentiate between the direction set on the content node and the editor's internal direction as set by the user.  We actually need to mutually exclusive flags.  The absence of both of them means that no internal direction has been set...

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3de1641119d1

Comment 14

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre