Last Comment Bug 313164 - Bidi: Caret navigation is broken inside inline elements
: Bidi: Caret navigation is broken inside inline elements
Status: RESOLVED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8rc1
Assigned To: Uri Bernstein (Google)
:
:
Mentors:
Depends on:
Blocks: 288789
  Show dependency treegraph
 
Reported: 2005-10-20 10:11 PDT by Uri Bernstein (Google)
Modified: 2008-07-31 02:44 PDT (History)
4 users (show)
asa: blocking1.8rc1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (582 bytes, text/html)
2005-10-20 10:23 PDT, Uri Bernstein (Google)
no flags Details
patch (4.01 KB, patch)
2005-10-20 10:29 PDT, Uri Bernstein (Google)
smontagu: review+
roc: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Uri Bernstein (Google) 2005-10-20 10:11:30 PDT
When I fixed bug 288789 by changing the semantics of
nsFrameList::GetPrev[Next]VisualFor(), I neglected to change the case where the
parent frame is not a block frame (i.e. it's an inline frame). This has no
effect on textareas (which can't contain inlines), but it does break HTML
editing, keyboard-selection, and caret navigation when bidirectional text is
contained within an inline element.

I'll attach a testcase and a patch soon.
Comment 1 Uri Bernstein (Google) 2005-10-20 10:23:51 PDT
Created attachment 200231 [details]
testcase

In Composer, or Caret Browsing mode:

In the upper paragraph, try moving the caret in and out of the numbers (in the
1st and 3rd lines). Notice that the caret moves to all kinds of unexpected
places.

The lower paragraph (which is idetical except it is not wrapping a <span>)
works correctly.
Comment 2 Uri Bernstein (Google) 2005-10-20 10:29:03 PDT
Created attachment 200233 [details] [diff] [review]
patch

In the case where the parent is not a block frame, use the direction-aware
nsFrameOrigin (with a dummy value of "0" for the line number), instead of just
comparing the x coordinates (disregarding direction).

Simon - do you think you can review this soon so we will have some sort of
chance of getting this into 1.8? (I'm not too optimistic, but let's try).
Comment 3 Simon Montagu :smontagu 2005-10-20 15:51:53 PDT
Comment on attachment 200233 [details] [diff] [review]
patch

r=smontagu
Comment 4 Uri Bernstein (Google) 2005-10-20 16:12:46 PDT
Comment on attachment 200233 [details] [diff] [review]
patch

Thanks, Simon!

ROC - same request here. This is a regression, and I'd really like to try and
slip the fix into 1.5.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-10-20 18:18:11 PDT
Comment on attachment 200233 [details] [diff] [review]
patch

looks good ... it's pretty clear that nothing has changed for LTR too
Comment 6 Uri Bernstein (Google) 2005-10-21 01:38:59 PDT
Comment on attachment 200233 [details] [diff] [review]
patch

Asking for last-minute approval1.8rc1. The changed code is only reached when
navigating in bidi HTML, so any risk is limited to that area (which is pretty
broken without this fix).
Comment 7 Asa Dotzler [:asa] 2005-10-21 11:36:01 PDT
not going to flag this as a blocker but if it lands before the freeze, then it's
in :-)
Comment 8 Uri Bernstein (Google) 2005-10-21 13:16:02 PDT
Checked in by timeless to both trunk and branch, 2005-10-21, 13:06.

Note You need to log in before you can comment on or make changes to this bug.