Closed Bug 1105611 Opened 10 years ago Closed 9 years ago

nsIAccessibleEditableText doesn't support contenteditable editor which has pseudo elements (::before and/or ::after)

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(4 files, 4 obsolete files)

When I work on bug 1098151, I realized that nsIAccessibleEditableText doesn't support contenteditable editors which has pseudo elements like ::before and/or ::after.

For example, setTextContents tries to remove them first, then, trying to insert text *before* ::before pseudo element. That will fail in nsPlaintextEditor, of course.
I have a question. methods of nsIAccessibleEditableText take offsets of text content. Should the offset include pseudo element's text? For example,

+---------------------------------------------------------+
|+--------+                                      +-------+|
|| before | content                              | after ||
|+--------+                                      +-------+|
+---------------------------------------------------------+

In this case, should offset 0 be the "c" of "content"? Or "b" of "before"? And should the text length of this element is 7 (content)? Or 18 (before + content + after)?
Flags: needinfo?(surkov.alexander)
:before and :after stuff should be part of the text (and be achievable via text interface) but it's not implemented yet in Firefox.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #3)
> :before and :after stuff should be part of the text (and be achievable via
> text interface) but it's not implemented yet in Firefox.

Okay, then, setTextContents should replace only the "content" at least for now, right? I.e., 6 - 13 should be editable with nsIAccessibleEditableText?
Flags: needinfo?(surkov.alexander)
if it's not difficult to implement replacing all content including before/after then I would say to do so, otherwise only the content replaced is fine I think
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #5)
> if it's not difficult to implement replacing all content including
> before/after then I would say to do so, otherwise only the content replaced
> is fine I think

I think that the former is impossible because ::before and ::after are appended from CSS, not from (visible) DOM tree. So, only the latter is possible.
those are also DOM in the end, but I guess every reflow will throw away your changes. Changing the style is not probably something that setTextContent should do. So, yeah, I think changing the 'content' only is a way to go
Okay, editabletext.js should support testing in contenteditable elements which have ::before and/or ::after.

For that, editableTextTest should take before content and after content as optional arguments.

If aBeforeContent is has text, editableTextTest should treat offset 0 is the start of "content.
Assignee: nobody → masayuki
Attachment #8529495 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8531073 - Flags: review?(surkov.alexander)
For making HyperTextAccessible ignore pseudo elements at selecting content and editing text, we need to fixes:

1. When replacing whole text, it should select all contents in the editor because if there is ::after pseudo element, editor may insert <br> for guaranteeing one line height to the element which becomes empty. At replacing whole contents, we need to remove the odd <br> element before inserting the specified text.

2. Set selection range outside of pseudo elements. If specified range start or end is in ::before, actual range start or end should be 0 of its parent. If specified range start or end is in ::after, actual range start or end should be the last child of the parent. This means that ::before and ::after pseudo elements never included into the range if range is between ::before and ::after. On the other hand, this also means that ::before and ::after pseudo elements are always included into the range if the range is between ::after of an element and ::before of another element. I.e.,

The former guarantees:
+---------------------------------------------------+
|+--------------+                  +---------------+|
||::before      |abcdefghijklmnopqr|::after        ||
|+--------------+                  +---------------+|
+---------------------------------------------------+
                 <--  selected  -->

The latter case guarantees:
-------------------++------------------
  +---------------+||+--------------+
  |::after        ||||::before      |
  +---------------+||+--------------+
-------------------++------------------
 <------------- selected ------------>
Attachment #8531075 - Flags: review?(surkov.alexander)
what happens if you try to set a range to pseudo elements content?
(In reply to alexander :surkov from comment #10)
> what happens if you try to set a range to pseudo elements content?

If it's ::before, immediately after it and inside its parent.

If it's ::after, immediately before it and inside its parent too.
I meant without your patch, if you would use OffsetToDOMPoint() method to set a range
(In reply to alexander :surkov from comment #12)
> I meant without your patch, if you would use OffsetToDOMPoint() method to
> set a range

It creates a range in a dummy node of pseudo element, its valid only in Gecko. Therefore, it succeeds to select inside a ::before or ::after. However, it's not editable. Therefore, nsIEditor.deleteSelection() and nsIPlaintextEditor.insertText() fail to edit the node with warning messages.
Comment on attachment 8531075 [details] [diff] [review]
part.2 HyperTextAccessible should set DOM range outside of pseudo elements

Review of attachment 8531075 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/HyperTextAccessible.cpp
@@ +368,5 @@
>    DOMPoint startPoint = OffsetToDOMPoint(aStartOffset);
>    if (!startPoint.node)
>      return false;
>  
> +  nsIContent* container = GetNearestElementContent(startPoint.node);

you don't need more than two iterations, right? so can you do
nsIContent* elm = startPoint.node->IsElement() ? startPoint.node : startPoint.node->GetParent();

@@ +379,5 @@
> +    aRange->SetStart(container->GetParent(),
> +                     container->GetParent()->GetChildCount());
> +  } else {
> +    aRange->SetStart(startPoint.node, startPoint.idx);
> +  }

it'd be good to have method for that to avoid copy below like ClosestNotGeneratedDOMPoint()

also it'd be good to have a comment why OffsetToDOMRange needs this stuff opposite to OffsetToDOMPoint
Attached patch bug1105611-2.diff (obsolete) — Splinter Review
Hi, sorry for the delay.

(In reply to alexander :surkov from comment #14)
> ::: accessible/generic/HyperTextAccessible.cpp
> @@ +368,5 @@
> >    DOMPoint startPoint = OffsetToDOMPoint(aStartOffset);
> >    if (!startPoint.node)
> >      return false;
> >  
> > +  nsIContent* container = GetNearestElementContent(startPoint.node);
> 
> you don't need more than two iterations, right? so can you do
> nsIContent* elm = startPoint.node->IsElement() ? startPoint.node :
> startPoint.node->GetParent();

Looks like that text node is always child of ::before or ::after. So, we can check only a node and its parent. However, your code could return an nsIContent which is not an element. Therefore, I think that parent node should be checked if it's an element. See this patch what I did.

> @@ +379,5 @@
> > +    aRange->SetStart(container->GetParent(),
> > +                     container->GetParent()->GetChildCount());
> > +  } else {
> > +    aRange->SetStart(startPoint.node, startPoint.idx);
> > +  }
> 
> it'd be good to have method for that to avoid copy below like
> ClosestNotGeneratedDOMPoint()

No, the below calls nsRange::SetEnd(). So, they are not just copied. And we cannot make a simple method for this.

> also it'd be good to have a comment why OffsetToDOMRange needs this stuff
> opposite to OffsetToDOMPoint

I agree.
Attachment #8531075 - Attachment is obsolete: true
Attachment #8531075 - Flags: review?(surkov.alexander)
Attachment #8533109 - Flags: review?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> Created attachment 8533109 [details] [diff] [review]
> bug1105611-2.diff
> 
> Hi, sorry for the delay.

me too

> (In reply to alexander :surkov from comment #14)

> See this patch what I did.

yeah but do you need a method for that?

> > @@ +379,5 @@
> > > +    aRange->SetStart(container->GetParent(),
> > > +                     container->GetParent()->GetChildCount());
> > > +  } else {
> > > +    aRange->SetStart(startPoint.node, startPoint.idx);
> > > +  }
> > 
> > it'd be good to have method for that to avoid copy below like
> > ClosestNotGeneratedDOMPoint()
> 
> No, the below calls nsRange::SetEnd(). So, they are not just copied.

sure, but I meant to have something like

DOMPoint closestPoint = ClosestNotGeneratedDOMPoint(startPoint);
aRange->SetStart(closestPoint.node, closestPoint.idx);

closestPoint = ClosestNotGeneratedDOMPoint(endPoint);
aRange->SetEnd(closestPoint.node, closestPoint.idx);
Indeed, it makes sense!
Attachment #8533109 - Attachment is obsolete: true
Attachment #8533109 - Flags: review?(surkov.alexander)
Attachment #8535452 - Flags: review?(surkov.alexander)
Comment on attachment 8535452 [details] [diff] [review]
part.2 HyperTextAccessible should set DOM range outside of pseudo elements

Review of attachment 8535452 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/HyperTextAccessible-inl.h
@@ +64,5 @@
> +  if (!plaintextEditor) {
> +    return;
> +  }
> +  // Move focus, first.
> +  SetSelectionRange(0, 0);

if DeleteText() was called unconditionally then you don't need that?

@@ +67,5 @@
> +  // Move focus, first.
> +  SetSelectionRange(0, 0);
> +  // DeleteText() may cause inserting <br> element in some cases. Let's
> +  // select all again and replace whole contents.
> +  editor->SelectAll();

does SetSelectionRange(0, CharactedCount()) works?

::: accessible/generic/HyperTextAccessible.cpp
@@ +443,5 @@
> +    nsIContent* parent = aDOMPoint.node->GetParent();
> +    if (parent && parent->IsElement()) {
> +      container = parent;
> +    }
> +  }

makes sense to move this out of the method to not calculate it twice

::: accessible/generic/HyperTextAccessible.h
@@ +137,5 @@
>    uint32_t TransformOffset(Accessible* aDescendant, uint32_t aOffset,
>                             bool aIsEndOffset) const;
>  
>    /**
>     * Convert start and end hypertext offsets into DOM range.

it'd be good to add that the method excludes generated content from returned range, perhaps instead long params descriptions, up to you
(In reply to alexander :surkov from comment #19)
> ::: accessible/generic/HyperTextAccessible-inl.h
> @@ +64,5 @@
> > +  if (!plaintextEditor) {
> > +    return;
> > +  }
> > +  // Move focus, first.
> > +  SetSelectionRange(0, 0);
> 
> if DeleteText() was called unconditionally then you don't need that?

I guess so.

> @@ +67,5 @@
> > +  // Move focus, first.
> > +  SetSelectionRange(0, 0);
> > +  // DeleteText() may cause inserting <br> element in some cases. Let's
> > +  // select all again and replace whole contents.
> > +  editor->SelectAll();
> 
> does SetSelectionRange(0, CharactedCount()) works?

Hmm, IIRC, it doesn't work. It caused something asserts or errors in editor. I'll check it on Monday.

> ::: accessible/generic/HyperTextAccessible.cpp
> @@ +443,5 @@
> > +    nsIContent* parent = aDOMPoint.node->GetParent();
> > +    if (parent && parent->IsElement()) {
> > +      container = parent;
> > +    }
> > +  }
> 
> makes sense to move this out of the method to not calculate it twice

I'm not sure what you meant. Did you mean that I should create a method like:

static nsIContent* GetContentIfElement(nsINode* aNode)
{
  return aNode->IsElement() ? aNode->AsContent() : nullptr;
}

?

And it should be called in ClosestNotGeneratedDOMPoint() like:

nsIContent* container = GetContentIfElement(aDOMPoint.node);
if (!container) {
  container = GetContentIfElement(aDOMPoint.node->GetParent());
}

?

> ::: accessible/generic/HyperTextAccessible.h
> @@ +137,5 @@
> >    uint32_t TransformOffset(Accessible* aDescendant, uint32_t aOffset,
> >                             bool aIsEndOffset) const;
> >  
> >    /**
> >     * Convert start and end hypertext offsets into DOM range.
> 
> it'd be good to add that the method excludes generated content from returned
> range, perhaps instead long params descriptions, up to you

Okay.
Flags: needinfo?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)

> > makes sense to move this out of the method to not calculate it twice
> 
> I'm not sure what you meant. Did you mean that I should create a method like:

I meant something like this

HyperTextAccessible::OffsetsToDOMRange()
{
  nsIContent* container = nullptr;
  // calcualte it
  DOMPoint startPointForDOMRange = ClosestNotGeneratedDOMPoint(container);
}
Flags: needinfo?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> (In reply to alexander :surkov from comment #19)
> > @@ +67,5 @@
> > > +  // Move focus, first.
> > > +  SetSelectionRange(0, 0);
> > > +  // DeleteText() may cause inserting <br> element in some cases. Let's
> > > +  // select all again and replace whole contents.
> > > +  editor->SelectAll();
> > 
> > does SetSelectionRange(0, CharactedCount()) works?
> 
> Hmm, IIRC, it doesn't work. It caused something asserts or errors in editor.
> I'll check it on Monday.

This hits:

> Assertion failure: aIndex < Length() (invalid array index), at d:\mozilla\mc-a\fx-dbg\dist\include\nsTArray.h:946
> #01: gfxSkipCharsIterator::SetSkippedOffset (d:\mozilla\mc-a\src\gfx\thebes\gfxskipchars.cpp:86)
> #02: mozilla::a11y::HyperTextAccessible::RenderedToContentOffset (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible.cpp:1860)
> #03: mozilla::a11y::HyperTextAccessible::OffsetToDOMPoint (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible.cpp:417)
> #04: mozilla::a11y::HyperTextAccessible::OffsetsToDOMRange (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible.cpp:377)
> #05: mozilla::a11y::HyperTextAccessible::SetSelectionBoundsAt (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible.cpp:1531)
> #06: mozilla::a11y::HyperTextAccessible::SetSelectionRange (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible.cpp:1224)
> #07: mozilla::a11y::HyperTextAccessible::ReplaceText (d:\mozilla\mc-a\src\accessible\generic\hypertextaccessible-inl.h:72)
> #08: mozilla::a11y::xpcAccessibleHyperText::SetTextContents (d:\mozilla\mc-a\src\accessible\xpcom\xpcaccessiblehypertext.cpp:459)
(In reply to alexander :surkov from comment #21)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> 
> > > makes sense to move this out of the method to not calculate it twice
> > 
> > I'm not sure what you meant. Did you mean that I should create a method like:
> 
> I meant something like this
> 
> HyperTextAccessible::OffsetsToDOMRange()
> {
>   nsIContent* container = nullptr;
>   // calcualte it
>   DOMPoint startPointForDOMRange = ClosestNotGeneratedDOMPoint(container);
> }

Hmm, ClosestNotGeneratedDOMPoint() may receive different elements. So, I don't like this design, though. (This approach means that aDOMPoint and aElementContent must be valid relation. Callers need to guarantee that. I.e., ClosestNotGeneratedDOMPoint() may not work fine if caller don't pass wrong content.)
Attachment #8535452 - Attachment is obsolete: true
Attachment #8535452 - Flags: review?(surkov.alexander)
Attachment #8536362 - Flags: review?(surkov.alexander)
Comment on attachment 8536362 [details] [diff] [review]
part.2 HyperTextAccessible should set DOM range outside of pseudo elements

Review of attachment 8536362 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/HyperTextAccessible-inl.h
@@ +69,5 @@
> +  // DeleteText() may cause inserting <br> element in some cases. Let's
> +  // select all again and replace whole contents.
> +  editor->SelectAll();
> +
> +  plaintextEditor->InsertText(aText);

if you add few words about why we don't use HyperTextAccessible::InsertText() then it'd be great

::: accessible/generic/HyperTextAccessible.cpp
@@ +396,5 @@
>    if (!endPoint.node)
>      return false;
>  
> +  if (startPoint.node != endPoint.node) {
> +    container = GetElementAsContentOf(endPoint.node);

I missed they may have different containers, thus if you get back to previous version where GetElementAsContentOf was part of ClosestNotGeneratedDOMPoint() is totally fine
Attachment #8536362 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8531073 [details] [diff] [review]
part.1 Add tests of nsIAccessibleEditableText with contentediable editors which have ::before or ::after

Review of attachment 8531073 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/editabletext/editabletext.js
@@ +35,2 @@
>  {
> +  this.firstEditableOffset = typeof aBeforeContent === "string" ? aBeforeContent.length : 0;

you know I think  I would change rather the tests then testing function, since it adds more complexity to testing. In other words I would change editableTextTest and fix arguments there for generated content.
(In reply to alexander :surkov from comment #25)
> Comment on attachment 8531073 [details] [diff] [review]
> part.1 Add tests of nsIAccessibleEditableText with contentediable editors
> which have ::before or ::after
> 
> Review of attachment 8531073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/editabletext/editabletext.js
> @@ +35,2 @@
> >  {
> > +  this.firstEditableOffset = typeof aBeforeContent === "string" ? aBeforeContent.length : 0;
> 
> you know I think  I would change rather the tests then testing function,
> since it adds more complexity to testing. In other words I would change
> editableTextTest and fix arguments there for generated content.

So, should I wait something you will do, or, should I just land only part.2 with fixing the points reviewed above?
Flags: needinfo?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)

> > you know I think  I would change rather the tests then testing function,
> > since it adds more complexity to testing. In other words I would change
> > editableTextTest and fix arguments there for generated content.
> 
> So, should I wait something you will do, or, should I just land only part.2
> with fixing the points reviewed above?

well, saying "I would do" I meant "I would do if I was you" :) As long as you don't have existing tests regressions I think you are ok to land the patch but of course having more test coverage is a better way to go. I don't really mind about your approach for the test suite, it just look more complicated than it could be. So if you are ok to spend a little more time to polish test part then it'd really cool.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #27)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> 
> > > you know I think  I would change rather the tests then testing function,
> > > since it adds more complexity to testing. In other words I would change
> > > editableTextTest and fix arguments there for generated content.
> > 
> > So, should I wait something you will do, or, should I just land only part.2
> > with fixing the points reviewed above?
> 
> well, saying "I would do" I meant "I would do if I was you" :)

Ah, I see.

> As long as
> you don't have existing tests regressions I think you are ok to land the
> patch but of course having more test coverage is a better way to go.

Yeah, I agree. But it's not clear what should I do. You said "I would change editableTextTest and fix arguments there for generated content". It sounds like what I did. Did you mean canceling the change of arguments and editableTextTest should detect ::before and/or ::after automatically? It might be possible to retrieve with CSS OM, but I'm not sure.

> So if you are ok to spend a little more time
> to polish test part then it'd really cool.

I don't have so much time because I'm working on redesigning IME APIs for B2G, but I can stay if I can do it quickly.
Flags: needinfo?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)

> Yeah, I agree. But it's not clear what should I do. You said "I would change
> editableTextTest and fix arguments there for generated content". It sounds
> like what I did. Did you mean canceling the change of arguments and
> editableTextTest should detect ::before and/or ::after automatically? It
> might be possible to retrieve with CSS OM, but I'm not sure.

sorry for being unclear, I think I mentioned wrong function name. I meant it'd be good to avoid automatic offset computations by means of this.firstEditableOffset. Instead we could change addTestEditable, for example,

et.scheduleTest(et.setTextContents, "hello", aBeforeLen, Len - aAfterLen);

What you do also makes sense and if you like the approach then I think it can be taken. However the patch has few disputable parts, for example, this.copyText excludes generated content. Also you introduce aAfterContent argument for editableTextTest but never use it, however it seems like you should in setTextContents

> > So if you are ok to spend a little more time
> > to polish test part then it'd really cool.
> 
> I don't have so much time because I'm working on redesigning IME APIs for
> B2G, but I can stay if I can do it quickly.

sure, I understand, sometimes bugs take more time than we wanted to spend. Ok, let's see if we can figure out the approach that we like both, that shouldn't take much time and then it may have some easy implementation.
Flags: needinfo?(surkov.alexander)
Hmm, do you really like this better than the previous patch?
Attachment #8540145 - Flags: review?(surkov.alexander)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> Created attachment 8540145 [details] [diff] [review]
> part.1 Add tests of nsIAccessibleEditableText with contentediable editors
> which have ::before or ::after
> 
> Hmm, do you really like this better than the previous patch?

it's not exactly what I meant. I meant to feed offsets for testing (not strings), comment #29. So we could rid all magic related with generated content offsets, i.e. if generated content offset affects on result then point out that explicitly.
Flags: needinfo?(masayuki)
Hi, sorry for the delay but I don't have much time for this now...
Flags: needinfo?(masayuki)
Attached patch test partSplinter Review
fishing test part (skip disabled tests for now)
Attachment #8568728 - Flags: review?(yzenevich)
Attachment #8531073 - Flags: review?(surkov.alexander)
Attachment #8540145 - Flags: review?(surkov.alexander)
Comment on attachment 8568728 [details] [diff] [review]
test part

Review of attachment 8568728 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/editabletext/test_1.html
@@ +24,4 @@
>      {
>        var et = new editableTextTest(aID);
> +      var startOffset = aBeforeContent ? aBeforeContent.length : 0;
> +      var endOffset = aAfterContent ? aAfterContent.length : 0;

Do we actually use this?
not used currently
Comment on attachment 8568728 [details] [diff] [review]
test part

Review of attachment 8568728 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with 2 comments addressed.

::: accessible/tests/mochitest/editabletext/editabletext.js
@@ +65,5 @@
>        var acc = getAccessible(aID, nsIAccessibleEditableText);
>        acc.setTextContents(aValue);
>      }
>  
> +    var skipOffset = aSkipStartOffset ? aSkipStartOffset : 0;

Nit: no need for skipOffset, lets just use aSkipStartOffset.
aSkipStartOffset = aSkipStartOffset || 0;

::: accessible/tests/mochitest/editabletext/test_1.html
@@ +24,4 @@
>      {
>        var et = new editableTextTest(aID);
> +      var startOffset = aBeforeContent ? aBeforeContent.length : 0;
> +      var endOffset = aAfterContent ? aAfterContent.length : 0;

Lets remove it then
Attachment #8568728 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/61d2c27782aa
https://hg.mozilla.org/mozilla-central/rev/e2b73c8f6638
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
yzen and surkov:

Thank you for your work. And I'm very sorry for that I couldn't finish the work by myself.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #38)
> yzen and surkov:
> 
> Thank you for your work. And I'm very sorry for that I couldn't finish the
> work by myself.

no worries, thanks for the work, hope to see you one day around a11y again
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: