Closed Bug 233348 Opened 16 years ago Closed 12 years ago

BiDi marker is hidden when the caret is at the edge of a textarea

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzillamozilla, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(12 files, 7 obsolete files)

8.68 KB, image/png
Details
723 bytes, text/html
Details
36.68 KB, image/png
Details
37.24 KB, image/png
Details
45.50 KB, image/png
Details
16.86 KB, image/png
Details
91.06 KB, image/jpeg
Details
135.62 KB, image/jpeg
Details
135.62 KB, image/jpeg
Details
91.23 KB, image/jpeg
Details
6.71 KB, image/png
Details
922 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
If an LTR caret is at the beginning of an RTL textarea, or if an RTL caret is at
the beginning of an LTR textarea, its BiDi marker is hidden by the edge of the
textarea. Typing a single character reveals that the maker was there, just hidden.

This is not a dupe of Bug 98157 which talks about text fields in which the BiDi
marker remains hidden regardless of caret location.

A screenshot that compares this with IE6 is coming.

Prog.
Attached image IE6 vs. Mozilla 1.6
Blocks: BidiCaret
Seeing on OS X => All/All
OS: Windows XP → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
This patch *should* have fixed the problem, and also make textareas and inputs
look a bit better IMO. It actually *does* work for LTR textareas and inputs,
but for RTL textareas it triggeres the horrible effects of bug 304609 (which is
currently Camino-only, because Camino does specify (left) padding for the
anonymous div)
Depends on: 304609
Depends on: 306349
No longer depends on: 304609
Comment on attachment 194089 [details] [diff] [review]
patch

Now that bug 306349 is fixed, this patch can work.
Notice that this affects the appearence of all textareas (not just RTL). Personally I think it's an improvement regardless of the bidi marker isuue, but others might disagree.
Attachment #194089 - Attachment description: patch [not for checkin] → patch
Attachment #194089 - Flags: review?(dbaron)
Assignee: mozilla → uriber
So, there was a good bit of work into making textarea and text input sizing compatible with IE.

The first question is:  for a:
 * textarea
 * input type=text
with:
 * no width specified
 * a width specified using CSS 'width'
 * a width specified using HTML 'size' or 'rows'
does this make the text box wider or the area available for text inside it narrower?  Depending on the answers, I may have some other questions.
(Actually, there was probably more work put into making it compatible with Nav4, and then there was a change of course.)
Attached image screenshot, before & after (obsolete) —
Left is current, right is with the padding (I actually used 3px, not 2px here).
As you can see, in all cases the size of the actual control does not change. The space available for the text inside the control gets smaller.
(I overrode the defaut border because otherwise, at least on Mac, the right edge is very difficult to detect).
Upon closer inspection (this is unrelated to dbaron's questions), it seems that to fully fix this, we need 2px of padding on the left, but 3px on the right - because adding any right padding causes the caret to be displayed one pixel to the right of where it is currently displayed (when to the right of the rightmost character). "Left" and "right" here are absolute terms - regardless of whether the control or text is RTL or LTR.
I'm not sure if this is intentional, or perhaps a bug should be filed for this.
Attached image screenshot from Win/IE (obsolete) —
Screenshot from Win/IE (after fiddling with the fonts a bit, to make them more like in Firefox).
Notice that IE displays the caret overlapping the leftmost pixel of the "a", so it only requires two white pixels to the left of the "a" in order to fit the bidi marker.
Firefox (which displays the caret immediately to the left of the "a", not overlapping it) currently has one white pixel to the left. With the patch, it would have three.
I'm a bit uncomfortable with changes like this, but I'd like to hear opinions from roc and bzbarsky.
How does this affect the sizing of inputs?  For example:

  <input type="text" size="5" style="font-family: monospace">

Will that be sized correctly on the inside (big enough for 5 chars)?

What about:

  <input type="text" style="width: 100px"> 

What will actually be 100px in that case, with this patch?
(In reply to comment #12)
> How does this affect the sizing of inputs?

See comment #7 / comment #8. It might be nice If someone could provide screenshots of the testcase on Windows, with and without the patch.
Ah.  So the size="" case gets broken.  That's really rather unfortunate; could we avoid breaking that somehow?
In other words, what we should do is that the "important" part of the sizing should not change with this patch.  What part is "important" is different for size/cols, width in quirks mode, and width in standards mode.  I suspect as things stand the last two are fine and the size/cols thing needs adjustment.
Comment on attachment 194089 [details] [diff] [review]
patch

OK, I'll look into adjusting size/cols to take padding into account when I have time.
Attachment #194089 - Flags: review?(dbaron)
I think we have some code that nudges the regular caret by one pixel sometimes to ensure that it's visible in a textarea. Could we just nudge by two pixels instead when necessary?
Attached patch patch v2 (obsolete) — Splinter Review
Implementing what was suggested in column #15. Also taking into account comment #9.
Notice that I also take into account vertical padding, if it exists (for ROWS).
Attachment #194089 - Attachment is obsolete: true
Attachment #209014 - Flags: review?(dbaron)
"after" is with patch v2. Notice that the cols/size (and default) controls become wider to adjust for the paddings.
Attachment #206119 - Attachment is obsolete: true
Same thing, in standards mode.
(In reply to comment #17)
> I think we have some code that nudges the regular caret by one pixel sometimes
> to ensure that it's visible in a textarea. Could we just nudge by two pixels
> instead when necessary?

IMHO this would not be the best solution. The caret will overlap the letter in a visually non-appealing way.
Also, I thing adding the paddings is a good thing regardless of this bug. IE has paddings. Safari has (large) paddings. Camino has paddings. Firefox should have paddings as well.
The rendering of <textarea cols="16"> changed (and same for <input size="16"> if it were using a monospace font).  We really shouldn't be showing that part of an 's' there, imo.
(In reply to comment #22)
> The rendering of <textarea cols="16"> changed (and same for <input size="16">
> if it were using a monospace font).  We really shouldn't be showing that part
> of an 's' there, imo.

Yes, I noticed that: it's a result of taking the right padding into account (although the actual right padding is hidden at the end of the line, in this case).
Since taking the padding into account was your idea, how would you resolve this? Should only the left (or right, in RTL) padding be accounted for?
Whatever it takes to get the sizing right.  ;)

In IE, is it really the case that the letters are not flush to the left side but _are_ flush to the right? In other words, is the padding really at the end of all the text and not around the whole "text area"?
(In reply to comment #24)
> In IE, is it really the case that the letters are not flush to the left side
> but _are_ flush to the right? In other words, is the padding really at the end
> of all the text and not around the whole "text area"?

I'm not at a Windows machine right now, but attachment 206802 [details] (comment #10) suggests that this is indeed the case, at least for <input>s.

This screenshot demonstrates that even today, with monospaced font, we don't necessarily allow an integral number of characters on the line in <textarea cols="...">.
This is because after calculating the width based on the font size, we're adding space for a vertical scroll bar. If such a scrollbar is not actually necessary, and the scrollbar width does not fit an integral number of characters, the result is partially-hidden letters.
Yes, textarea has the scrollbar issue.  Compat there is less important than compat in <input type="text">, actually.  There, we need to be compatible to within a few pixels on typical use cases; otherwise page layouts break...

It does look like IE only accounts for the left padding; might be worth checking RTL text in IE.
Attached image IE & Firefox on WinXP
I did some research on text inputs in IE and Firefox on Win XP. This is what I found:
1. For unstyled inputs, IE adds a bluish 1px border, and a 1px padding on the outer frame. We are fully emulating this. In the attached screenshot, the bluish inputs are unstyled, and the red ones are styled with "border:1px solid red;". Notice that specifying a border removes the 1px outer padding both on IE and on Fx. So far - no differences.
2. IE has (in all cases) a 1px horizontal padding on the "inner" frame (i.e. on both sides of the text within the frame). This padding is *not* accounted for when calculating the control's width. We currently don't have such padding. As a result, our inputs allow one extra pixel-column for the text. This can clearly be seen in the screenshot, and is true for both the pixel-width and "size=" cases.
3. IE "nudges" the caret one pixel to the right when it is positioned to the left of the leftmost character in the input field (this is true, in absolute terms, also in RTL controls). We don't do that.
4. In IE, the directionality of the characters surrounding the caret affects the caret positioning. e.g., in the last line of the screenshot, if the rightmost character would have been an LTR character, the caret would have been positioned one pixel to the right of where it is depicted.
5. IE allows the caret and the directional marker to be painted on top of (XORed  with) the padding and border of the outer frame, and even outside it, in the rare case where this is necessary. We currently don't allow that - we crop the caret to the inner frame. Notice that points 1-4 combine to reduce the number of cases where the marker is actually drawn over or across the border.

I think one thing we should do for sure is to achieve comatability with IE on point 2, by adding 1px padding on the inner div on both sides, without affecting the overall control width, even in the "size=" case. This will make us look exactly like IE, ignoring the caret itself. However, it will do little to fix this bug, as it would allow only 1px of the directional marker to show on the left side, and none of it on the right (which is the more important side).

We can also implement point 3, although, again, this will only help this bug on the left side (I'm also not quite sure why IE does this).

Point 4 needs further investigation, which I'll try to do. Fixing it (assuming there's something to fix) would help us with 1px on the right side.

Following IE on point 5 would, of course, be the ultimate solution to this bug. mrbkap - can you comment on this? How does your work on bug 287813 affect our ability to do this?
Attachment #206802 - Attachment is obsolete: true
Attachment #209014 - Attachment is obsolete: true
Attachment #209014 - Flags: review?(dbaron)
Depends on: 326123
I submitted bug 326123 for the issue in point #2 of comment #28 (adding a 1px padding).
Assignee: uriber → nobody
QA Contact: zach → layout.bidi
I'm trying to implement method 4 since method 5 seems to be more difficult to implement in the current code. Any ideas?
Attached patch patch v1.0 (obsolete) — Splinter Review
Hi,
  please find the patch attached. I moved caret hook direction detection upward to make it possible to detect hook prblem on the corners.
Attachment #225206 - Flags: review?(uriber)
Comment on attachment 225206 [details] [diff] [review]
patch v1.0

Sorry, but I'm not qualified to review this code. Try mrbkap, perhaps.
Attachment #225206 - Flags: review?(uriber)
Attachment #225206 - Flags: review?(mrbkap)
Comment on attachment 225206 [details] [diff] [review]
patch v1.0

Does this patch bump the caret back over thin characters, like 't' and 'i'?
(In reply to comment #33)
> (From update of attachment 225206 [details] [diff] [review] [edit])
> Does this patch bump the caret back over thin characters, like 't' and 'i'?

This patch does not look on the underlaying text. It just avoids the caret to be too close to the right/left when it has bidi hook. 

(In reply to comment #34)
> This patch does not look on the underlaying text.

I'm not sure what you mean here. The code doesn't look at the text, no, but by moving the caret back towards the left edge, you could still end up over a thin character? Or am I missing something?

(In reply to comment #35)
> I'm not sure what you mean here. The code doesn't look at the text, no, but by
> moving the caret back towards the left edge, you could still end up over a thin
> character? Or am I missing something?

I mean you are right, since we do not check the text the cursor may end up over a thin character.
Attached patch patch v1.1 (obsolete) — Splinter Review
Hi,
  I fixed that problem, now we let the cursor move as normall but do not clip it. So, we do not have problem of overlaping small characters like i.
Attachment #225206 - Attachment is obsolete: true
Attachment #226050 - Flags: review?(mrbkap)
Attachment #225206 - Flags: review?(mrbkap)
Hi,
  I've not get any feedback on the patch. I also found two other problems when I was working on this:

1- the carret moves out of the frame when we have a long RTL text which goes out of the frame and I press the "End".
2- the caret height scales when we change the text size to be bigger, but its withds does not scale.

I looked into bugzilla but I could not find it there. I'm looking into fixes for these problems now.

Mohsen
Comment on attachment 226050 [details] [diff] [review]
patch v1.1

Sorry for the delay in my commenting on this, but I'm still worried about this patch. This patch would have to be tested a lot to make sure that not clipping it doesn't leave caret 'turds' to the right of the cliprect (which is the reason, afaik for the clipping).

Also, it appears that you're doing these diffs against the 1.8 branch or something. The current caret code on trunk looks quite a bit different (and can probably do what you want), so I'd recommend looking at that instead of the old code.
mrbkap,

> Sorry for the delay in my commenting on this,
It is ok.

> but I'm still worried about this
> patch. This patch would have to be tested a lot to make sure that not clipping
> it doesn't leave caret '****' to the right of the cliprect (which is the
> reason, afaik for the clipping).

I understand that removing clipping from the code would be worrying somehow, and I have just done it for the hook(marker) not the caret itself because of that. On the other hand, if everything is fine the caret should be in the clipping area anyway. Actually, clipping just hides the caret whenever it is in the wrong position and mask some bugs. Actually, I found out the RTL text bug when I commented both clipping while in current code the caret just disappears without giving any idea of what is hepenning.
I think it would be useful to comment clipping at least during development.

> Also, it appears that you're doing these diffs against the 1.8 branch or
> something. The current caret code on trunk looks quite a bit different (and can
> probably do what you want), so I'd recommend looking at that instead of the old
> code.

Actually, I just checked out the code from cvs by running make -f client.mk checkout and setting the variables for projects to checkout. Does'nt this give me the current code in the trunk?

Mohsen
mrbkap:
   do you have any reservation, if I just remove intesect for hookrect and keep it for caret itself. Since, the position and size of hook depends on rect itself, if it is clipped, hook will be moved and clipped.
   This just allows hook to be slightly out of the box and to be visible.

Uri, David, mrbkap:
  have you seen the bug I mentioned before in bugzilla? should I submit them as new bugs? or should I just send the patches to this bug?
Comment on attachment 226050 [details] [diff] [review]
patch v1.1

I wiped out and checked out the source again for both 1.8 and 1.9. This patch is for 1.8 (Firefox 2) and I checked the new code in 1.9 and the problem exist in the new code. I will send another patch for trunk (1.9).
Attachment #226050 - Flags: review?(dbaron)
mrbkap,

I have two questions:
  - can we draw anything outside the frame we are in the new nsCaret code?
  - how can I get the scaling factor when the text has changed using view menu-> increase/decrease?

Mohsen

P.S: are you not sure about the solution for 1.8? is there any other idea?
This is an screenshot of firefox caret hook. Please take a look at the next one and compare it to the one for IE. The Bidi hook is not visible when the text size becomes bigger since it remains the same size since its size depends on the caret width which does not scale with the text size.
the bigger text size
Attached image Caret hook for IE
Attachment #226050 - Flags: review?(dbaron) → review?(roc)
This isn't going to get fixed for 1.8. Concentrate on a 1.9 patch.

For 1.9, you can make the caret+hook as big as you like, although you'll have to refactor the code a bit.
For 1.9, though, you can't really avoid clipping. The caret is painted at about the same time as the text it's associated with, and is subject to the same transformations and effects as the text.
Attached patch P1 (obsolete) — Splinter Review
I think on right-to-left frames, caret should be moved 1 pixel(equal to its width) to the left.
On left-to-right frames, it's okay that the left side of the caret rectangle be equal to the end position of the selected text. But on right-to-left frames the RIGHT side of the caret rectangle must be equal to the end position of the selection.
The problem is worse when the caret width is more than 1 pixel.
Currently on RTL inputs (because of this problem), caret is drawn on the right-padding of anonymous-div and its hook is not visible. But with this patch applied, it moves 1 pixel to the left and its hook will be shown.
Attachment #242697 - Flags: superreview?(roc)
Attachment #242697 - Flags: review?(mrbkap)
Attached image Before and After P1
Different snapshots of sample inputs with following style before and after applying P1 patch.

 input {
   border:	solid 1px red;
   width:	100px;
   font-family:	monospace;
   font-size:	20px;
 }
Comment on attachment 242697 [details] [diff] [review]
P1

Are there such things as empty RTL frames? If so, what does the caret do with this patch? It seems like it'd try to move out of the left side of the frame.

Also, a nit: in this file, braces only go on multi-line if statements (and each brace gets its own line).
This is what exactly happens to LTR frames too.
I think LTR and RTL frames should be mirror of each other in all the senses. Now they differ in caret position. This patch resolves this. 
Blake,
Should I post a notation fixed patch?
Is there any other problem with the patch?
Comment on attachment 242697 [details] [diff] [review]
P1

Okay, let's try it.
Attachment #242697 - Flags: review?(mrbkap) → review+
Attachment #242697 - Attachment is obsolete: true
Attachment #243999 - Flags: superreview?(roc)
Attachment #243999 - Flags: review?(mrbkap)
Attachment #242697 - Flags: superreview?(roc)
Attachment #243999 - Flags: review?(mrbkap) → review+
Attachment #243999 - Flags: superreview?(roc) → superreview+
Whiteboard: [checkin needed]
Comment on attachment 243999 [details] [diff] [review]
[checked in] P1 - Notation fixed

I checked this in on behalf of Haamed:

Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.167; previous revision: 1.166
done
Attachment #243999 - Attachment description: P1 - Notation fixed → [checked in] P1 - Notation fixed
Whiteboard: [checkin needed]
Attachment #226050 - Attachment is obsolete: true
Attachment #226050 - Flags: review?(roc)
Attachment #226050 - Flags: review?(mrbkap)
Is there any particular reason why this bug is still open?  I can't reproduce this on trunk...
ok great
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.