Closed Bug 629172 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- final+

People

(Reporter: tomer, Assigned: ehsan)

References

()

Details

(Keywords: regression, rtl, Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → ehsan
blocking2.0: --- → ?
Component: General → Editor
Keywords: regression
QA Contact: general → editor
This is a regression from bug 581536.  We reconstruct the frame, and we lose the internal directionality while doing that.
Blocks: 581536
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #507357 - Flags: review?(roc)
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.
(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.
+    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()?
(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?
(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?
Seems like surely we could at least have nsEditor::UpdateDirAttribute(Element* aElement), which sets aElements' dir attribute according to the editor's mFlags.
(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)?
No, that's synchronous.

Just go with comment #8?
Attached patch Patch (v2)Splinter Review
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...
Attachment #507357 - Attachment is obsolete: true
Attachment #507357 - Flags: review?(roc)
Attachment #507612 - Attachment description: WIP → Patch (v2)
Attachment #507612 - Flags: review?(roc)
Whiteboard: [hardblocker] → [hardblocker][needs landing]
http://hg.mozilla.org/mozilla-central/rev/3de1641119d1
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [hardblocker][needs landing] → [hardblocker]
Target Milestone: --- → mozilla2.0b11
Duplicate of this bug: 630740
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.