Closed
Bug 277553
Opened 20 years ago
Closed 19 years ago
clicking on Textarea with a RTL direction doesnt work
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: contact, Assigned: uriber)
References
()
Details
(Keywords: rtl)
Attachments
(3 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Well
the problem is with rtl textareas, I am working with hebrew,
when I click on the left side of the textarea, after the end of the line, the
text indicator should go to the end of that line, that is working
fine on the first line,
BUT when I go down (pressing enter) and want to come back to the end of the line
of the above line.
exmpale: I want to go back from here to "on the first line," text mentioned
above, but the indicator is stuck unless I go to the last character in that
line, that is really making me insane sometimes when writing in a public forums
in Hebrew,
I hope you will find a solution
Reproducible: Always
Steps to Reproduce:
1.go to a rtl textarea (or make it rtl by right clicking and selecting switch
text direction)
2. write some words
3. press enter
4. write some more words
5. click on the left side of the words you wrote in step no 2
Actual Results:
Now you can see that the indicator is not going up
Expected Results:
the text indicator was supose to go up like it does when the dir="ltr"
Comment 1•20 years ago
|
||
Confirming bug, 2005-01-06-05 trunk Linux.
Assignee: firefox → nobody
Status: UNCONFIRMED → NEW
Component: General → Layout: Form Controls
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout.form-controls
Version: unspecified → Trunk
Comment 2•20 years ago
|
||
*** Bug 277555 has been marked as a duplicate of this bug. ***
Comment 3•20 years ago
|
||
It'd be nice to have a minimal testcase here....
Comment 4•20 years ago
|
||
Comment 5•20 years ago
|
||
Is this a problem with just textareas, or also with editor in general?
Reporter | ||
Comment 6•20 years ago
|
||
(In reply to comment #5)
> Is this a problem with just textareas, or also with editor in general?
what do you mean by editor?
like when I right click and select view source?
in that case there is no rtl option in that editor.
or forms in general?
all other forms are seems to be ok
Comment 7•20 years ago
|
||
No, I mean editor. Composer, mailcompose, contentEditable iframes, etc.
Comment 8•20 years ago
|
||
Yes, editor also suffers from the bug, as you can see with this designmode
testcase.
Comment 9•20 years ago
|
||
Over to bidi, but this may be a selection issue...
Component: Layout: Form Controls → Layout: BiDi Hebrew & Arabic
Updated•20 years ago
|
Assignee: nobody → mkaply
QA Contact: layout.form-controls → zach
Assignee | ||
Comment 10•20 years ago
|
||
A simpler, more efficient, and (hopefully) less buggy implementation of
nsLineIterator::FindFrameAt() for the BiDi (or suspected BiDi) case.
Notice that this implementation does not use CheckLineOrder().
In principle, the new code can be used for the no-BiDi case as well. It is only
slightly less efficient (it always scans the entire line), and this could be
probably be taken care of. However, I preferred not to touch the non-BiDi code.
This patch also fixes the bug described in bug 207186 comment 2 (regarding the
behaviour of the up/down arrows at end of line in an RTL textbox).
Assignee | ||
Updated•20 years ago
|
Comment 11•20 years ago
|
||
Uri, I'm almost certain that you intended to ask Simon Montagu to review your
patch, rather than Simon Fraser, am I right?
Prog.
Assignee | ||
Comment 12•20 years ago
|
||
(In reply to comment #11)
> Uri, I'm almost certain that you intended to ask Simon Montagu to review your
> patch, rather than Simon Fraser, am I right?
>
> Prog.
Actually, no. I saw That Simon Fraser reviewed previous patches on this code,
and that in general, this code was maintained mainly by the Editor team, not the
Layout team. However, since I wasn't sure, I also sent an e-mail to Simon Fraser
asking him if he's the right person to review this. No disrespect to Simon M, of
course.
Comment 13•20 years ago
|
||
The patch's code looks kind of general-purpose... what does it have to do with
bidiness anyway?
Assignee | ||
Comment 14•20 years ago
|
||
(In reply to comment #13)
> The patch's code looks kind of general-purpose... what does it have to do with
> bidiness anyway?
Well, for starters, the added code is wrapped in
+#ifdef IBMBIDI
+ if (aCouldBeReordered) {
....
+ }
+#endif
(where aCouldBeReordered is set to aPresContext->BidiEnabled() by the caller).
But if you're asking _why_ I limited my patch to the BiDi case, and not applied
it to all cases, the reason is basically that I didn't want to risk breaking
stuff that is working (the simple LTR case). I figured this will make the patch
less risky, and will give it a better chance getting in for Firefox 1.1.
If I get strong positive feedback from the reviewer (or others who know this
code), I'll be happy to change my mind.
Assignee | ||
Comment 15•20 years ago
|
||
(In reply to comment #10)
> [The new code] is only slightly less efficient (it always scans the entire line)
Correction: it only does this if aX is not within one of the frames (which is a
rare case).
Comment 16•20 years ago
|
||
Comment on attachment 187293 [details] [diff] [review]
patch
I'm not really qualified to review this code. You should ask someone with more
knowledge of the frame hierarchy (dbaron, bz etc).
Attachment #187293 -
Flags: review?(sfraser_bugs)
Assignee | ||
Updated•20 years ago
|
Attachment #187293 -
Flags: review?(dbaron)
Assignee | ||
Comment 17•20 years ago
|
||
Comment on attachment 187293 [details] [diff] [review]
patch
Moving reveiw request to bzbarsky, which is CCed on this bug and seems to have
less on his review plate.
Boris - please let me know if this is not OK with you.
Attachment #187293 -
Flags: review?(dbaron) → review?(bzbarsky)
Comment 18•20 years ago
|
||
I'm still sorting through everything that happened while I was away for a month.
It's likely to be well over a week before I get a chance to even look at this.
Assignee | ||
Comment 19•20 years ago
|
||
(In reply to comment #18)
If there's someone which is qualified to review the patch and you know to be
less busy, please let me know.
Otherwise, I'll wait till you have time.
Comment 20•20 years ago
|
||
OK, so my problem is that I don't really know this code either. dbaron might;
not sure... That said, this patch makes the bidi and non-bidi branches return
different frames if, say, aX < line.bounds.x and mRightToLeft is true. Is that
really intended? That is, what does the mRightToLeft boolean actually mean here?
Comment 21•20 years ago
|
||
Also, I would much prefer a single codepath for both bidi and non-bidi, if
there's not a huge perf difference. That would prevent issues like the one I
just raised, where the codepaths do slightly different things...
Assignee | ||
Comment 22•20 years ago
|
||
(In reply to comment #20)
> OK, so my problem is that I don't really know this code either. dbaron might;
Naturally, I'll be glad to hear comments from dbaron, or smontagu, or anyone
which does understand this code, on the patch in general and on the questions
you raise below specifically. I just got the impression that dbaron is a very
busy person.
> That said, this patch makes the bidi and non-bidi branches return
> different frames if, say, aX < line.bounds.x and mRightToLeft is true. Is that
> really intended?
It's certainly not intended, but I also can't see why it would happen, at least
in the normal case. As far as I can tell, the (logically) last non-zero-width
frame (which, in case mRightToLeft is true, is also the visually leftmost
non-zero-width frame) will be returned in both cases. Please tell me what I'm
missing.
The only case I can think of where a different frame would be returned is if the
positioning of the frames is "artificially" altered - in which case my approach
would be, IMO, better, as it would return the frame which is visually leftmost -
in accordance with the visual orientation of this method.
>That is, what does the mRightToLeft boolean actually mean here?
mRightToLeft, as far as I understand, signifies the directionality of the block
element containing the line (aka the "paragraph direction"). This is the basic
direction in which the frames are layed out.
>Also, I would much prefer a single codepath for both bidi and non-bidi, if
>there's not a huge perf difference. That would prevent issues like the one I
>just raised, where the codepaths do slightly different things...
As I wrote above, the new code _should_ work just fine for the non-bidi case. I
was simply being conservative, trying to make sure that if I broke anything it
would be just the bidi case. But perhaps that's not a good approach. If asked,
I'll be happy to provide a patch where the new code is used for both cases.
Comment 23•20 years ago
|
||
(In reply to comment #22)
> As I wrote above, the new code _should_ work just fine for the non-bidi case. I
> was simply being conservative, trying to make sure that if I broke anything it
> would be just the bidi case. But perhaps that's not a good approach. If asked,
> I'll be happy to provide a patch where the new code is used for both cases.
Please do so. We have way too many IBMBIDI ifdefs already; we don't need more
where they're not needed.
Comment 24•20 years ago
|
||
> in which case my approach would be, IMO, better
Agreed. With your approach, the only thing mRightToLeft affects is the
aXIsBefore/After stuff, which makes sense to me... Let's just use the new code
in both cases, ok? A patch to that effect would be great.
Assignee | ||
Comment 25•20 years ago
|
||
Use the new approach for both bidi and non-bidi cases. This eliminates the need
for the aCouldBeReordered parameter (which was only used here, so I removed it
from the interface and the other implementing class as well).
Attachment #187293 -
Attachment is obsolete: true
Attachment #190473 -
Flags: review?(bzbarsky)
Updated•20 years ago
|
Attachment #187293 -
Flags: review?(bzbarsky)
Comment 26•20 years ago
|
||
Comment on attachment 190473 [details] [diff] [review]
patch v. 2
General question: if aX falls inside several different frames (eg relative
positioning), then what does this method promise to return? Does it depend on
mRightToLeft?
>Index: layout/generic/nsLineBox.cpp
>+ if (nsnull == closestFromLeft ||
!closestFromLeft, please. Similar in other places where we compare to nsnull.
r+sr=bzbarsky with that fixed and if the patch's behavior is correct in light
of the general question above.
Attachment #190473 -
Flags: superreview+
Attachment #190473 -
Flags: review?(bzbarsky)
Attachment #190473 -
Flags: review+
Assignee | ||
Comment 27•20 years ago
|
||
Changed "nsnull ==" to "!" everywhere.
>General question: if aX falls inside several different frames (eg relative
>positioning), then what does this method promise to return? Does it depend on
>mRightToLeft?
In such a case, the first frame (logically) in which aX falls is returned. This
does not depend on mRightToLeft.
Not sure what the procedure for carrying over review flags is, so I'm just
asking for the reviews again.
Attachment #190473 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #190516 -
Flags: superreview?(bzbarsky)
Attachment #190516 -
Flags: review?(bzbarsky)
Comment 28•20 years ago
|
||
Comment on attachment 190516 [details] [diff] [review]
patch v. 2.1
You can usually carry over review flags if the reviewer said you could (which I
sort of did, in this case, but I wasn't clear enough).
Attachment #190516 -
Flags: superreview?(bzbarsky)
Attachment #190516 -
Flags: superreview+
Attachment #190516 -
Flags: review?(bzbarsky)
Attachment #190516 -
Flags: review+
Assignee | ||
Comment 29•20 years ago
|
||
Comment on attachment 190516 [details] [diff] [review]
patch v. 2.1
Requesting approval1.8b4, since this (together with bug 299527) is a pretty
major bug in bidi editing.
Attachment #190516 -
Flags: approval1.8b4?
Updated•20 years ago
|
Attachment #190516 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 30•20 years ago
|
||
I won't be around much until next Tuesday (Aug. 2) to deal with possible
regressions, so I'm not asking anybody to check this in yet. I'll do so as soon
as I'm back.
Assignee | ||
Comment 31•19 years ago
|
||
Patch checked in by timeless at 2005-08-02 14:55.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 32•19 years ago
|
||
Note that Testcase2 is currently unusable due to bug 302885.
Updated•19 years ago
|
Target Milestone: --- → mozilla1.8beta4
Comment 33•17 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in
before you can comment on or make changes to this bug.
Description
•