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

Status: VERIFIED FIXED

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

Description

[Prognathous]

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.

Comment 1

[Prognathous]

Attachment: IE6 vs. Mozilla 1.6

Comment 2

[Uri Bernstein]

Seeing on OS X => All/All

Comment 3

[Uri Bernstein]

Attachment: patch

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)

Comment 4

[Uri Bernstein]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, there was probably more work put into making it compatible with Nav4, and then there was a change of course.)

Comment 7

[Uri Bernstein]

Attachment: testcase for answering comment #5

Comment 8

[Uri Bernstein]

Attachment: screenshot, before & after

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).

Comment 9

[Uri Bernstein]

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.

Comment 10

[Uri Bernstein]

Attachment: screenshot from Win/IE

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm a bit uncomfortable with changes like this, but I'd like to hear opinions from roc and bzbarsky.

Comment 12

[Boris Zbarsky [: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?

Comment 13

[Uri Bernstein]

(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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah.  So the size="" case gets broken.  That's really rather unfortunate; could we avoid breaking that somehow?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Uri Bernstein]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 18

[Uri Bernstein]

Attachment: patch v2

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).

Comment 19

[Uri Bernstein]

Attachment: before & after in quirks mode

"after" is with patch v2. Notice that the cols/size (and default) controls become wider to adjust for the paddings.

Comment 20

[Uri Bernstein]

Attachment: before & after in standards mode

Same thing, in standards mode.

Comment 21

[Uri Bernstein]

(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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Uri Bernstein]

(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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

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"?

Comment 25

[Uri Bernstein]

(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.

Comment 26

[Uri Bernstein]

Attachment: current build, large text

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Uri Bernstein]

Attachment: 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?

Comment 29

[Uri Bernstein]

I submitted bug 326123 for the issue in point #2 of comment #28 (adding a 1px padding).

Comment 30

[Mohsen Naderi]

I'm trying to implement method 4 since method 5 seems to be more difficult to implement in the current code. Any ideas?

Comment 31

[Mohsen Naderi]

Attachment: patch v1.0

Hi,
  please find the patch attached. I moved caret hook direction detection upward to make it possible to detect hook prblem on the corners.

Comment 32

[Uri Bernstein]

Comment on attachment 225206 [details] [diff] [review]
patch v1.0

Sorry, but I'm not qualified to review this code. Try mrbkap, perhaps.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 225206 [details] [diff] [review]
patch v1.0

Does this patch bump the caret back over thin characters, like 't' and 'i'?

Comment 34

[Mohsen Naderi]

(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. 

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

(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?

Comment 36

[Mohsen Naderi]

(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.

Comment 37

[Mohsen Naderi]

Attachment: patch v1.1

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.

Comment 38

[Mohsen Naderi]

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 39

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 40

[Mohsen Naderi]

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

Comment 41

[Mohsen Naderi]

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 42

[Mohsen Naderi]

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).

Comment 43

[Mohsen Naderi]

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?

Comment 44

[Mohsen Naderi]

Attachment: Firefox screenshot (normal text size)

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.

Comment 45

[Mohsen Naderi]

Attachment: hook size for caret on bigger text size

the bigger text size

Comment 46

[Mohsen Naderi]

Attachment: Firefox screenshot for caret hook (big text size)

Comment 47

[Mohsen Naderi]

Attachment: Caret hook for IE

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 50

[Haamed Gheibi]

Attachment: P1

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.

Comment 51

[Haamed Gheibi]

Attachment: 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 52

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 53

[Haamed Gheibi]

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. 

Comment 54

[Haamed Gheibi]

Blake,
Should I post a notation fixed patch?
Is there any other problem with the patch?

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 242697 [details] [diff] [review]
P1

Okay, let's try it.

Comment 56

[Haamed Gheibi]

Attachment: [checked in] P1 - Notation fixed

Comment 57

[Uri Bernstein]

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

Comment 58

[(no longer active)]

Is there any particular reason why this bug is still open?  I can't reproduce this on trunk...

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ok great