Closed Bug 202046 Opened 22 years ago Closed 21 years ago

Empty (inline) elements cause caret movement to fail

Categories

(Core :: DOM: Selection, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: carosendahl, Assigned: KaiE)

Details

(Keywords: topembed+, Whiteboard: editorbase+)

Attachments

(3 files, 2 obsolete files)

Attaching two test cases.  Arrow around in both of them in the editor to see
lovely behaviour.
Attached file Test 1
Attached file Test 2
Whiteboard: editorbase
using Test 2, i see this problem on all platforms, using 2003.04.23 builds.
nominating.

michael, is this similar to the problem you were seeing on nscp aim (nim)?
Keywords: nsbeta1, topembed
OS: Windows 2000 → All
Hardware: PC → All
The IM problem I saw is resolved with todays build.
Keywords: topembedtopembed+
ah, then i think the IM problem you had --which is fixed with today's build--
was prolly due to bug 202834 (fixed yesterday).

i don't think this particular bug is recent regression, as it occurred with a
build from 4/1.

charles, do you recall if this ever worked for you?
editorbase+
Keywords: nsbeta1nsbeta1+
Whiteboard: editorbase → editorbase+
Test case 2 did not work for me until the latest builds.

Also, test case 1:  right arrow to a hard stop.  Press the end key and left
arrow infinitely.
Kaie -- Can you update the target Milestone to indicate when you will be able to
land a fix?
I would like to try to get this into 1.4 final.
Setting target milestone to 1.4 beta, because I don't see a 1.4 final milestone.
Priority: -- → P1
Target Milestone: --- → mozilla1.4beta
Target Milestone: mozilla1.4beta → mozilla1.4final
QA Contact: sairuh → beppe
Minimal test case:

<body>
a
<any></any>
b
</body>

Start at a, arrow around using left-right keys, and you'll never reach b.

Same behaviour seen with <font></font> and <span></span> and <small></small>.


-> Selection.
Component: Editor: Core → Selection
Summary: Arrow key misbehaviour → Empty (inline) elements cause caret movement to fail
Blocks: 205412
Moving target milestone, it's unlikely this will make it for 1.4 final.
Target Milestone: mozilla1.4final → mozilla1.5beta
In 
  nsFrame::GetFrameFromDirection() 
the test for 
  if (testRect.IsEmpty()) 
is true.
The comment besides that test says:
  // this must be a non-renderable frame creatd
  // at the end of the line by Bidi reordering

In our test cases, there isn't such a thing, it seems it is incorrect to make
this conclusion, that seeing such an empty frame is an indicator for "Bidi
reordering".

I guess our empty tag is producing the empty frame?


Since in our test case we don't have any Bidi text, I comment out the action
that is done when the above test succeeds.

Commenting that out fixes the testcase!
Simon, please see my previous comment.

The code in nsFrame::GetFrameFromDirection() seems to make an assumption based
on emptiness of a frame, which doesn't seem to be always correct.
The mentioned test was checked in in the context of bug 71370.
Attached patch Patch v1 (obsolete) — Splinter Review
Thanks to Simon, who helped me to come up with this patch.

It fixes the reported test cases for me, and it does not seem to cause a
regression in a Bidi document Simon gave me.
Attachment #125767 - Flags: review?(smontagu)
Comment on attachment 125767 [details] [diff] [review]
Patch v1

sr=kin@netscape.com
Attachment #125767 - Flags: superreview+
Comment on attachment 125767 [details] [diff] [review]
Patch v1

Nits: while you're here, can you make the brace positioning conform to local
style in 
+    if (testRect.IsEmpty()) {
and change the comment from "this must be..." to "If the rectangle is empty and
the NS_FRAME_IS_BIDI flag is set, this is most likely..."

With those changes, r=smontagu
Attachment #125767 - Flags: review?(smontagu) → review+
Attached patch Patch v1b (obsolete) — Splinter Review
Updated patch as requested.
Attachment #125767 - Attachment is obsolete: true
Comment on attachment 125780 [details] [diff] [review]
Patch v1b

carrying forward review stamps
Attachment #125780 - Flags: superreview+
Attachment #125780 - Flags: review+
Comment on attachment 125780 [details] [diff] [review]
Patch v1b

Requesting 1.4 branch approval for this caret movement correctness fix.
Attachment #125780 - Flags: approval1.4?
Comment on attachment 125780 [details] [diff] [review]
Patch v1b

>@@ -4349,9 +4349,15 @@
>       continue;  //we should NOT be getting stuck on the same piece of content on the same line. skip to next line.
>   }
>   newFrame->GetRect(testRect);
>-  if (testRect.IsEmpty()) { // this must be a non-renderable frame creatd at the end of the line by Bidi reordering
>-    lineJump = PR_TRUE;
>-    aPos->mAmount = eSelectNoAmount;
>+  if (mState & NS_FRAME_IS_BIDI)
>+  {
>+    if (testRect.IsEmpty())
>+    {
>+      // If the rectangle is empty and the NS_FRAME_IS_BIDI flag is set, this is most likely 
>+      // a non-renderable frame created at the end of the line by Bidi reordering.
>+      lineJump = PR_TRUE;
>+      aPos->mAmount = eSelectNoAmount;
>+    }
>   }
>   PRBool newLineIsRTL = PR_FALSE;
>   if (lineJump) {

Why the gratuitous brace style changes, and the over-indented if-if?  The diff
would be minimal if you used && instead of nested ifs:

  // This must be a non-renderable frame created at the end of the line by Bidi
  // reordering.
  if (testRect.IsEmpty() && (mState & NS_FRAME_IS_BIDI)) {
    // This is most likely a non-renderable frame created at the end of the
line by
    // Bidi reordering.
    lineJump = PR_TRUE;
    aPos->mAmount = eSelectNoAmount;
  }

/be
Attached patch Patch v1cSplinter Review
Updated patch to address Brendan's comment.
Attachment #125780 - Attachment is obsolete: true
Attachment #125780 - Flags: approval1.4?
Comment on attachment 125860 [details] [diff] [review]
Patch v1c

Carrying forward reviews.
Attachment #125860 - Flags: superreview+
Attachment #125860 - Flags: review+
Attachment #125860 - Flags: approval1.4?
fixed on trunk
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
No longer blocks: 205412
Comment on attachment 125860 [details] [diff] [review]
Patch v1c

moving approval request forward.
Attachment #125860 - Flags: approval1.4? → approval1.4.x?
This caused regression bug 210110
Comment on attachment 125860 [details] [diff] [review]
Patch v1c

this patch caused regression bug 210110
Attachment #125860 - Flags: approval1.4.x?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: