Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 1080832 - [Messages][RTL] Cursor is badly located in recipients and subject when they are empty. r=julien #26097

Closed
wants to merge 1 commit into from
Closed

Bug 1080832 - [Messages][RTL] Cursor is badly located in recipients and subject when they are empty. r=julien #26097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/sms/index.html
View file
Open in desktop
Expand Up @@ -262,7 +262,7 @@ <h1 id="messages-header-text" aria-hidden="true"></h1>
<section class="subject-composer js-subject-composer hide">
<div contentEditable="true" class="subject-composer-input"
role="textbox"
aria-multiline="false">
aria-multiline="false"><br />
</div>
<div class="subject-composer-placeholder"
data-l10n-id="messagesSubjectInput_placeholder">
Expand Down
19 changes: 17 additions & 2 deletions apps/sms/js/subject_composer.js
View file
Open in desktop
Expand Up @@ -82,6 +82,13 @@
return newValue;
}

function clearInput(input) {
// Currently Gecko doesn't insert automatically <br> inside empty content
// editable element, but to be compliant with the editor spec it should.
// Once Gecko is fixed we should remove this hack. See bug 1098151.
input.innerHTML = '<br />';
}

var SubjectComposer = function(node) {
EventDispatcher.mixin(this, [
'focus',
Expand Down Expand Up @@ -161,13 +168,21 @@
throw new Error('Value should be a valid string!');
}

priv.input.textContent = priv.updateValue(value);
var updatedValue = priv.updateValue(value);

if (updatedValue) {
priv.input.textContent = updatedValue;
} else {
clearInput(priv.input);
}
};

SubjectComposer.prototype.reset = function ms_reset() {
var priv = privateMembers.get(this);

priv.value = priv.input.textContent = '';
clearInput(priv.input);

priv.value = '';
priv.node.classList.add('hide');
priv.isVisible = false;

Expand Down
1 change: 0 additions & 1 deletion apps/sms/style/sms.css
View file
Open in desktop
Expand Up @@ -717,7 +717,6 @@ Do not remove.
}

#messages-recipients-list > .recipient[contenteditable=true] {
flex-grow: 1;
height: 2.8rem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @eeejay do you remember why you added this in #23013 ? From our point of view it's useless (because we programmatically give the focus if the user click in a blank space in the panel) but maybe you had a reason for a11y?

The consequence of this property is that a recipient being edited takes the whole available space. Without this property it takes only the space its content needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an accessibility perspective, that empty space that a user clicks needs to have an appropriate role. I had the entry grow and take up all that space, so that a blind user will have an easier time finding it. In other words, the recipient list looks like this in accessibility:
[recipient button], [recipient button], [entry ]

line-height: 2.8rem;
border-radius: 0;
Expand Down
9 changes: 9 additions & 0 deletions apps/sms/test/unit/subject_composer_test.js
View file
Open in desktop
Expand Up @@ -99,6 +99,8 @@ suite('SubjectComposer >', function() {
() => subjectComposer.setValue(), 'Value should be a valid string'
);
assert.equal(subjectComposer.getValue(), '');
// See bug bug 1098151 for details
assert.equal(input.innerHTML.trim(), '<br>');

subjectComposer.setValue('test');

Expand Down Expand Up @@ -133,6 +135,11 @@ suite('SubjectComposer >', function() {
'Line 1<br> Line 2<br><br><br><br> Line 3<br>'
);

subjectComposer.setValue('');
assert.equal(subjectComposer.getValue(), '');
// See bug bug 1098151 for details
assert.equal(input.innerHTML.trim(), '<br>');

subjectComposer.setValue('');
input.innerHTML = content;
input.dispatchEvent(new InputEvent('input'));
Expand Down Expand Up @@ -346,6 +353,8 @@ suite('SubjectComposer >', function() {

assert.equal(subjectComposer.getValue(), '');
assert.equal(input.textContent, '');
// See bug bug 1098151 for details
assert.equal(input.innerHTML.trim(), '<br>');
assert.isFalse(subjectComposer.isVisible());
// Visibility change and change events should not be fired on reset
sinon.assert.notCalled(onChangeStub);
Expand Down