Closed Bug 1080832 Opened 9 years ago Closed 9 years ago

[Messages][RTL] Cursor is badly located in recipients and subject when they're empty

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: julienw, Unassigned)

References

Details

(Whiteboard: [p=1])

Attachments

(2 files)

testcase.html
1.18 KB, text/html
Details
GitHub pull request URL (workaround)
46 bytes, text/x-github-pull-request
julienw
: feedback+
Details | Review 
In RTL mode, the cursor is badly located in the recipients panel and the subject input when it's empty.

We already checked that adding a <br> seems to work around the issue, but there may be a gecko issue. We should create a minimal testcase showing the issue and maybe file a separate bug about it too.
Expected:
* the cursor should be at the right when the document is in RTL mode, and at the left when the document is in LTR mode.
* however, if in LTR the user inputs a first RTL character, or if in RTL the user inputs a first LTR character, then the subject/recipient should move to respectively RTL and LTR. This can be easily achieved with a dir=auto attribute (or possibly with a moz-isolate value for unicode-bidi, but not sure).
p=1 to make a minimal testcase and then ask gecko developers (eg :masayuki) once we have a testcase.
Blocks: sms-sprint-2.1S9
Whiteboard: [p=1]
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Attached file testcase.html — 
Hey Masayuki,

We're trying to make our (Gaia Messages app) works well in RTL mode. In the app we widely use contenteditable divs as the "inputs" for a variety of reasons. Currently we're observing the following strange behaviour that doesn't comply with our RTL requirement:

If you focus empty contenteditable element (without any <br> inside) in RTL mode then caret is displayed in the left most position (like in LTR mode). Once you enter something then caret follows text correctly. Everything works fine for plain inputs and contenteditable elements with nested <br> + no any issue is observed in LTR mode (please see attached test case). So the question is, should it be somehow handled at Gecko level or should we always take care about having at least one <br> inside of contenteditable element?

Daniel, I'm ni? you because this issue happens only if contenteditable element is located inside flex container (that is true for majority of contenteditable elements in Messages app). Maybe you have some thoughts?

Thank you guys!
Flags: needinfo?(masayuki)
Flags: needinfo?(dholbert)
Might very well be a flexbox bug. I'll take a deeper look in the next day or so.
At least for now, I guess authors need to insert <br>.

However, we should insert <br> automatically for compatibility with other browsers. Other browsers guarantee at least one line height for empty contentediable element. But Gecko makes 0-height box. This incompatibility is too bad for us. So, we should also fix this bug in Gecko side.
Flags: needinfo?(masayuki)
Masayuki-san, do you already have a bug for this, or do you want that we file one?
Flags: needinfo?(masayuki)
(In reply to Julien Wajsberg [:julienw] from comment #6)
> Masayuki-san, do you already have a bug for this, or do you want that we
> file one?

I'm not sure. Aryeh, how about you?
Flags: needinfo?(masayuki) → needinfo?(ayg)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> However, we should insert <br> automatically for compatibility with other
> browsers. Other browsers guarantee at least one line height for empty
> contentediable element. But Gecko makes 0-height box. This incompatibility
> is too bad for us. So, we should also fix this bug in Gecko side.

I tested this page in some version or other of IE, Firefox, and Chrome:

  <!DOCTYPE html>
  abc
  <div contenteditable></div>
  def

and confirmed that IE and Chrome don't collapse the div but we do.  This seems sensible enough, so I'm not opposed to changing it, although it definitely has to wait for Ehsan to return before landing.  I'm not aware of an existing bug.
Flags: needinfo?(ayg)
Filed bug 1097649 for this.

In this bug we can stick to the cursor positioning issue. On the testcase in attachment 8520813 [details] we don't collapse thanks to a min-height property, and yet the cursor is badly positioned when we're in a RTL document and the contenteditable is empty in a flex container.

We can workaround the issue in the SMS app, but first let's see if it's an issue in the Flexible Layout implementation.
The testcase's flex-vs.-block positioning difference boils down to this code, which is used for positioning the caret:
> 5887 nsFrame::GetPointFromOffset(int32_t inOffset, nsPoint* outPoint)
> 5888 {
[...]
> 5898       bool isRTL = (NS_GET_EMBEDDING_LEVEL(this) & 1) == 1;
> 5899       if ((!isRTL && inOffset > newOffset) ||
> 5900           (isRTL && inOffset <= newOffset)) {
> 5901         pt = contentRect.TopRight();

If the container is "display:block", isRTL is true there, and we use the top-right corner.  BUT, if it's "display:flex", then isRTL is false, and we skip that "pt" setting.

I don't know what NS_GET_EMBEDDING_LEVEL is about, but we're apparently not setting it correctly, on children of a flex container.
Flags: needinfo?(dholbert)
Thanks for investigation Daniel!

Hey Simon, do you know who can help us to fix the issue mentioned in the comment 10?

Thanks!
Flags: needinfo?(smontagu)
To avoid having too many things going on here, I spun off bug 1097894 for the platform bug about cursor-positioning in a RTL contenteditable flex container. (I actually don't think it's flex-specific -- I can trigger it with "display:table-cell", too, as shown in the testcase attached there.)  I needinfo'd Simon on that bug -- canceling his needinfo here.

Let's leave this bug here focused on fixing the messages app (which might mean just waiting for bug 1097894 to be fixed, or might mean working around this in the app itself).
Flags: needinfo?(smontagu)
In the meantime let's have a workaround as backup plan :)
Attachment #8522036 - Flags: feedback?(felash)
Hey Eitan, left a question for you in https://github.com/mozilla-b2g/gaia/pull/26097/files#r20296152
Thanks !
Flags: needinfo?(eitan)
Comment on attachment 8522036 [details] [review]
GitHub pull request URL (workaround)

lgtm, thanks !
Attachment #8522036 - Flags: feedback?(felash) → feedback+
Depends on: 1097894
I tested the patch with a screen reader, and it does indeed break explore by touch with the screen reader since the entry (ie. editable content) has a width of 0. I would recommend trying this with the screen reader yourself, before and after the patch, to see what I mean.

Here are some instructions:
https://wiki.mozilla.org/Accessibility/Mobile/ScreenReader
Flags: needinfo?(eitan)
Thanks Eitan, I understand the reason even without trying now :)

However, since the whole recipient panel is touchable, maybe it makes sense to add an aria role to the recipient panel that would have the same behavior?
Flags: needinfo?(eitan)
Hey Ehsan, we need your advice here :) Let me describe the problem we're trying to solve:

──html[dir=rtl]
   │
   └──div.recipients
        │
        └──span.phone-or-contact-name-input[contenteditable=true][dir=auto]

Here we have RTL document with [contenteditable=true] as input, it also has [dir=auto] to at least correctly format phone numbers in international format (eg. +1234). The problem here is that the caret is always LTR-oriented if [contenteditable=true][dir=auto] is empty. The first workaround we tried was to make [contenteditable] narrow enough (so that when it's empty only caret is displayed) and properly align it itself - everything works fine except that it's amolst not recongnizable by ScreenReader, so we have to make it as wide as possible.

I've quickly skimmed through the spec [1] and found that at least for textareas we have the following statement:

---------------------------
* If the element is a textarea element and the dir attribute is in the auto state

    ** If the element's value contains a character of bidirectional character type AL or R, and there is no character of bidirectional character type L anywhere before it in the element's value, then the directionality of the element is 'rtl'.

    ** Otherwise, if the element's value is not the empty string, or if the element is a root element, the directionality of the element is 'ltr'.

    ** Otherwise, the directionality of the element is the same as the element's parent element's directionality.
---------------------------

So if I understand it correctly, then if textarea[dir=auto] is empty its direction (thus caret position) should be inherited from the parent (that is RTL in our case). 

Is the same rule valid for [contenteditable=true][dir=auto]/input[dir=auto]?

[1] https://html.spec.whatwg.org/multipage/dom.html#the-dir-attribute
Flags: needinfo?(ehsan.akhgari)
(In reply to Julien Wajsberg [:julienw] from comment #17)
> Thanks Eitan, I understand the reason even without trying now :)
> 
> However, since the whole recipient panel is touchable, maybe it makes sense
> to add an aria role to the recipient panel that would have the same behavior?

Generally, I think that could be hard, since the role should generally attributed to the actual control, in this case, the contenteditable=true element. Doing otherwise could introduce unwanted issues.

Also, an 'entry' role typically will not have 'button' children, I am not sure how that would work.

It is still worth trying and seeing how well it works out though.
Flags: needinfo?(eitan)
Some updates: 

* Subject caret position was indirectly fixed by patch for bug 1078384 (flex was removed from the subject input);

* Workaround for recipients input caret position is introduced in patch for bug 1096330. Though it should be removed once issue in comment 18 is fixed in Gecko, if confirmed by Ehsan of course :)


> Eitan Isaacson [:eeejay] from comment #19
> Also, an 'entry' role typically will not have 'button' children, I am not sure how that would work.
> It is still worth trying and seeing how well it works out though.

Yeah, looks like you're right, "button" elements inside "entry" field don't seem to be recognized by ScreenReader
See Also: → 1103011
Filed a separate bug 1103011 for the issue in comment 18, so moving ni? there.
Flags: needinfo?(ehsan.akhgari)
Since discussion about these issues is moved to another bugs and issues from comment 0 are fixed in the scope of other bugs we can close this one (caret position for subject input was fixed by patch for bug 1078384, recipients input - bug 1096330).

Here are related Gecko bugs just for the record:

* Bug 1098151 - Empty contenteditable elements should have <br> or min-height as one line-height for preventing collappsed;

* Bug 1103011 - Caret position inside empty editable element (input, textarea, contenteditable) with dir=auto doesn't respect parent's directionality;

* Bug 1103348 - Directionality of dir=auto element is not updated when replacing strong-directional contents with all-neutral characters after Select All;

* Bug 904846 - Caret positioning is incorrect in an empty contenteditable element with ::before pseudo;

* Bug 1097894 - [RTL] Caret is mispositioned in empty RTL contenteditable block, when the parent isn't also a block;
Assignee: azasypkin → nobody
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15374/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.