Bug 229798 – Use isRendererReplacedElement for SimplifiedBackwardsTextIterator

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Frédéric Wang (:fredw)]

.

[Frédéric Wang (:fredw)]

Created attachment 437132 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 437151 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 437151 [details]
Patch

Hi Chris, I think you reviewed this part of the code in the past. Can you please take a look?

[Darin Adler]

Comment on attachment 437151 [details]
Patch

I assume the real benefits of this fix are more indirect, for the clients of SimplifiedBackwardsTextIterator, such as the visible boundary functions, which presumably give us incorrect results in these cases.

It’s unfortunate that we don’t really know what those cases are and so can only detect the improvement with a unit test of the iterator itself.

[Frédéric Wang (:fredw)]

Comment on attachment 437151 [details]
Patch

It seems the crashes in mac-debug-wk1 are unrelated, the same happens for https://ews-build.webkit.org/#/builders/56/builds/14425

(In reply to Darin Adler from comment #4)
> Comment on attachment 437151 [details]
> Patch
> 
> I assume the real benefits of this fix are more indirect, for the clients of
> SimplifiedBackwardsTextIterator, such as the visible boundary functions,
> which presumably give us incorrect results in these cases.
>
> It’s unfortunate that we don’t really know what those cases are and so can
> only detect the improvement with a unit test of the iterator itself.

I agree. It seems the new cases were added for the sake of a11y. I wonder if our a11y thing is actually using SimplifiedBackwardsTextIterator. At least the visible boundary function is already emitting "," for some cases, so it looks like we want consistency here.

Also wonder if the isMedia() part is actually even used, we are no longer testing it at all after the move to modern media control. Will try to remove in a separate patch.

[EWS]

Committed r281979 (241286@main): <https://commits.webkit.org/241286@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437151 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82712284>

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #5)
> I agree. It seems the new cases were added for the sake of a11y. I wonder if
> our a11y thing is actually using SimplifiedBackwardsTextIterator. At least
> the visible boundary function is already emitting "," for some cases, so it
> looks like we want consistency here.
> 
> Also wonder if the isMedia() part is actually even used, we are no longer
> testing it at all after the move to modern media control. Will try to remove
> in a separate patch.

OK, I checked again and there are actually tests broken if we remove that. More at bug 229857. It seems the a11y code does not use SimplifiedBackwardsTextIterator, I was not able to trigger the code even with previousTextMarker. So I added another case in backwards-text-iterator-basic.html

[Darin Adler]

(In reply to Frédéric Wang (:fredw) from comment #8)
> It seems the a11y code does not use
> SimplifiedBackwardsTextIterator

AXObjectCache::previousBoundary uses SimplifiedBackwardsTextIterator, and I believe that is accessibility code. Called by AXObjectCache::startCharacterOffsetOfSentence and AXObjectCache::startCharacterOffsetOfWord. Really surprised that accessibility has its own redundant copy of functions VisibleUnits.cpp; worth fixing that some day.

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #9)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > It seems the a11y code does not use
> > SimplifiedBackwardsTextIterator
> 
> AXObjectCache::previousBoundary uses SimplifiedBackwardsTextIterator, and I
> believe that is accessibility code. Called by
> AXObjectCache::startCharacterOffsetOfSentence and
> AXObjectCache::startCharacterOffsetOfWord. Really surprised that
> accessibility has its own redundant copy of functions VisibleUnits.cpp;
> worth fixing that some day.

OK, thanks I hadn't tried to dig more into the code actually.  what I meant is that I did changes to text-marker-previous-next.html in https://bugs.webkit.org/attachment.cgi?id=437274&action=review which uses nextTextMarker/previousTextMarker ; I'm able to break that new test by tweaking TextIterator but not with SimplifiedBackwardsTextIterator.